-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add helmify
subcommand
#67
Conversation
Using the helmify subcommand, users can generate a Helm chart for their Spin Apps. Signed-off-by: Thorsten Hans <[email protected]>
Because I extracted flag validation from the scaffold function, the corresponding unit test must call validateScaffoldFlags instead of scaffold Signed-off-by: Thorsten Hans <[email protected]>
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.
Thank you for the contribution. Would you mind taking some time to write down some documentation in the README?
A PR like this was a concern of mine when the scaffold
command was introduced to the plugin. Every company has their own tools and their own mechanisms for deploying assets to Kubernetes. By adding scaffold
, I was concerned that we've now opened the floodgates and we will now need to maintain commands for other third-party projects like Helm, Kustomize, cdk8s, <insert obscure template language here>... How do you envision us handling that maintenance burden over time? Do you think it'd make sense to split out helmify
as a separate CLI plugin?
I've marked this as "request changes" because there are a few things that should be addressed before merging this PR, but I think it'd be worth addressing the larger question of whether it makes sense to maintain this as a separate plugin before proceeding.
// overwriting the default value of a flag defined on the parrent comand does not work | ||
// that's why I went for a PreRunE | ||
helmifyCmd.Flags().StringVarP(&scaffoldOpts.output, "out", "o", defaultChartOutput, "Path for writing the Helm chart to") | ||
scaffoldCmd.AddCommand(helmifyCmd) |
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 think it'd be better if we make this spin kube helmify
rather than spin kube scaffold helmify
. While both scaffold
and helmify
perform the same rough amount of work (prepare a Spin application to be deployed to Kubernetes), the use cases for both are quite distinct (prepare an app for package management and distribution vs scaffold out a manifest), and I believe they should be treated as such.
By tying this command as a subcommand of scaffold
, every feature flag that gets introduced to scaffold
must now be added to helmify
. There may be cases in the future where there will be a feature flag that will only work with scaffold
but not helmify
, and it'd be confusing for users to see that feature flag available in helmify
's help output because of the way this command has been architected.
k8s.io/api v0.29.2 | ||
k8s.io/apimachinery v0.29.2 | ||
helm.sh/helm/v3 v3.14.3 | ||
k8s.io/api v0.29.3 |
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.
Why is this included if it isn't a direct dependency of the plugin?
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.
If you're talking about the helm.sh/helm/v3
dependency, it's added because I decided to use chart
and chartutil
from helm itself instead of rolling everything manually
k8s.io/apimachinery v0.29.2 | ||
helm.sh/helm/v3 v3.14.3 | ||
k8s.io/api v0.29.3 | ||
k8s.io/apimachinery v0.29.3 |
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.
Why is this included if it isn't a direct dependency of the plugin?
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.
what do you mean by "this" k8s.io/apimachinery
? As part of the PR I added helm
as a dependency maybe I've to go mod tidy
it again?
Alternatively, what makes this command different from a starter chart written specifically for Spin apps? |
there is also https://github.com/arttor/helmify (which we use in spin-operator I believe). |
@bacongobbler I was also uncertain if this should be added to IMO we could address the majority of scenarios with being able to scaffold plain k8s manifests and helm charts. I went for a subcommand of I'm happy to move the command around. |
I don't see a helm starter chart as an equivalent solution for this.
As addition to the 2nd point, |
While I appreciate your perspective on this matter, I've found @rajatjindal's solution quite compelling. Through
It accepts all of
This example not only demonstrates the practical utility of I understand the inclination to develop a bespoke solution. However, I would prefer that we harness tools that already exist from the community. It not only fosters collaboration, but also builds upon a foundation of tooling that many developers are already familiar with. Unless there's an overwhelmingly compelling reason to develop something independently, we should strive to engage and collaborate with the community. Additionally, this project's core objective is to provide a great developer tool to interact with Spin Apps running on Kubernetes. That objective involves the relationship between the Kubernetes API and the client ( What are your thoughts on documenting the integration of |
averageUtilization: {{ .Values.targetCpuUtilizationPercentage }} | ||
- type: Resource | ||
resource: | ||
name: memory |
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.
This line needs to be indented one more space. I just discovered this bug and filed an issue: #68
Closing; |
This PR adds the
helmify
subcommand. Which allows users to generate Helm charts for a Spin App.The command is added as subcommand to
scaffold
and re-uses the flags defined onscaffold
.Usage