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

Add localize command handle #4959

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Add localize command handle #4959

merged 3 commits into from
Feb 1, 2023

Conversation

annasong20
Copy link
Contributor

This PR implements the handle that exposes localize as a command to users.

In order to test the command handle code, this PR moves existing test helpers in krusty to api/testutils.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 5, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 5, 2023
@annasong20
Copy link
Contributor Author

/cc @yuwenma

@k8s-ci-robot k8s-ci-robot requested a review from yuwenma January 5, 2023 21:16
@annasong20
Copy link
Contributor Author

annasong20 commented Jan 6, 2023 •
edited
Loading

@natasha41575 Does the failed presubmit test: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kustomize/4959/kustomize-presubmit-master/1611155994727944192

mean that we need to release a new version of api before I can merge this PR?

natasha41575
natasha41575 reviewed Jan 10, 2023
View reviewed changes
Copy link
Contributor

@natasha41575 natasha41575 left a comment

Choose a reason for hiding this comment

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

/hold

We should make sure we have everything we want to add to alpha kustomize localize before we expose it in the CLI.

api/testutils/loctest/helper.go Outdated Show resolved Hide resolved
kustomize/commands/localize/localize.go Outdated Show resolved Hide resolved
kustomize/commands/localize/localize.go Outdated Show resolved Hide resolved
kustomize/commands/localize/localize.go Outdated Show resolved Hide resolved
kustomize/commands/localize/flagscope.go Outdated Show resolved Hide resolved
kustomize/commands/localize/localize.go Outdated Show resolved Hide resolved
kustomize/commands/localize/localize.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2023
@yuwenma
Copy link
Contributor

yuwenma commented Jan 11, 2023

Defer to @natasha41575 comments about some user messages and stylish suggestions, otherwise looks good on my side. 👍

@yuwenma
Copy link
Contributor

yuwenma commented Jan 11, 2023 •
edited
Loading

@annasong20 I think you may have to split this PR into two parts, having the ./api PR merges first and then ./kustomize PR. Because these two libraries are released separately, if regression happens, one will affect the other.

@annasong20 annasong20 mentioned this pull request Jan 12, 2023
Merged
@annasong20
Copy link
Contributor Author

@annasong20 I think you may have to split this PR into two parts, having the ./api PR merges first and then ./kustomize PR. Because these two libraries are released separately, if regression happens, one will affect the other.

@yuwenma excellent suggestion! #4974 now handles the api changes: moving the test helpers from api/krusty/localizer to api/testutils/localizertest.

@annasong20 annasong20 mentioned this pull request Jan 12, 2023
Closed
@k8s-ci-robot
Copy link
Contributor

@annasong20: 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.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 12, 2023
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 12, 2023
@annasong20
Copy link
Contributor Author

/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 Jan 12, 2023
@annasong20
Copy link
Contributor Author

@natasha41575, @yuwenma, @KnVerey I cherry-picked the relevant commits from #4974, #4975 to demonstrate what the final code would look like.

However, we should look at those 2 PRs first. After/If those PRs merge, I will rebase this one off master.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2023
@k8s-ci-robot k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 18, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 18, 2023
@natasha41575 natasha41575 self-assigned this Jan 30, 2023
natasha41575
natasha41575 approved these changes Jan 30, 2023
View reviewed changes
Copy link
Contributor

@natasha41575 natasha41575 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I built off this branch and played around with kustomize localize and it's looking good. I think one thing could be improved about the UX is I would like to see some sort of success message after running kustomize localize. In particular, if I don't provide the destination argument, it would serve as an easy place for the user to see the name of the localized directory. Even if I do provide the destination argument, it's good feedback for the user to see that everything went as expected.

e.g. it would be nice to see

$ kustomize localize remote
Successfully localized `remote` into directory `localized-remote`.

Other than this small comment, this PR LGTM. I know we are waiting on #4975, so I can remove the hold on this when that goes in.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2023
@annasong20 annasong20 mentioned this pull request Jan 30, 2023
Merged
@annasong20
Copy link
Contributor Author

I built off this branch and played around with kustomize localize and it's looking good. I think one thing could be improved about the UX is I would like to see some sort of success message after running kustomize localize. In particular, if I don't provide the destination argument, it would serve as an easy place for the user to see the name of the localized directory. Even if I do provide the destination argument, it's good feedback for the user to see that everything went as expected.

e.g. it would be nice to see

$ kustomize localize remote
Successfully localized `remote` into directory `localized-remote`.

Other than this small comment, this PR LGTM. I know we are waiting on #4975, so I can remove the hold on this when that goes in.

Thanks, @natasha41575! I exposed the destination directory path in #5010. Once that merges, I'll rebase and use the returned destination in the Success message.

@annasong20
Copy link
Contributor Author

@natasha41575 I added the success message and rebased to include #5011, the merged solution to #4975.

Lmk if there's anything else blocking this PR.

@natasha41575
Copy link
Contributor

/lgtm
/approve

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 1, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20, 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 merged commit 2361660 into kubernetes-sigs:master Feb 1, 2023
@annasong20 annasong20 deleted the localize-command branch February 1, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natasha41575 natasha41575 natasha41575 approved these changes

@KnVerey KnVerey Awaiting requested review from KnVerey

@yuwenma yuwenma Awaiting requested review from yuwenma

Assignees

@natasha41575 natasha41575

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/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@annasong20 @yuwenma @k8s-ci-robot @natasha41575