-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
✨ (kustomize/v2-alpha) fixes in the scaffold to follow the changes in the MAJOR release of kustomize #2750
✨ (kustomize/v2-alpha) fixes in the scaffold to follow the changes in the MAJOR release of kustomize #2750
Conversation
Skipping CI for Draft Pull Request. |
/test all |
/test all |
1613219
to
9e86e1e
Compare
/test all |
9e86e1e
to
d7ec8b6
Compare
a740c9b
to
3b0b86f
Compare
@@ -60,10 +60,10 @@ metadata: | |||
name: serving-cert # this name should match the one appeared in kustomizeconfig.yaml | |||
namespace: system | |||
spec: | |||
# $(SERVICE_NAME) and $(SERVICE_NAMESPACE) will be substituted by kustomize | |||
# SERVICE_NAME_PLACEHOLDER and SERVICE_NAMESPACE_PLACEHOLDER will be substituted by kustomize |
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.
IMO the suffix _PLACEHOLDER
is not needed. The all-caps style is easily recognized as var-syntax style IMO.
But I wonder if this change really work? Have you tested the resulting scaffolding @camilamacedo86? If you are trying to migrate from using vars to replacements, I think there is a bit more to be done, ref. https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/vars/#more-complex-migration-example
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.
Hi @erikgb this changes are for the new alpha plugin. (alpha and not in the default scaffild)
The changes from vars to replacements are done here: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config/kdefault/kustomization.go#L92-L190
In this follow-up pr, I am just trying to adjust the rest of the places.
Why did I change the var by the placeholders? The doc says:
Replace every instance of $(SOME_SECRET_NAME) with any arbitrary placeholder value.
Anyway, if we do not do this replacement it still working because as you can see in the kustomize config where the replacements are used we are working with positions.
The kustomize changes on the alpha one have been tested in the e2e here : https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v3/plugin_cluster_test.go#L132-L151
However, we will still be needing other follow-ups and address further changes to the new alpha plugin ONLY.
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.
Ok, I understand that the migration to replacements has started. Good! But I would just stick with the existing names SERVICE_NAME
\ SERVICE_NAMESPACE
and drop the _PLACEHOLDER
postfix. It still looks like a placeholder IMO.
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.
@erikgb : If we run kustomize edit fix --vars to help you move forward with the new changes for 4.x they suggest PLACEHOLDER word to make clear for users that it is just a PLACEHOLDER so because of that I kept as suggested but yep I think we can remove the word
3b0b86f
to
e004455
Compare
e004455
to
9aa57e0
Compare
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.
Looks good to me!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Moving forward since the @erikgb suggestions are addressed and we have also another LGTM. |
Description
bases:
withresources:
With: