Skip to content

use merge key list instead of a single merge key#3159

Merged
k8s-ci-robot merged 2 commits into
kubernetes-sigs:masterfrom
natasha41575:MergeKeyList
Nov 9, 2020
Merged

use merge key list instead of a single merge key#3159
k8s-ci-robot merged 2 commits into
kubernetes-sigs:masterfrom
natasha41575:MergeKeyList

Conversation

@natasha41575

@natasha41575 natasha41575 commented Oct 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

ref: #3111

Still a draft, but the test I added to patchstrategicmerge_test.go passes

Working on edge cases (will add tests) and cleaning up & removing some redundancy in the code

This will update kyaml to use x-kubernetes-list-map-keys from the OpenAPI schema as a merge key tuple.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 30, 2020
@k8s-ci-robot

k8s-ci-robot commented Oct 30, 2020

Copy link
Copy Markdown
Contributor

Hi @natasha41575. 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.

Details

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 Oct 30, 2020
@natasha41575

natasha41575 commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

/assign @monopole

@natasha41575
natasha41575 force-pushed the MergeKeyList branch 5 times, most recently from fab35f3 to bb5c79f Compare October 30, 2020 22:14
@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 Oct 30, 2020
yuwenma
yuwenma reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/yaml/fns.go Outdated
yuwenma
yuwenma reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/yaml/rnode.go
yuwenma
yuwenma reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/yaml/fns.go Outdated
yuwenma
yuwenma reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/yaml/fns.go
yuwenma
yuwenma reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/yaml/rnode.go Outdated
yuwenma
yuwenma reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/yaml/rnode.go Outdated
Shell32-Natsu
Shell32-Natsu reviewed Nov 2, 2020
View reviewed changes

@Shell32-Natsu Shell32-Natsu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a little bit more comments.

Comment thread kyaml/yaml/fns.go Outdated
Comment thread kyaml/yaml/fns.go
Comment thread kyaml/yaml/fns.go Outdated
Comment thread kyaml/yaml/fns.go Outdated
Comment thread kyaml/yaml/rnode.go Outdated
phanimarupaka
phanimarupaka reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/openapi/openapi.go
phanimarupaka
phanimarupaka reviewed Nov 2, 2020
View reviewed changes
Comment thread kyaml/yaml/walk/associative_sequence.go
@phanimarupaka

phanimarupaka commented Nov 2, 2020

Copy link
Copy Markdown
Contributor

@natasha41575 Please add tests to Merge3 logic as well. This change affects both kpt and kustomize and we need tests to cover kpt(merge3) cases.

@natasha41575
natasha41575 force-pushed the MergeKeyList branch 3 times, most recently from 3137492 to cdfa1f6 Compare November 2, 2020 23:56
monopole
monopole reviewed Nov 3, 2020
View reviewed changes
Comment thread api/filters/patchstrategicmerge/patchstrategicmerge_test.go
@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 Nov 6, 2020
@natasha41575

natasha41575 commented Nov 6, 2020

Copy link
Copy Markdown
Contributor Author

/cc @monopole

@k8s-ci-robot
k8s-ci-robot requested a review from monopole November 6, 2020 23:45
This was referenced Nov 7, 2020
Closed
Closed
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2020
@monopole

monopole commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

Please rebase and squash down to one commit.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2020
monopole
monopole approved these changes Nov 9, 2020
View reviewed changes

@monopole monopole left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, 2 commits better given two modules, thanks

@monopole

monopole commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

/lgtm

@k8s-ci-robot

k8s-ci-robot commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details 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 lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 9, 2020
@k8s-ci-robot
k8s-ci-robot merged commit d66fc46 into kubernetes-sigs:master Nov 9, 2020
@natasha41575 natasha41575 mentioned this pull request Nov 9, 2020
Closed
@natasha41575
natasha41575 deleted the MergeKeyList branch November 9, 2020 20:24
@natasha41575 natasha41575 mentioned this pull request Nov 10, 2020
Merged
@ephesused ephesused mentioned this pull request Nov 10, 2020
Closed
@natasha41575 natasha41575 mentioned this pull request Jan 28, 2021
Closed
This was referenced Jul 8, 2021
Closed
Closed
@tsekine354 tsekine354 mentioned this pull request Apr 1, 2025
Open
2 tasks
@tsekine354 tsekine354 mentioned this pull request May 14, 2025
Merged
This was referenced Jul 19, 2026
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

5 more reviewers
@apelisse apelisse apelisse left review comments
@Shell32-Natsu Shell32-Natsu Shell32-Natsu left review comments
@yuwenma yuwenma yuwenma left review comments
@phanimarupaka phanimarupaka phanimarupaka left review comments
@monopole monopole monopole approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@monopole monopole

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@natasha41575 @k8s-ci-robot @phanimarupaka @monopole @apelisse @Shell32-Natsu @yuwenma