-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add plugins support in auto update #292
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
base: main
Are you sure you want to change the base?
Conversation
rem1niscence
left a comment
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.
A refactor of the autoupdater code is required in order for this to be scalable and merged for production usage, in the comments I left multiple suggestions on how to do it but basically it resumes into
- not creating specialized structs
PluginReleasebut augment the ones that already existsRelease - Do not create special logic for plugin lifetime on the coordinator but reuse the
Supervisorto manage the plugins through it, and the coordinator will simply need to loop through them and update as needed - This is not on the comments but also required: Right now the plugin update and canopy release update are done together, that is dangerous and could leave the system on a broken state if any of the plugin fails for whatever reasone, the canopy binary must always run so the plugin lifetime/update must be done separate from the canopy binary
cea6e1e to
f6c835f
Compare
rem1niscence
left a comment
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.
The main takeaway from this review is to not have any hardcoded values in both coordinator.go and updater.go. Add those values to the main.go and pass them as configs
…coded in the same place
b75f4dd to
3a87c6a
Compare
513aea3 to
47d4b46
Compare
47d4b46 to
f871cc1
Compare
rem1niscence
left a comment
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.
code is getting better, keep up the good work
|
Do not merge yet |
Add plugins support in auto update