-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix: patch additions honor source key style #5196
Merged
k8s-ci-robot
merged 4 commits into
kubernetes-sigs:master
from
ephesused:issue4928-append-honors-key-style
Sep 1, 2023
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
691b7d1
fix: patch additions honor source key style
ephesused c76fd5e
test: update psm key style test
ephesused 096b2c4
test: add psm test for different key types
ephesused 78b8139
Merge branch 'master' into issue4928-append-honors-key-style
ephesused File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could you consider adding a test case when the key-value has
"6443"
and6443
?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.
I'm happy to extend or add other tests, but I'm not quite following the scenario you'd like me to test. Are you asking that this test to change its current input "6443" entry to:
And leave the patch entry like so?
That would mean this test would be exercising different value types.
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.
I'm sorry for writing a misreadable comment.
I just want to add to tests that mixed the same name string key and int key.
Ex.
"6443": "foobar"
and a patch has6443: "barfoo_int"
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.
Done - note that this scenario mirrors one that I discussed with @KnVerey in my earlier attempt.
A key point to make is that I'm intending this fix to be targeted at the specific issue in #4928: the key style of a node that's newly created in a merge.
This PR is not intended to alter the existing key style behavior for patches (which, I admit, is somewhat confusing). In that context, the adjusted test case we are discussing falls outside of the scope of what this PR changes. The current behavior for kustomize when input and patch have a matching key is to retain the key style of the input. This PR does not change that behavior.
Edited to fix a bad link.
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.
I understood this sentence. I want to keep until behavior with test cases.
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.
So I want to say that we need to add test cases while keeping at the tests from 691b7d1.
Could you add test cases using the int key under your tests?
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.
Apologies - I believe I've got it right this time. Thanks for your patience.