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

Replace obsolete patches syntax #1117

Merged
merged 2 commits into from
Aug 26, 2022
Merged

Conversation

deepy
Copy link
Contributor

@deepy deepy commented Aug 25, 2022

bart0sh
bart0sh previously approved these changes Aug 25, 2022
Copy link
Member

@bart0sh bart0sh left a comment

Choose a reason for hiding this comment

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

/lgtm

@mythi
Copy link
Contributor

mythi commented Aug 25, 2022

@deepy thanks a lot! The documentation still talks about patches so I wonder how is this failing. With the latest kustomize?

@deepy
Copy link
Contributor Author

deepy commented Aug 25, 2022

patches themselves aren't obsolete, I assume it's just the way of adding patches that has changed (since the thing is still there https://github.com/kubernetes-sigs/kustomize/blob/v3.3.1/pkg/types/kustomization.go#L59-L62 and does hold both type of patches)

The syntax for adding them through patches is different though: https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/patches/#patch-using-path-strategic-merge

I'm unfortunately not familiar enough with kustomize to give a good answer other than: I think the syntax might have changed twice since then

tkatila
tkatila previously approved these changes Aug 25, 2022
@mythi
Copy link
Contributor

mythi commented Aug 25, 2022

patches themselves aren't obsolete, I assume it's just the way of adding patches that has changed

Hmm, looks like the future (v1) is patches: only:
kubernetes-sigs/kustomize#4706
kubernetes-sigs/kustomize#4733

Maybe we can keep patches if there are no issues?

@eero-t
Copy link
Contributor

eero-t commented Aug 25, 2022

If future is patches: only, will it inherit the $patch: delete feature from the to-be-removed patchesStrategicMerge feature?

@natasha41575
Copy link

natasha41575 commented Aug 25, 2022

Found this from a link to a kustomize PR.

from the to-be-removed patchesStrategicMerge feature?

The patchesStrategicMerge feature is not being removed. It is just being consolidated into the patches field. There will be a long deprecation cycle with documented migration steps and an automated command (kustomize edit fix) to do the migration for you.

@mythi
Copy link
Contributor

mythi commented Aug 25, 2022

Found this from a link to a kustomize PR.

@natasha41575 thanks a lot for chiming in!

from the to-be-removed patchesStrategicMerge feature?

The patchesStrategicMerge feature is not being removed. It is just being consolidated into the patches field. There will be a long deprecation cycle with documented migration steps and an automated command (kustomize edit fix) to do the migration for you.

This PR proposes a move from patches to patchesStrategicMerge and were just discussing that it may not be necessary given the future plans. Agree?

@deepy
Copy link
Contributor Author

deepy commented Aug 25, 2022

Some change is going to be needed, I initially created this PR because it wasn't working for me, but if I use - path: foo.yaml instead of - foo.yaml it works, so I'll update the PR to do that instead.

@deepy deepy dismissed stale reviews from tkatila and bart0sh via 79986a6 August 25, 2022 19:08
@@ -1,6 +1,6 @@
bases:
- ../../base/
patches:
patchesStrategicMerge:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this one in as the file is already using patchesJson6902 (which is also being moved into the patches field) to prevent confusion

@deepy
Copy link
Contributor Author

deepy commented Aug 25, 2022

I've updated the PR to now do essentially the opposite, this solution also works for me.

With no changes I get Error: json: cannot unmarshal string into Go struct field Kustomization.patches of type types.Patch though so I'd appreciate this being merged :-)

@deepy deepy changed the title Replace obsolete patches with patchesStrategicMerge Replace obsolete patches syntax Aug 25, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1117 (c4937ba) into main (d981282) will not change coverage.
The diff coverage is n/a.

❗ Current head c4937ba differs from pull request most recent head 79986a6. Consider uploading reports for the commit 79986a6 to get more accurate results

@@           Coverage Diff           @@
##             main    #1117   +/-   ##
=======================================
  Coverage   53.01%   53.01%           
=======================================
  Files          40       40           
  Lines        4350     4350           
=======================================
  Hits         2306     2306           
  Misses       1917     1917           
  Partials      127      127           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

@deepy thanks for the patience! I think this is all good now, it fixes your errors in a (hopefully) future-proof manner.

Copy link
Member

@bart0sh bart0sh left a comment

Choose a reason for hiding this comment

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

/lgtm

@bart0sh bart0sh merged commit 6347609 into intel:main Aug 26, 2022
@deepy deepy deleted the patches-strategic-merge branch August 26, 2022 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants