-
Notifications
You must be signed in to change notification settings - Fork 174
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
Support targeted Patches in the PostRenderer specification #432
Support targeted Patches in the PostRenderer specification #432
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.
Thanks for your contribution! 🙇 🥇
Given the kustomize-controller has a likewise feature, I think it would be better to align the APIs to work the same. This means that we would need the following:
- The function from here: https://github.com/fluxcd/kustomize-controller/blob/main/controllers/kustomization_generator.go#L220
- A new
Patches
API field, as present in: https://github.com/fluxcd/kustomize-controller/blob/main/api/v1beta2/kustomization_types.go#L89. We import the same package (github.com/fluxcd/pkg/apis/kustomize
), so this should be available. - Tiny bit of code to take the field into account (but modified to work with the
HelmRelease
):for _, m := range kg.kustomization.Spec.Patches { kus.Patches = append(kus.Patches, kustypes.Patch{ Patch: m.Patch, Target: adaptSelector(&m.Target), }) }
c93e10d
to
48735f3
Compare
Thanks for the review @hiddeco! I applied your suggestions and adjusted the tests. Should I rebase/squash the commits to have a clean commit history for this feature? |
Yes, please rebase and squash 😄 |
8ea8bb9
to
e34c193
Compare
Signed-off-by: Tomek Rękawek <rekawek@adobe.com>
e34c193
to
5b1b1ce
Compare
@hiddeco - done. |
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 looks good to me, thanks for following through on the suggested changes @trekawek ☕
That was quick. Thanks for the feedback and merging it so fast, much appreciated! |
The release will take a bit longer I am afraid, due to some extensive changes to the source-controller we are currently working on to finalize (which need to be taken into account on this side). If your patch is business critical, I can provide you with a RC build which needs to be combined with the updated CRD. |
There's no need to create a RC build, but thanks for the offer @hiddeco! |
This PR adds support for the
target
selector for strategic merge Kustomize patches, similarly as it's supported for the JSON6902 patches.If the strategic merge patch have following form:
it'll be translated into appropriate targeted Patch in the effective Kustomize configuration.
Of course, the non-targeted form of the strategic merge patch is still supported:
The rationale behind it is a case in which certain Deployments or other K8s resources can be enabled or disabled with a Helm property. Now if we want to patch such a Deployment with the strategic merge patch (e.g. in order to add an env variable), the reconciliation will fail if the Deployment doesn't exist - this means we can't have a universal postRenderer configuration which will work regardless of the Helm values configuration.
The JSON6902 patches, with the explicit "target" section will work, as they'll simply ignore the missing targets. However, manipulating env variables with the JSON6902 patches is much harder than with the strategic merge patches and sometimes may not be possible. This is also true for other properties nested inside arrays, indexed by their "name" property (containers, envs, volumes, etc.)