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.

Sign up for GitHub

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

Jump to bottom

🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge / ✨ Upgrade kustomize version from v5.1.1 to v5.2.1 #3661

Merged
merged 1 commit into from
Oct 30, 2023
Merged

🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge / ✨ Upgrade kustomize version from v5.1.1 to v5.2.1 #3661

merged 1 commit into from
Oct 30, 2023

Conversation

MaGaroo
Copy link
Contributor

@MaGaroo MaGaroo commented Oct 18, 2023 •
edited
Loading

Hi!
It fixes #3539

P.S. I hope I've chosen the right title this time. 😅

@k8s-ci-robot
Copy link
Contributor

Hi @MaGaroo. 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 18, 2023
@MaGaroo MaGaroo changed the title 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge [WIP] 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge Oct 18, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2023
@MaGaroo MaGaroo force-pushed the remove-patchesstrategicmerge branch from 6b8c51a to d09ba11 Compare October 18, 2023 05:55
@MaGaroo MaGaroo changed the title [WIP] 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge Oct 18, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2023
camilamacedo86
camilamacedo86 reviewed Oct 18, 2023
View reviewed changes
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 work !

/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 Oct 18, 2023
@MaGaroo MaGaroo force-pushed the remove-patchesstrategicmerge branch from d09ba11 to 90b62c2 Compare October 18, 2023 08:49
@MaGaroo
Copy link
Contributor Author

MaGaroo commented Oct 19, 2023

@camilamacedo86
Sorry, I hadn't run the e2e tests locally.
After examination, I found out that the changes related to fixing kubernetes-sigs/kustomize#5049 have not been released by kustomize team yet. In fact, current version of kustomize (v5.1.1) is released in July, and the pr kubernetes-sigs/kustomize#5194 is merged in September.

So, as @lentzi90 had told in #3539, we can wait for them to release a new version, or alternatively, we can separate the patches in webhookcainjection_patch.yaml into 2 files. IMO the former approach is better (although makes this PR unmergeable for a period of time). What's your opinion?

@lentzi90
Copy link

@camilamacedo86 Sorry, I hadn't run the e2e tests locally. After examination, I found out that the changes related to fixing kubernetes-sigs/kustomize#5049 have not been released by kustomize team yet. In fact, current version of kustomize (v5.1.1) is released in July, and the pr kubernetes-sigs/kustomize#5194 is merged in September.

So, as @lentzi90 had told in #3539, we can wait for them to release a new version, or alternatively, we can separate the patches in webhookcainjection_patch.yaml into 2 files. IMO the former approach is better (although makes this PR unmergeable for a period of time). What's your opinion?

It looks like just a few hours after your comment, kustomize made a new release that includes the fix 🙂
Can we bump the version to make use of it?

@MaGaroo
Copy link
Contributor Author

MaGaroo commented Oct 20, 2023 •
edited
Loading

@camilamacedo86 Sorry, I hadn't run the e2e tests locally. After examination, I found out that the changes related to fixing kubernetes-sigs/kustomize#5049 have not been released by kustomize team yet. In fact, current version of kustomize (v5.1.1) is released in July, and the pr kubernetes-sigs/kustomize#5194 is merged in September.

So, as @lentzi90 had told in #3539, we can wait for them to release a new version, or alternatively, we can separate the patches in webhookcainjection_patch.yaml into 2 files. IMO the former approach is better (although makes this PR unmergeable for a period of time). What's your opinion?

It looks like just a few hours after your comment, kustomize made a new release that includes the fix 🙂
Can we bump the version to make use of it?

😂
Okay, I'll bump kustomize version in the current pr in a few minutes.

@MaGaroo
Remove use of deprecated patchesStrategicMerge
6c0d90f 
It also needed to bump version of kustomize to
v5.2.1 in order to support multiple patches in a
single file. (config/default/webhookcainjection_-
patch.yaml)
@MaGaroo MaGaroo force-pushed the remove-patchesstrategicmerge branch from 90b62c2 to 6c0d90f Compare October 20, 2023 10:41
lentzi90
lentzi90 reviewed Oct 20, 2023
View reviewed changes
Copy link

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you for working on this!
It looks good to me, but I have very limited experience with this code base. Hopefully someone more knowledgeable can review 🙂

@camilamacedo86 camilamacedo86 changed the title 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge Oct 27, 2023
@camilamacedo86 camilamacedo86 changed the title 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge 🐛 (kustomize/v2,go/v4): Remove use of deprecated patchesStrategicMerge / ✨ Upgrade kustomize version from v5.1.1 to v5.2.1 Oct 27, 2023
camilamacedo86
camilamacedo86 approved these changes Oct 27, 2023
View reviewed changes
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 work 🥇

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2023
camilamacedo86
camilamacedo86 approved these changes Oct 27, 2023
View reviewed changes
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.

This one we will need to merge manually because API diff complains when we bump the kustomize version

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, MaGaroo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

camilamacedo86
camilamacedo86 reviewed Oct 27, 2023
View reviewed changes
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.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2023
@MaGaroo
Copy link
Contributor Author

MaGaroo commented Oct 28, 2023

This one we will need to merge manually because API diff complains when we bump the kustomize version

Any action needed from me?

@camilamacedo86 camilamacedo86 merged commit 19c0caa into kubernetes-sigs:master Oct 30, 2023
20 of 22 checks passed
@Neo2308 Neo2308 mentioned this pull request May 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees

@camilamacedo86 camilamacedo86

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove use of deprecated patchesStrategicMerge
4 participants
@MaGaroo @k8s-ci-robot @lentzi90 @camilamacedo86