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

fix: patch additions honor source key style #5196

Merged
merged 4 commits into from Sep 1, 2023
Merged

fix: patch additions honor source key style #5196

merged 4 commits into from Sep 1, 2023

Conversation

ephesused
Copy link
Contributor

When a patch appends a new node, it should honor the key style from the patch. Prior to this commit, no style was applied, leading to problems when the key value could be interpreted as a different type based on its content. For example, "9110" needs quoting to ensure it is seen as a string in yaml.

Fixes #4928

This PR replaces #5005, where I messed up my working branch. Given circumstances, I figured it was simpler to just cut a new PR.

@ephesused
fix: patch additions honor source key style
691b7d1 
When a patch appends a new node, it should honor the key style from the
patch. Prior to this commit, no style was applied, leading to problems
when the key value could be interpreted as a different type based on its
content. For example, "9110" needs quoting to ensure it is seen as a
string in yaml.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 8, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @ephesused. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2023
@ephesused ephesused mentioned this pull request Jun 8, 2023
Closed
@koba1t
Copy link
Member

koba1t commented Jun 13, 2023

/triage accepted
/kind bug
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jun 13, 2023
@koba1t
Copy link
Member

koba1t commented Jun 13, 2023

/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 Jun 13, 2023
koba1t
koba1t reviewed Jun 13, 2023
View reviewed changes
api/filters/patchstrategicmerge/patchstrategicmerge_test.go
name: blabla
namespace: blabla-ns
data:
"6443": "foobar"
Copy link
Member

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" and 6443?

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'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:

  "6443": 6443

And leave the patch entry like so?

  "6443": "barfoo"

That would mean this test would be exercising different value types.

Copy link
Member

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.

  • base configMap has "6443": "foobar" and a patch has 6443: "barfoo_int"

Copy link
Contributor Author

@ephesused ephesused Jun 13, 2023 •
edited

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.

Copy link
Member

Choose a reason for hiding this comment

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

In that context, the adjusted test case we are discussing falls outside of the scope of what this PR changes.

I understood this sentence. I want to keep until behavior with test cases.

Copy link
Member

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?

Copy link
Contributor Author

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.

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

@koba1t
Copy link
Member

koba1t commented Jun 20, 2023

Thanks @ephesused !
/lgtm

/cc @natasha41575 @annasong20

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2023
@Hammond95
Copy link

any updates on this?

@natasha41575 natasha41575 self-assigned this Aug 30, 2023
@natasha41575
Copy link
Contributor

Like the other PR, would you mind pushing a dummy commit to see if it retriggers the tests?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023
@ephesused
Copy link
Contributor Author

Like the other PR, would you mind pushing a dummy commit to see if it retriggers the tests?

I simply merged to make this PR current. I see the workflows waiting to run, pending approval from a maintainer. Thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ephesused, natasha41575

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2023
@natasha41575
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2023
@k8s-ci-robot k8s-ci-robot merged commit 169fdd7 into kubernetes-sigs:master Sep 1, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koba1t koba1t koba1t left review comments

@natasha41575 natasha41575 natasha41575 approved these changes

@annasong20 annasong20 Awaiting requested review from annasong20

Assignees

@natasha41575 natasha41575

@koba1t koba1t

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. kind/bug Categorizes issue or PR as related to a bug. 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Kustomize Core Usability Bugs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kustomize fails to apply overlay if key in base yaml is numeric and missing
5 participants
@ephesused @k8s-ci-robot @koba1t @Hammond95 @natasha41575