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

Reduce error output on "kubectl delete" by adding "--ignore-not-found" #7801

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guessi
Copy link
Contributor

@guessi guessi commented Feb 4, 2025

What type of PR is this?

/kind cleanup
/kind feature

What this PR does / why we need it:

While doing ./hack/vpa-down.sh cleanup before deploy/re-deploy, there would have some error output as there's no resource to delete, by adding --ignore-not-found could greatly reduce those unnecessary output.

Which issue(s) this PR fixes:

Fixes n/a

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md

Reduce error output on "kubectl delete" by adding "--ignore-not-found"

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 4, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: guessi
Once this PR has been reviewed and has the lgtm label, please assign voelzmo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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
Copy link
Contributor

Hi @guessi. Thanks for your PR.

I'm waiting for a kubernetes 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 4, 2025
if [ "${ACTION}" == "delete" ]; then
EXTRA_FLAGS+=" --ignore-not-found"
fi
${SCRIPT_ROOT}/hack/vpa-process-yaml.sh $(script_path $i) | kubectl ${ACTION} ${EXTRA_FLAGS} -f - || true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After changed, it would still have some errors, but it's much lesser

% ./hack/vpa-down.sh
Deleting VPA Admission Controller certs.
Unregistering VPA admission controller webhook
Warning: deleting cluster-scoped resources, not scoped to the provided namespace
resource mapping not found for name: "verticalpodautoscalers.autoscaling.k8s.io" namespace: "" from "STDIN": no matches for kind "CustomResourceDefinition" in version "apiextensions.k8s.io/v1beta1"
ensure CRDs are installed first
resource mapping not found for name: "verticalpodautoscalercheckpoints.autoscaling.k8s.io" namespace: "" from "STDIN": no matches for kind "CustomResourceDefinition" in version "apiextensions.k8s.io/v1beta1"
ensure CRDs are installed first

the error here is simply because it try to do kubectl delete --ignore-not-found ./deploy/vpa-beta2-crd.yaml but it contains apiVersion: apiextensions.k8s.io/v1beta1 which might not available to the cluster (depends on what k8s version it is running with).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... no matches for kind "CustomResourceDefinition" in version "apiextensions.k8s.io/v1beta1" ... could be easily cleaned by adding 2>/dev/null but I'm not sure if it is what we expected?

@adrianmoisey
Copy link
Member

/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 Feb 4, 2025
@adrianmoisey
Copy link
Member

/remove-kind feature

@k8s-ci-robot k8s-ci-robot removed the kind/feature Categorizes issue or PR as related to a new feature. label Feb 4, 2025
Copy link
Member

@omerap12 omerap12 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 your PR. a tiny nit from my side.

@@ -74,6 +74,10 @@ for i in $COMPONENTS; do
if [[ ${ACTION} == print ]]; then
${SCRIPT_ROOT}/hack/vpa-process-yaml.sh $(script_path $i)
else
${SCRIPT_ROOT}/hack/vpa-process-yaml.sh $(script_path $i) | kubectl ${ACTION} -f - || true
EXTRA_FLAGS=""
if [ "${ACTION}" == "delete" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we have the same pattern as we have above?

if [ ${ACTION} == delete ] ; then

Copy link
Contributor Author

@guessi guessi Feb 5, 2025

Choose a reason for hiding this comment

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

It's trying to avoid shellcheck error, but yes, I can make the change.

Copy link
Contributor Author

@guessi guessi Feb 5, 2025

Choose a reason for hiding this comment

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

Fixed, coding style should now aligned.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I know it's not ideal to omit the "", but I want to keep the coding style consistent. If you're up for it, feel free to adjust the above to follow a more optimal approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm totally fine for both, let's keep this PR simple - do one thing at a time :-)

@guessi guessi force-pushed the reduce-error-output branch from aad5157 to ae9d130 Compare February 5, 2025 08:34
@guessi guessi force-pushed the reduce-error-output branch from ae9d130 to e2bba56 Compare February 5, 2025 08:34
@omerap12
Copy link
Member

omerap12 commented Feb 5, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants