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.

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

feat: add remove configmap command #4796

Conversation

yufeiminds
Copy link
Contributor

@yufeiminds yufeiminds commented Sep 15, 2022

If someone need update the file list of the configMapGenerator from directory, they can use add configmap and remove configmap together.

Like this:

# Initialize
kustomize edit add configmap routes-01 --from-file="folder1/conf.d/*"
kustomize edit add configmap routes-02 --from-file="folder2/conf.d/*"

# Refresh configmap
kustomize edit remove configmap routes-01,routes-02
kustomize edit add configmap routes-01 --from-file="folder1/conf.d/*"
kustomize edit add configmap routes-01 --from-file="folder1/conf.d/*"

#4493

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 15, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: yufeiminds / name: Yufei Li (0d7c56d)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 15, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @yufeiminds!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 15, 2022
@k8s-ci-robot
Copy link
Contributor

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

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 the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 15, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 15, 2022
@natasha41575 natasha41575 requested a review from koba1t October 14, 2022 17:25
@natasha41575
Copy link
Contributor

@koba1t could you please do a quick review of this PR?

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, @yufeiminds
Thanks for your contribution!!
I add a few comments for your code.
Could you check there?

@k8s-ci-robot
Copy link
Contributor

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

@yufeiminds
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 Oct 31, 2022
@yufeiminds yufeiminds requested review from koba1t and removed request for KnVerey and yuwenma October 31, 2022 11:15
@yufeiminds
Copy link
Contributor Author

yufeiminds commented Oct 31, 2022

Yet another question, should we create a new pull request to support removing secrets? Because they are brothers. 😄

@koba1t
Copy link
Member

koba1t commented Oct 31, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 31, 2022
@koba1t
Copy link
Member

koba1t commented Oct 31, 2022

/lgtm

hi @natasha41575
This PR looks good to me, Could you exec tests and lint on GitHub actions?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2022
@natasha41575
Copy link
Contributor

This PR looks good to me, Could you exec tests and lint on GitHub actions?

Thanks for the review, and sorry for the delay! Running the tests now.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2022
@yufeiminds
Copy link
Contributor Author

Hi, @koba1t

I have fixed the lint issues, there is an issue with testpackage linter. It will conflict with the remove_test package. So I create a nolint comment to ignore it. It may not affect the feature and code smell. If we need to fix it after that. I can create another pull request to fix all the commands of remove/remove_test package.

And @natasha41575, Could you exec tests and lint on GitHub actions again? Thank you~ 😄

@koba1t
Copy link
Member

koba1t commented Nov 25, 2022

It will conflict with the remove_test package.

Sorry, I didn't know that package already existed, and I think that it needs to clean up...
nolint:testpackage is used here and I think it permitted.

So, I think your fix is good. Thanks for your fix!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2022
@yufeiminds
Copy link
Contributor Author

@natasha41575 hi, any feedback about this PR?

@natasha41575 natasha41575 added this to the v5.0.0 milestone Jan 6, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2023
@yufeiminds
Copy link
Contributor Author

Hi, @natasha41575, @koba1t. I resolved the conversations. Please push it forward again. Thank you~

@natasha41575 natasha41575 removed this from the v5.0.0 milestone Jan 31, 2023
@yufeiminds
Copy link
Contributor Author

@koba1t Hi, can you review it and push it forward? Thank you. 🍻

@koba1t
Copy link
Member

koba1t commented Feb 14, 2023

/lgtm
Thank you!

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

@natasha41575 Hi, does this PR and #4872 could be merged?

I used my own modified version in our pipeline before it was merged. I am looking forward to it being integrated into the mainline as soon as possible. If any problems arise, please feel free to reach out to me. 😊

Copy link
Member

@stormqueen1990 stormqueen1990 left a comment

Choose a reason for hiding this comment

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

LGTM! Just leaving a minor suggestion for the error message.

/lgtm

m.ConfigMapGenerator = newConfigMaps
err = mf.Write(m)
if err != nil {
return fmt.Errorf("configmap cannot write back to file, got %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

Minor suggestion:

Suggested change
return fmt.Errorf("configmap cannot write back to file, got %w", err)
return fmt.Errorf("failed to write kustomization file: %w", err)

@k8s-ci-robot
Copy link
Contributor

@stormqueen1990: changing LGTM is restricted to collaborators

In response to this:

LGTM! Just leaving a minor suggestion for the error message.

/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.

@stormqueen1990
Copy link
Member

/lgtm

cmd := &cobra.Command{
Use: "configmap",
Short: "Removes specified configmap" +
konfig.DefaultKustomizationFileName(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should say Removes specified configmap from kustomization.yaml

It currently reads as Removes specified configmapkustomization.yaml

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 1, 2023
@natasha41575
Copy link
Contributor

@yufeiminds The tests aren't triggering, I think because you created the PR before the tests ran. Can you push a commit address #4796 (comment)? I think that will trigger the tests.

@k8s-ci-robot
Copy link
Contributor

@yufeiminds: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kustomize-presubmit-master 927804d link unknown /test kustomize-presubmit-master

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

"github.com/spf13/cobra"
"sigs.k8s.io/kustomize/api/konfig"
"sigs.k8s.io/kustomize/api/types"
"sigs.k8s.io/kustomize/kustomize/v4/commands/internal/kustfile"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to change to v5

@stormqueen1990
Copy link
Member

Closing this in favour of #5327

/close

@k8s-ci-robot
Copy link
Contributor

@stormqueen1990: Closed this PR.

In response to this:

Closing this in favour of #5327

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Development

Successfully merging this pull request may close these issues.

5 participants