Skip to content
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

✨ (go/v3) - Update kustomize from v3.8.7 to v4.4.1 and use replacements instead of vars #2498

Closed

Conversation

afritzler
Copy link
Contributor

@afritzler afritzler commented Jan 28, 2022

Since the usage of vars in kustomize will be deprecated soon it would make sense to use replacements instead as advised in the warning:

WARNING: There are plans to deprecate vars. For existing users of vars, we recommend migration to replacements as early as possible. There is a guide for convering vars to replacements at the bottom of this page under “convert vars to replacements”. For new users, we recommend never using vars, and starting with replacements to avoid migration in the future.

Another issue we were facing was vars-name collisions when using multiple kubebuilder based controllers in one root kustomization as vars are being propagated to the top.

To test the correct behaviour you can run

diff <(kustomize build ../MY_OLD_DEPLOYMENT/config/default/) <(kustomize build config/default/)

to make sure that the expected result matches the one from the original var based one. (of course make sure that the webhook configuration is commented out)

@k8s-ci-robot
Copy link
Contributor

Welcome @afritzler!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 28, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @afritzler. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 28, 2022
@afritzler
Copy link
Contributor Author

afritzler commented Jan 30, 2022

I guess it also makes sense to remove the vars in annotations in the following files:

crd/patches/enablecainjection_patch.go
kdefault/enablecainection_patch.go

as those are not really needed anymore.

@afritzler
Copy link
Contributor Author

Also how about all the test data and the documentation (book)? Those CERTIFICATE_NAMESPACE, CERTIFICATE_NAME, SERVICE_NAMESPACE and SERVICE_NAME are used all over the place and are not really needed when using the replacements approach.

@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 30, 2022
@camilamacedo86

This comment has been minimized.

@camilamacedo86

This comment has been minimized.

@afritzler

This comment has been minimized.

@afritzler

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2022
@afritzler afritzler force-pushed the use_kustomize_replacements branch 5 times, most recently from 824443d to 1bb22ec Compare January 31, 2022 17:22
@camilamacedo86 camilamacedo86 changed the title ✨ Use replacements instead of vars in default kustomization scaffolding ✨ Update kustomize from v3.8.7 to v4.4.1 and use replacements instead of vars in default kustomization scaffolding Jan 31, 2022
@camilamacedo86
Copy link
Member

/test pull-kubebuilder-e2e-k8s-1-15-12

@camilamacedo86 camilamacedo86 changed the title ✨ Update kustomize from v3.8.7 to v4.4.1 and use replacements instead of vars in default kustomization scaffolding ✨ (go/v3) - Update kustomize from v3.8.7 to v4.4.1 and use replacements instead of vars in default kustomization scaffolding Feb 1, 2022
@camilamacedo86 camilamacedo86 changed the title ✨ (go/v3) - Update kustomize from v3.8.7 to v4.4.1 and use replacements instead of vars in default kustomization scaffolding ✨ (go/v3) - Update kustomize from v3.8.7 to v4.4.1 and use replacements instead of vars Feb 1, 2022
camilamacedo86
camilamacedo86 previously approved these changes Feb 1, 2022
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great contribution 🥇

The appdiff ci checker is failing in this case because of the kustomize update.
It is fine.

The changes here shows fine for me.
/approved

It would be nice to get a second reviewer as well

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2022
@coderanger
Copy link

For the record this will make the default skeleton incompatible with kubectl -k in Kubernetes 1.20 and 1.21. Because of how Kubectl versioning works this isn't a full blocker (even if it was, standalone Kustomize would still be an option), but it should be added to the release notes that this requires Kustomize >= 4.1.3 or Kubectl >= 1.22.0

@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 1, 2022

Hi @coderanger,

Thank you for your help:

For the record this will make the default skeleton incompatible with kubectl -k in Kubernetes 1.20 and 1.21.
#2498

Hi @afritzler,

Based on the above comment, we cannot move with this one. We need support for the latest 3 k8s versions at least.

/hold

Also, in this case, it shows that we would need to address them via go/v4 plugin instead.

I think we need:

I did the above actions see: #2503

In this way, are you ok with we closing this PR?

@camilamacedo86
Copy link
Member

/approved cancel

Since it was identified that would mean no longer support k8s < 1.22

@camilamacedo86
Copy link
Member

/approved cancel

@camilamacedo86
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2022
@camilamacedo86 camilamacedo86 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2022
@camilamacedo86 camilamacedo86 dismissed their stale review February 1, 2022 12:39

we checked that we cannot move forward with.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afritzler
To complete the pull request process, please assign estroz after the PR has been reviewed.
You can assign the PR to them by writing /assign @estroz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@afritzler
Copy link
Contributor Author

afritzler commented Feb 1, 2022

@camilamacedo86 as replacements have been introduced in Kustomize v4.1.3 and given your support strategy I guess it is valid to sit this one out. Thanks a lot for your supporting me on that one!

Maybe we can revisit this one once k8s v1.25 hits the road. :-)

@coderanger
Copy link

@afritzler If you want to release it before then, you can make your own Kubebuilder layout plugin that extends the core one maybe? :)

@afritzler
Copy link
Contributor Author

@coderanger I thought about it - but then you have the maintenance effort. We fixed it now via documentation™️ for us 😄

@camilamacedo86
Copy link
Member

HI @afritzler,

Thank you a lot for your contribution. 🥇
It will not be lost, asap we are able to do this update we can use the changes made in this PR.

I am closing this one for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants