Skip to content

Conversation

stoyan-zoubev
Copy link
Contributor

@stoyan-zoubev stoyan-zoubev commented Jun 2, 2023

[#2] Initial Contribution of the Update Manager

This is the initial contribution of the implementation of a device update manager as proposed.
More technical documentation will follow with additional PRs.

Signed-off-by: Stoyan Zoubev [email protected]

- golang version bumped to 1.19.4 in validation.yaml
- fixed broken links in README.md file
- removed commented/unused code from source files
- use strings.Builder for string concatenation in toActionsString function.
Copy link
Contributor

@k-gostev k-gostev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the NOTICE file we add only what is included in the go binary file. This can be checked with go version -m <binary>. It is nice that there are not any absences from what I checked, however some additions are not needed.

Copy link
Contributor

@k-gostev k-gostev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some general recommendations, nits and some things that I am just curious about for the api/agent. I will continue later on with the other packages.

the imports in three groups that is from top to bottom: standard libraries, internal libraries, external libraries, all separated by a new line.


Signed-off-by: Stoyan Zoubev <[email protected]>
These are not needed, according to the Eclipse Kanto Project Lead.

Signed-off-by: Stoyan Zoubev <[email protected]>
@stoyan-zoubev stoyan-zoubev requested a review from k-gostev June 6, 2023 11:40
Copy link
Contributor

@k-gostev k-gostev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another batch with general recommendations, nits and suggestions, plus a couple of questions that I have. I did not went trough all files, I still need to cover some more ground, so thanks for your patience.

@stoyan-zoubev stoyan-zoubev requested a review from k-gostev June 6, 2023 16:01
Copy link
Contributor

@k-gostev k-gostev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another set of some minor stuff here and there. I still need to go trough the domain and orchestration packages.

Copy link
Contributor

@k-gostev k-gostev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another set of some minor stuff here and there. I still need to go trough the domain and orchestration packages.

@k-gostev k-gostev linked an issue Jun 8, 2023 that may be closed by this pull request
- DesiredStateClient's Unsubscribe method to return error.
- fixed typos, missing doc, wrong text descriptions.
- removed incorrect log messages, extend log message with extra info.
- some test case improvements.

Signed-off-by: Stoyan Zoubev <[email protected]>
Copy link
Contributor

@k-gostev k-gostev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit more general suggestions from me

Copy link
Contributor

@k-gostev k-gostev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments regarding the unit tests. Whether you chose to fix them now, or in the incoming test PR, those comments should be addressed.

- moved main package into nested directory cmd/update-manager
- only containers update agent remains as default updage agents config
- mqtt topics pre-generated, no concatenations on every request
- other minor source code improvements
@stoyan-zoubev stoyan-zoubev force-pushed the initial-contribution branch from 7c7716a to de86738 Compare June 12, 2023 12:19
- all byte[] parameters replaced with concrete types
- serialization/deserialization to/from byte[] moved to mqtt clients
- renaming: Publish -> Send, Connect/Subscribe -> Start, Disconnect/Unsubscribe -> Stop
- unit test case improvements
- mock file regenerated
@stoyan-zoubev stoyan-zoubev changed the title [#216] Initial contribution of UpdateManager to Eclipse Kanto. Initial contribution of UpdateManager to Eclipse Kanto Jun 15, 2023
@k-gostev k-gostev merged commit b91f0c3 into eclipse-kanto:main Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial Contribution of the Update Manager
4 participants