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

feat: edit set secret #5467

Merged
merged 7 commits into from
Feb 8, 2024
Merged

feat: edit set secret #5467

merged 7 commits into from
Feb 8, 2024

Conversation

stormqueen1990
Copy link
Member

@stormqueen1990 stormqueen1990 commented Nov 29, 2023 •
edited
Loading

  • Add new functionality to allow editing a secret in a kustomization file. Initial implementation supports only --from-literal option.
  • Refactor edit set configmap to reuse some testing bits.

Fixes #4493

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 29, 2023
@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.

@stormqueen1990 stormqueen1990 marked this pull request as draft November 29, 2023 04:36
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023
@stormqueen1990 stormqueen1990 marked this pull request as ready for review December 18, 2023 02: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 Dec 18, 2023
charles-chenzz
charles-chenzz reviewed Jan 16, 2024
View reviewed changes
Copy link
Member

@charles-chenzz charles-chenzz left a comment

Choose a reason for hiding this comment

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

only some minor non-blocking comments, btw the test util was clean!

kustomize/commands/edit/set/all.go Outdated Show resolved Hide resolved
kustomize/commands/edit/set/setsecret.go Outdated
Use: "secret NAME [--from-literal=key1=value1] [--namespace=namespace-name] [--new-namespace=new-namespace-name]",
Short: fmt.Sprintf("Edits the value for an existing key for a secret in the %s file", konfig.DefaultKustomizationFileName()),
Long: fmt.Sprintf(`Edits the value for an existing key in an existing secret in the %s file.
Both secret name and key name must exist for this command to succeed.`, konfig.DefaultKustomizationFileName()),
Copy link
Member

Choose a reason for hiding this comment

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

the length of short and long seems like same, can we remove one of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Long continues on the next line! Do you feel we could add more to it?
I'd prefer to have a more complete description on the long one than scrap it completely.

Copy link
Member

Choose a reason for hiding this comment

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

would prefer more complete in the view of a user

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have an example of the format you'd like to see?

ncapps
ncapps reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@ncapps ncapps left a comment

Choose a reason for hiding this comment

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

Nice consolidation with the test util.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2024
@koba1t
Copy link
Member

koba1t commented Jan 17, 2024

It looks like the test failed in CI. Could you investigate and fix it?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2024
@stormqueen1990
Copy link
Member Author

It looks like the test failed in CI. Could you investigate and fix it?

Hi there @koba1t! It looks like that was a flake and it went away after I rebased. This is ready for a new look 🙂

@stormqueen1990 stormqueen1990 requested a review from ncapps January 19, 2024 18:05
koba1t
koba1t requested changes Jan 28, 2024
View reviewed changes
Copy link
Member

@koba1t koba1t left a comment

Choose a reason for hiding this comment

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

Hi, @stormqueen1990!
Thanks for your work!

I'm sorry for the delay in my review.
I added a few comments. Could you check that?

kustomize/commands/internal/configmapsecret/configmapsecret_testutils.go Outdated Show resolved Hide resolved
kustomize/commands/edit/set/setsecret_test.go Show resolved Hide resolved
@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 28, 2024
@stormqueen1990 stormqueen1990 requested a review from koba1t January 30, 2024 04:02
@stormqueen1990
feat: edit set secret
27a8ff9 
* Add new functionality to allow editing a secret in a kustomization file. Initial
  implementation supports only --from-literal option.
* Refactor edit set configmap to reuse some testing bits.
@stormqueen1990
chore: fix spacing in added description
23fbdd2
@stormqueen1990
chore: changes from code review
fd09a6e 
Remove error checks from the utility function and bubble them up instead.
@stormqueen1990
chore: update help to include bit about default namespace
ca8d629 
Add a blurb to the help output for both 'edit set configmap' and 'edit set secret'
to clarify that whenever a namespace is not specified, the default namespace is
implicitly defined as part of the commands.
@stormqueen1990
chore: add failure test case for empty generator
6c1fea7
@stormqueen1990
fix: remove excessive quoting from error messages
14c091a
@stormqueen1990
fix: update long description as per code review request
3bb9a6d
@koba1t
Copy link
Member

koba1t commented Feb 8, 2024

Thanks for your contribution!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 b154361 into kubernetes-sigs:master Feb 8, 2024
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 approved these changes

@annasong20 annasong20 Awaiting requested review from annasong20

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

@charles-chenzz charles-chenzz Awaiting requested review from charles-chenzz

@ncapps ncapps Awaiting requested review from ncapps

Assignees

@ncapps ncapps

@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. 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.
Projects
Kustomize Commands outside of `build`
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support edit set configmap
5 participants
@stormqueen1990 @k8s-ci-robot @koba1t @ncapps @charles-chenzz