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

Declare kubectl wait flag in a way consistent with other deletion flags #64375

Merged
merged 1 commit into from
May 29, 2018

Conversation

nilebox
Copy link

@nilebox nilebox commented May 27, 2018

What this PR does / why we need it:
A follow up PR for #64034 and #63979 that makes declaring wait flag consistent with the other flags.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #64401

Special notes for your reviewer:

Release note:


@k8s-ci-robot
Copy link
Contributor

@nilebox: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 27, 2018
@k8s-ci-robot k8s-ci-robot requested review from mengqiy and shiywang May 27, 2018 13:52
@neolit123
Copy link
Member

/area kubectl
@kubernetes/sig-cli-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. area/kubectl labels May 27, 2018
// To preserve backwards compatibility, but prevent accidental data loss, we convert --grace-period=0
// into --grace-period=1 and wait until the object is successfully deleted. Users may provide --force
// to bypass this wait.
o.WaitForDeletion = true
Copy link
Member

@neolit123 neolit123 May 27, 2018

Choose a reason for hiding this comment

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

comment above needs adjustment.

if f.Output != nil {
cmd.Flags().StringVarP(f.Output, "output", "o", *f.Output, "Output mode. Use \"-o name\" for shorter output (resource/name).")
}

Copy link
Member

Choose a reason for hiding this comment

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

no need for this whitespace change.

Copy link
Author

Choose a reason for hiding this comment

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

sure, but I don't see any value in keeping it.

@@ -211,5 +219,6 @@ func NewDeleteFlags(usage string) *DeleteFlags {
// add non-defaults
Force: &force,
Timeout: &timeout,
Wait: &wait,
Copy link
Member

Choose a reason for hiding this comment

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

please, align it to the above param values.

@nilebox nilebox force-pushed the delete-wait-cleanup branch 2 times, most recently from 4a1d473 to d4fdae8 Compare May 27, 2018 23:24
@ash2k
Copy link
Member

ash2k commented May 28, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 28, 2018
@nilebox nilebox changed the title [WIP] Declare wait flag in a way consistent with other deletion flags Declare wait flag in a way consistent with other deletion flags May 28, 2018
@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 May 28, 2018
@nilebox
Copy link
Author

nilebox commented May 28, 2018

/sig CLI

@nilebox
Copy link
Author

nilebox commented May 28, 2018

/test pull-kubernetes-kubemark-e2e-gce

@nilebox nilebox changed the title Declare wait flag in a way consistent with other deletion flags Declare kubectl wait flag in a way consistent with other deletion flags May 28, 2018
@soltysh soltysh added this to the v1.11 milestone May 28, 2018
@soltysh soltysh added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 28, 2018
@soltysh
Copy link
Contributor

soltysh commented May 28, 2018

This lgtm, please squash your changes into a single commit and I'll merge it.

@nilebox nilebox force-pushed the delete-wait-cleanup branch from acc5dd4 to 6f1b178 Compare May 28, 2018 22:20
@dixudx
Copy link
Member

dixudx commented May 29, 2018

/lgtm

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

@nilebox: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 29, 2018
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, nilebox, soltysh

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 May 29, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@dixudx @nilebox @soltysh

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.11 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

@soltysh soltysh added release-note-none Denotes a PR that doesn't merit a release note. status/approved-for-milestone and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. milestone/incomplete-labels labels May 29, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 64300, 64375). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 07e6410 into kubernetes:master May 29, 2018
@nilebox nilebox deleted the delete-wait-cleanup branch May 29, 2018 11:50
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. area/kubectl 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. milestone/incomplete-labels release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. 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.

kubectl replace --grace-period=0 --force ends with nil pointer dereference
7 participants