-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create generic tedge configuration plugin #2238
Conversation
5a428bb
to
b6b05dd
Compare
Robot Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's still draft, I didn't dive into details, but Cargo.toml
you can fix quickly.
b6b05dd
to
2869fb6
Compare
cc64c39
to
55d272a
Compare
b124d00
to
8b3b347
Compare
58fb1bb
to
5d7b18d
Compare
Codecov Report
Additional details and impacted files
|
23f5b38
to
f3f531d
Compare
1f08877
to
bd1b9cd
Compare
919be7e
to
0e1140a
Compare
7d9b22d
to
d67ce56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not finishing review, but I would submit now and will continue review tomorrow.
My general questions is:
- Do we need to create a library crate to avoid the code duplication between
c8y-configuration-manager
andtedge-configuration-manager
?
I would also remark that this implementation omits timer_actor
, which exists in c8y-configuration-manager
. Need to think which one is appropriate one to address the timeout, mapper or plugin?
2d67a03
to
601d2e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested according to the spec. It works, and addressing error cases! Very good job!
Almost all of my comments are minor. The exception is the default contents of the tedge-configuration-plugin.toml
file, but it should be one-liner change, so you only need 1 minute to fix it.
For others, If you have time, you can modify them. But don't spend too much time. If you feel it's too much change, then stop and we can merge this PR.
Signed-off-by: Krzysztof Piotrowski <[email protected]>
Signed-off-by: Krzysztof Piotrowski <[email protected]>
aa6f953
to
792a445
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Krzysztof Piotrowski <[email protected]>
Signed-off-by: Krzysztof Piotrowski <[email protected]>
792a445
to
f75f5c2
Compare
Proposed changes
This PR adds generic configuration plugin
TODO:
device-topic-root
anddevice-topic-identifier
with the one from tedge configTypes of changes
Paste Link to the issue
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments