-
Notifications
You must be signed in to change notification settings - Fork 39k
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
kubectl run: deprecate unused / nonuseful flags #112261
Conversation
[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 |
/hold |
/retest |
99adee2
to
209ca27
Compare
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. |
5ba23bf
to
86cb35b
Compare
/triage accepted |
|
||
// 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. |
There was a problem hiding this comment.
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
andgrace-period
are overridden at L235-235 tofalse
and-1
respectivelytimeout
is always ignored whenwait=false
filenames
,kustomize
andrecursive
would have to be passed to the Builder created at L425 viaFilenameParam
, but they aren't. So whenDeleteOptions#DeleteResult
runs, the resources iterated over are only the ones given byBuilder#ResourceNames
, i.e. the created objects (good!).force
under the hood actually turns intograce-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).
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
86cb35b
to
52819eb
Compare
There was a problem hiding this 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
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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
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
52819eb
to
09804a1
Compare
/lgtm |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: