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

kubectl run: deprecate unused / nonuseful flags #112261

Merged
merged 1 commit into from Nov 4, 2022
Merged

kubectl run: deprecate unused / nonuseful flags #112261

merged 1 commit into from Nov 4, 2022

Conversation

brianpursley
Copy link
Member

@brianpursley brianpursley commented Sep 6, 2022 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The --cascade flag, has no practical effect when used, so it is being deprecated.

The following flags, which are unused and ignored by kubectl run,
have been deprecated:

--filename
--force
--grace-period
--kustomize
--recursive
--timeout
--wait

This PR hides them and sets a deprecation message, so that they can be removed in a future release (1 year = kubectl 1.29 if this makes it into kubectl 1.26).

Which issue(s) this PR fixes:

Fixes #108630

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Deprecated the following kubectl run flags, which are ignored if set: --cascade, --filename, --force, --grace-period, --kustomize, --recursive, --timeout, --wait

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 6, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/cli Categorizes an issue or PR as relevant to SIG CLI. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 6, 2022
@brianpursley
Copy link
Member Author

/hold

@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 Sep 6, 2022
@brianpursley
Copy link
Member Author

/retest

@brianpursley
Copy link
Member Author

brianpursley commented Oct 17, 2022 •
edited

Updated the PR to specify in which kubectl version the flags will be removed, after a one year deprecation period.

Also hid the deprecated flags, so they don't show in help anymore. I think this is OK to do because they are no-ops, completely ignored if set.

If someone is using the flags, their command will not fail and they will get a deprecation message about the flag.

@brianpursley brianpursley changed the title kubectl/pkg/cmd/run: deprecate unused flags kubectl run: deprecate unused flags Oct 17, 2022
@brianpursley brianpursley force-pushed the k-108630-2 branch 2 times, most recently from 5ba23bf to 86cb35b Compare October 24, 2022 21:31
@KnVerey
Copy link
Contributor

KnVerey commented Nov 3, 2022

/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 3, 2022
KnVerey
KnVerey reviewed Nov 3, 2022
View reviewed changes
staging/src/k8s.io/kubectl/pkg/cmd/run/run.go

// Deprecate and hide unused flags.
// These flags are being added to the run command by DeleteFlags to support pod deletion after attach,
// but they are not used if set, so they effectively do nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the values are passed through during Complete, so I was wondering how it is that they don't actually work. Does the following match your understanding?

  • wait and grace-period are overridden at L235-235 to false and -1 respectively
  • timeout is always ignored when wait=false
  • filenames, kustomize and recursive would have to be passed to the Builder created at L425 via FilenameParam, but they aren't. So when DeleteOptions#DeleteResult runs, the resources iterated over are only the ones given by Builder#ResourceNames, i.e. the created objects (good!).
  • force under the hood actually turns into grace-period=0, so our overriding it to -1 as stated above nullfies this one too. The grace period would show up in the delete request body with -v=8 if this weren't true (it doesn't).

⚠️ I think cascade technically does work. If I do kubectl run -i -t mypod --image=busybox:latest -f base/resources.yaml --rm --pod-running-timeout=1s --cascade=orphan -v=8, then the object I get back includes "finalizers":["orphan"]. That said, this doesn't strike me as useful, since pods don't have dependents by default and this command in any case doesn't create any. But if we do remove it as well, we should use different reasoning. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

@KnVerey thanks for tracing through all that. I should have done that with this PR description!

I agree on all points.

Also, I'm not sure why I thought cascade was a no-op, but you're right, technically it is used. I'm not sure what effect it would have in the case of kubectl run (there would be no dependent resources?) but the reason for the deprecation is not the same as the other flags.

I'll break it out from the others, with a different comment, and different deprecation message... maybe just "This flag will be removed in version 1.29".

I'm also not opposed to leaving it, if there is a valid use case. I just can't think of one offhand or how we would explain to people when they should use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I can't really think of one either. Technically you could manually add ownerReferences to objects to cause it to have dependents, but I have no idea why you'd do that for an object created with run. Of course, removing the flag doesn't prevent them from doing that, only from changing the deletion policy used with the rm option--and rm can only be specified when run is used interactively (making it extra unlikely this is useful). If we want a reason, maybe "is not relevant for kubectl run"?

@brianpursley brianpursley changed the title kubectl run: deprecate unused flags kubectl run: deprecate unused / nonuseful flags Nov 3, 2022
KnVerey
KnVerey reviewed Nov 4, 2022
View reviewed changes
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

/hold
in case you want to take my nitpicks (up to you)
/lgtm
otherwise

staging/src/k8s.io/kubectl/pkg/cmd/run/run.go Outdated
cmd.Flags().MarkDeprecated("kustomize", "This flag is ignored and will be removed in version 1.29")
cmd.Flags().MarkDeprecated("recursive", "This flag is ignored and will be removed in version 1.29")
cmd.Flags().MarkDeprecated("timeout", "This flag is ignored and will be removed in version 1.29")
cmd.Flags().MarkDeprecated("wait", "This flag is ignored and will be removed in version 1.29")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I built this PR to see how the message actually looks, and apparently it is concatenated to the standard message with a comma:

Flag --filename has been deprecated, This flag is ignored and will be removed in version 1.29
Flag --kustomize has been deprecated, This flag is ignored and will be removed in version 1.29

Suggestion: Flag --kustomize has been deprecated, because it is not used by this command. It will be removed in version 1.29

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought that was a little weird too. I was following how most of the other deprecation messages were worded, but maybe it is a good time to break from that and try to make it better. I'll update the messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated:

Flag --filename has been deprecated, because it is not used by this command. It will be removed in version 1.29.
Flag --kustomize has been deprecated, because it is not used by this command. It will be removed in version 1.29.
Flag --cascade has been deprecated, because it is not relevant for this command. It will be removed in version 1.29.

Hopefully it will be clear these flags are only going to be removed from THIS command, and not every command. I think it should be clear enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022
@brianpursley
kubectl/pkg/cmd/run: deprecate flags
09804a1 
The --cascade flag, has no practical effect when used, so it is being deprecated.

The following flags, which are unused and ignored by kubectl run,
have been deprecated:

--filename
--force
--grace-period
--kustomize
--recursive
--timeout
--wait
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022
@KnVerey
Copy link
Contributor

KnVerey commented Nov 4, 2022

/lgtm
/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 Nov 4, 2022
@k8s-ci-robot k8s-ci-robot merged commit 3e8c848 into kubernetes:master Nov 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KnVerey KnVerey KnVerey left review comments

@dougsland dougsland Awaiting requested review from dougsland

Assignees

@KnVerey KnVerey

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. 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

kubectl run accidentally exposes --kustomize flag
3 participants
@brianpursley @k8s-ci-robot @KnVerey