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
kustomize edit add replacement #5444
base: master
Are you sure you want to change the base?
kustomize edit add replacement #5444
Conversation
/assign |
This PR has multiple commits, and the default merge method is: merge. 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. |
c242a1e
to
af18c2f
Compare
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.
Left some suggestions!
af18c2f
to
4f85cdb
Compare
4f85cdb
to
b015c9b
Compare
b015c9b
to
00a38ec
Compare
9b7c3b0
to
1fb4bdf
Compare
fa748d8
to
fd4d316
Compare
fd4d316
to
9825a6f
Compare
9825a6f
to
45a834b
Compare
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.
Good work here! 🎉
This is mostly LGTM, I just see one lint failure. I think we could also revert the go.work.sum
change, since your code is not adding new modules. What do you think?
go.work.sum
Outdated
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.
It doesn't look like your code changes should have updated this (I don't see any new modules). Do you think this can be reverted for this PR?
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.
the update is resolved the need-rebase label
/label tide/merge-method-squash |
45a834b
to
09b4bc5
Compare
09b4bc5
to
d85bb6e
Compare
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.
/lgtm
920d2b4
to
db34506
Compare
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.
/lgtm
would you mind approve @stormqueen1990 ? seems like you're approver of command(except build) |
for _, r := range m.Replacements { | ||
if len(r.Path) > 0 && r.Path == o.Replacement.Path { | ||
return fmt.Errorf("replacement for path %q already in %s file", r.Path, konfig.DefaultKustomizationFileName()) | ||
} | ||
} | ||
m.Replacements = append(m.Replacements, o.Replacement) |
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.
Hi @charles-chenzz
Could you add a check to file exists for replacement file?
I think other edit add
command is implemented that.
/assign |
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.
Went over the implementation and looks good to me. Lgtm after Yugo's review is addressed.
/hold - Adding hold so that it doesn't auto merge till Yugo's lgtm is in place.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: charles-chenzz, varshaprasad96 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 |
fix #4679