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
Remove unused flags from kubectl run #110668
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 |
/retest |
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.
That looks good to me but I'd expect removal of this line https://github.com/kubernetes/kubernetes/blob/202d1e6fc5993b93ef6100f0e3f516a249386f4a/staging/src/k8s.io/kubectl/pkg/cmd/run/run.go#L167. This PR still exposes delete flags?
Yeah. I was approaching this PR with a mindset of trying to keep things as similar to how they are currently as possible. But after doing this, I wonder if it might be better to simply remove the DeleteFlags altogether. I will circle back and take a look at what they would look like. |
Here is the help output, with flags removed:
|
@ardaguclu I removed DeleteFlags altogether. I think it simplifies things even more. Thanks for the suggestion. PTAL. |
That looks good to me. I'll label it later for the other reviewers have a chance to look at. |
The following flags, which do not apply to kubectl run, have been removed: --cascade --filename --force --grace-period --kustomize --recursive --timeout --wait These flags were being added to the run command to support pod deletion after attach, but they are not used if set, so they effectively do nothing. This PR also displays an error message if the pod fails to be deleted (when the --rm flag is used). Previously any error during deletion would be suppressed and the pod would remain. This PR also adds some unit tests for run and attach with and without the --rm flag. As such, some minor refactoring of the run command has been done to support mocking dependencies.
/retest |
/triage accepted |
/lgtm |
Hey folks, we come across this during our k8s upgrade to v1.25 😊 I think having the information that the flags are unused and which have being removed would be great to capture in the release notes. |
Actually, I think it was a mistake to completely remove the flags at this time, and this PR needs to be reverted. While the flags are completely useless, they do exist, and if someone happened to be setting them, their Even though the change is a good one to make, it hasn't gone through the required deprecation period, and could break backward compatibility if someone happened to be using those flags. |
Wouldn't be better adding manual deprecation for flags in run command instead directly reverting this useful fix?. These commands are redundant and we just don't want to break any script. In any case, there will be a backport to 1.25. |
Ideally, but I think process-wise, it makes sense to revert this and then do the deprecation in a new PR. This will allow time for the re-do PR to be reviewed. |
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
The following flags, which do not apply to kubectl run, have been removed:
These flags were being added to the run command to support pod deletion after attach, but they are not used if set, so they effectively do nothing.
This PR also displays an error message if the pod fails to be deleted (when the --rm flag is used). Previously any error during deletion would be suppressed and the pod would remain.
This PR also adds some unit tests for run and attach with and without the --rm flag. As such, some minor refactoring of the run command has been done to support mocking dependencies.
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.: