-
Notifications
You must be signed in to change notification settings - Fork 12
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
Implement Helm library #16
Conversation
Manual testing:
|
Why does your binary expose |
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.
Really nice work overall! Leaving some comments
I assumed that we wanted to have k8s
only talk with k8sd
. Is this simply a PoC that shows that this works, and a separate PR will move things to a components
endpoint?
As for the charts, I would prefer keeping the tar.gz
around unless we have to edit it somehow, things would be a lot more tidy (and has no functional differences)
I am also missing some tooling/README/doc that describes the process of upgrading the bundled charts (how do we fetch them? how do we tell k8s
to use the new version? this could look like helm pull ...
and update src/k8s/component/component.componentMap
accordingly.
As a final comment, I would love to be able to quickly tell the component version, e.g. have things like:
charts/coredns-1.11.1.tgz
charts/cilium-1.15.0.tgz
...
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.
Great work @mateoflorido!
One thing:
We want the CLI to only interact with the REST API that is exposed by k8sd
. I assume the current CLI implementation is for testing and will eventually moved to an /component
endpoint in a future PR?
Co-authored-by: Angelos Kolaitis <[email protected]>
I have addressed (hopefully) all the comments in the code review. I have performed a refactor on the Helm package (now Component Manager) to make it more generic. Also, I removed the flags for setting values and additional configuration for the components. |
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.
Added some comments on how the component manager interface is used. Sorry that I haven't seen this in the first review iteration.
I have switched to using a configuration file for defining the components, using Viper. Additionally, I have refactored the component package to make it more generic and decoupled from its implementation. |
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.
Cool stuff, @mateoflorido!
I really like the interface now.
One final question: The Refresh
method is not yet used, is it?
Summary
This pull request adds a library for managing Helm charts and includes a proof of concept for CLI entries (
enable
,disable
) for components.Changes
dump
plugin to bundle them within the snap.FAQ