-
Notifications
You must be signed in to change notification settings - Fork 902
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
feat(helm): Add Helm Chart pipeline trigger #8475
Conversation
- Pipeline trigger configuration - Execution status UI - Manual trigger UI
Let me first say that I'm so excited for this and the other related PRs! I'm curious what the implications are for #7312 though. |
@dbyron0 Interesting! I had not seen that particular feature - thanks for pointing it out. If I'm understanding that PR correctly, I would say these are certainly related, but still distinct use cases. The PR you mention is intended to provide an expected Helm artifact in a manual execution, regardless of how the Helm chart is normally delivered. This PR does have the effect of providing a Helm artifact, but only via a new Helm trigger, and only when a Helm trigger is configured. From the #7312 PR message, it appears under the following (pretty narrow) conditions:
A possible solution would be add another condition to activating the functionality from #7312 - only show that if there is also no Helm trigger configured. How does that sound to you? |
If I'm reading this PR correctly, it's possible to specify the helm chart for pipelines with helm triggers (and maybe even all of them when there's more than one)... and so then yes, it makes sense to disable the #7312 functionality. In the end, it seems like it'll still be possible to specify a helm chart for manual execution whether there's a trigger or not, which would be lovely. |
I'm not that familiar with deck code, perhaps @maggieneterval can you take a look? |
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.
@jcavanagh Looks great; thanks for the awesome feature! 💯 Added a couple of questions inline, but nothing blocking.
I'd be in favor of removing the Bake (Manifest) stage's current manual execution component to avoid confusion over two different Helm manual execution components, but defer to your judgment.
app/scripts/modules/core/src/pipeline/config/triggers/helm/HelmTriggerExecutionStatus.tsx
Show resolved
Hide resolved
app/scripts/modules/core/src/pipeline/config/triggers/helm/HelmTriggerTemplate.tsx
Show resolved
Hide resolved
@jcavanagh I'd also recommend making a PR against spinnaker.github.io to add a note to the curated changelog for next release so that users know that this feature has landed! 🥳 |
I can see there being confusion in the code, but not so much for users. If they have a trigger, they get one way of manual execution, and if they don't they get the other. It works for me to remove it, but can we wait to do it in a subsequent version of spinnaker? That way when I upgrade I don't have broken pipelines/user experience until I have a chance to change to this new way. |
The old manual Helm UI is further narrowed to not appear if a Helm trigger is configured.
@dbyron0 @maggieneterval I've added the aforementioned additional conditional for the old manual Helm UI here - they should hopefully not step on each other now, and the old use case can be preserved. |
Thanks, LGTM! 👍 |
Thank you! |
Relates to issue: spinnaker/spinnaker#4447
Related PRs:
spinnaker/echo#988
spinnaker/igor#829
spinnaker/clouddriver#4773