-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add renderer pipeline support to manifest generation in #483
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?
feat: add renderer pipeline support to manifest generation in #483
Conversation
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.
Trivy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
You need to make sure the pr checks pass (its our way of ensuring the code has been tested e2e before merge) |
2fdeb2d to
8967428
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…-to-manifest-generation-in
…-to-manifest-generation-in
pkg/manifests/template/template.go
Outdated
| return renderDefault(dir, svc, mapper) | ||
| } | ||
|
|
||
| var allManifests []unstructured.Unstructured |
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 is generally fine, but I think we need to ensure two things:
- The default renderer should still run. This helps with the primary usecase here of supporting a helm chart and some additional git sourced manifests in one shot
allManifestsneeds to be deduped at the end (very possible you could have two renderers generate the same thing). My guess is there's dedupe elsewhere done the chain that can be imitated, otherwise it's just lo.UniqBy(...) with a key using gvk namespace + name of the unstructured resource
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.
Can you clarify your first point? My understanding was that the default renderer would run 1) if no renderers were defined or 2) if >1 renderer is defined and one of them is AUTO with AUTO meaning to use the default renderer.
Are you saying that the default renderer should run every time regardless of what renderers are defined?
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.
Also, in the case of a dup, which renderer version should be kept? ie. if both raw and helm are rendered and a dup is detected, do we want the raw or do we want the helm?
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.
-
No we should run it regardless, or at least if it's just a helm chart. The most common usecase here is i want to install a chart + a few crs, and that's the nicest solve to handle that.
-
For dupes, just keep the last one in the chain, we can write some conditions on it, but for now that's fine.
…-to-manifest-generation-in
…-to-manifest-generation-in
Test Plan
Test environment: https://console.plrl-dev-aws.onplural.sh/
Checklist