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 a patch files accept multiple patches #5194

Merged
merged 11 commits into from Sep 15, 2023
Merged

fix a patch files accept multiple patches #5194

merged 11 commits into from Sep 15, 2023

Conversation

koba1t
Copy link
Member

@koba1t koba1t commented Jun 7, 2023

fix: #5049

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@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 Jun 7, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2023
@koba1t koba1t force-pushed the fix/patch_files_accept_multple_patches branch 4 times, most recently from 9962a9e to 132b558 Compare June 14, 2023 18:56
@koba1t koba1t marked this pull request as ready for review June 14, 2023 19:27
@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 Jun 14, 2023
@koba1t koba1t force-pushed the fix/patch_files_accept_multple_patches branch from 132b558 to a761588 Compare June 14, 2023 19:57
@koba1t
Copy link
Member Author

koba1t commented Jun 14, 2023

I test this function in this scenario.
If you think we need more tests, please tell me your ideas.

@natasha41575
Copy link
Contributor

/assign

@annasong20
Copy link
Contributor

/assign

annasong20
annasong20 suggested changes Jul 1, 2023
View reviewed changes
Copy link
Contributor

@annasong20 annasong20 left a comment

Choose a reason for hiding this comment

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

I think we're almost there! Feedback mostly to address #5049 (comment)

plugin/builtin/patchtransformer/PatchTransformer.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer.go Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer.go Outdated Show resolved Hide resolved
@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 koba1t force-pushed the fix/patch_files_accept_multple_patches branch from 2768237 to 0bb40bc Compare July 3, 2023 18:48
@koba1t koba1t force-pushed the fix/patch_files_accept_multple_patches branch from 0bb40bc to 692477a Compare July 3, 2023 19:07
annasong20
annasong20 reviewed Jul 8, 2023
View reviewed changes
Copy link
Contributor

@annasong20 annasong20 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and additional test, @koba1t! Apart from the document discussion, I think we're almost ready!

plugin/builtin/patchtransformer/PatchTransformer.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer_test.go Outdated Show resolved Hide resolved
api/krusty/multiplepatch_test.go Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer.go Outdated Show resolved Hide resolved
@annasong20
Copy link
Contributor

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 8, 2023
@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 Aug 23, 2023
@koba1t koba1t force-pushed the fix/patch_files_accept_multple_patches branch 2 times, most recently from ed962c0 to 7595196 Compare August 23, 2023 21:06
@koba1t koba1t force-pushed the fix/patch_files_accept_multple_patches branch from 7595196 to 6aac386 Compare August 23, 2023 21:21
@koba1t
Copy link
Member Author

koba1t commented Aug 23, 2023

@annasong20
Thanks for your reviews!
I completely fix every problem!

Could you recheck this PR?

@koba1t koba1t mentioned this pull request Sep 1, 2023
Closed
2 tasks
annasong20
annasong20 approved these changes Sep 9, 2023
View reviewed changes
Copy link
Contributor

@annasong20 annasong20 left a comment

Choose a reason for hiding this comment

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

This PR looks great! Thanks for iterating on this, @koba1t!!

Other than the error message that includes "target", the rest of the comments are minor. Feel free to lgtm yourself after removing "target" or I'd be happy to take another look. Also sorry for the delay in PR review.

/approve

plugin/builtin/patchtransformer/PatchTransformer.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer_test.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer_test.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer_test.go Outdated Show resolved Hide resolved
plugin/builtin/patchtransformer/PatchTransformer_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20, koba1t

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 9, 2023
@koba1t
Copy link
Member Author

koba1t commented Sep 11, 2023

Hi @annasong20
Thanks for your review!!
I'm done fixing everything from your comment.

I'll merge this PR without your additional check.
If you find any problem, Please feel free to revert or send me a DM.

/lgtm

@k8s-ci-robot
Copy link
Contributor

@koba1t: you cannot LGTM your own PR.

In response to this:

Hi @annasong20
Thanks for your review!!
I'm done fixing everything from your comment.

I'll merge this PR without your additional check.
If you find any problem, Please feel free to revert or send me a DM.

/lgtm

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 Author

koba1t commented Sep 11, 2023 •
edited

Sorry, @annasong20
I can't LGTM for my PR.
I'm afraid to ask, but could you check and add an LGTM to this PR?

@scila1996
Copy link

I"m so expecting for this PR will be merged !!

@oaxiento
Copy link

It'd be great to see this get merged. We absolutely need that feature. Thanks.

@annasong20
Copy link
Contributor

Looks amazing!!! Thank you SO much for carrying this PR to fruition!
/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 15, 2023
@k8s-ci-robot k8s-ci-robot merged commit 59696d1 into kubernetes-sigs:master Sep 15, 2023
9 checks passed
@koba1t koba1t deleted the fix/patch_files_accept_multple_patches branch September 16, 2023 05:14
@macieklewkowicz macieklewkowicz mentioned this pull request Sep 28, 2023
Draft
@MaGaroo MaGaroo mentioned this pull request Oct 19, 2023
Merged
@stormqueen1990 stormqueen1990 mentioned this pull request Oct 21, 2023
Closed
@Stringls Stringls mentioned this pull request Nov 2, 2023
Merged
AndiDog added a commit to giantswarm/management-cluster-bases that referenced this pull request Nov 20, 2023
@AndiDog
Upgrade kustomize to fix support for multiple patches in one file
2fa7c56 
See kubernetes-sigs/kustomize#5194 which was released in https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize%2Fv5.2.1
@AndiDog AndiDog mentioned this pull request Nov 20, 2023
Closed
@jfavellar90 jfavellar90 mentioned this pull request Nov 24, 2023
Open
@juliusvonkohout juliusvonkohout mentioned this pull request Mar 13, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalgn pascalgn pascalgn left review comments

@annasong20 annasong20 annasong20 approved these changes

@natasha41575 natasha41575 Awaiting requested review from natasha41575

@yuwenma yuwenma Awaiting requested review from yuwenma

Assignees

@annasong20 annasong20

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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
Kustomize Core Usability Bugs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

patches should accept a patch file with multiple patches
7 participants
@koba1t @k8s-ci-robot @natasha41575 @annasong20 @scila1996 @oaxiento @pascalgn