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

Proposal: Deprecate the usage of NoOptDefVal on flags #1442

Open
rikatz opened this issue Jun 15, 2023 · 13 comments
Open

Proposal: Deprecate the usage of NoOptDefVal on flags #1442

rikatz opened this issue Jun 15, 2023 · 13 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@rikatz
Copy link
Contributor

rikatz commented Jun 15, 2023

What would you like to be added:
Not added, but removed 😓

We use NoOptDefVal on flags like dry-run, validate and others (mapped below). This generates confusion on the command, as we accept string flags as key=value or key value, but when using NoOptDefVal this is not possible.

This separation of a value makes sense, for instance, let's take validate flag as an example:

kubectl create deploy --image=nginx --validate strict

In this case, do I want to create a deploy called strict and default value, or do I want to do strict validation but I forgot the deployment name? It is not possible to get it.

We already stated that this behavior will be deprecated on the dry-run flag, but I think we should move forward and do with all the other string flags.

If you pass a string flag, you should know what value you want (and not be susceptible to future behavior change if we want to change the "default hidden value".

This will also help to generate consistency in command usage, knowing that string flags can or can't be separated by "=" and that anything else is the real command arg.

Why is this needed:
Generate some consistency on command execution

I will try to remember the next sig-cli meeting to bring this subject, but also anyone looking at this feel free to comment here and tell me if it is worth to discussing this or if this behavior is this way and will not change, and close the issue :)

Commands and flags currently with this behavior

  • validate and dry-run flags on create * command
  • cascade flag on delete * command
  • set-raw-bytes flag on config set command
  • insecure-skip-tls-verify and embed-certs flag on config set-cluster command
  • embed-certs flag on config set-credentials command
@rikatz rikatz added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 15, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jun 15, 2023
@varshaprasad96
Copy link
Contributor

@rikatz I've been looking at this, and agree to the deprecation of NoOptDefVal for consistency. I can work on this if there is no strong opinion against doing so in the community meeting.

/assign @varshaprasad96

@mpuckett159
Copy link
Contributor

/triage accepted
I agree that this is an issue. This will be complicated and has more far reaching implications. Require deprecation process (and KEP). This affects more than just string flags also affects bool flags.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 22, 2023
@apelisse
Copy link
Member

I've clearly been abusing this, as someone who introduced it for both dry-run and validation. I'm not sure HOW I would have done without it.

@rikatz
Copy link
Contributor Author

rikatz commented Jun 29, 2023

@apelisse I don't think there was another way of doing it! But I think right now at least IMHO is a behavior that we should avoid, for command consistency, and make sure that if you want "--validation", you either call the flag a bool (validation=true,false) or a string (validation=type). Or, let's say, in the future if we say that dry-run is not a bool anymore, call the flag "dry-run-mode=client" (yikes, I know, it sucks, but I cannot also think on a different way)

@salavessa
Copy link

I just bumped into a highly undesired behavior performing a kubectl delete sts my-sts --cascade orphan (tested in kubectl versions v1.26.6 and v1.27.3).

By omitting the equals sign, --cascade is assuming the default option background instead of the orphan value and the command above actually calls a delete on both my-sts and orphan.

One thing that makes this even worse is that the bash auto-completion mechanism always sets --cascade<WHITE_SPACE> (exact same behavior for --dry-run).

Below is a snippet of commands with what's been described:

$ kubectl delete sts my-sts --cascade orphan --dry-run server
W0701 19:49:02.551736    8944 helpers.go:663] --dry-run is deprecated and can be replaced with --dry-run=client.
statefulset.apps "my-sts" deleted (dry run)
statefulset.apps "orphan" deleted (dry run)
statefulset.apps "server" deleted (dry run)

$ kubectl delete sts my-sts --cascade orphan --dry-run=server
statefulset.apps "my-sts" deleted (server dry run)
Error from server (NotFound): statefulsets.apps "orphan" not found

$ kubectl delete sts my-sts --cascade=orphan --dry-run=server
statefulset.apps "my-sts" deleted (server dry run)

Hope this will get sorted at some point.

Thanks!

@marckhouzam
Copy link
Member

One thing that makes this even worse is that the bash auto-completion mechanism always sets --cascade<WHITE_SPACE> (exact same behavior for --dry-run)

@salavessa I consider this a bug in cobra. would you be willing to open an issue? spf13/cobra

@marckhouzam
Copy link
Member

One thing that makes this even worse is that the bash auto-completion mechanism always sets --cascade<WHITE_SPACE> (exact same behavior for --dry-run)

@salavessa I consider this a bug in cobra. would you be willing to open an issue? spf13/cobra

So, after thinking about it some more, I don’t think cobra can do much better.

In your example @salavessa, if you trigger completion: kubectl delete sts my-sts --cascade<TAB>, cobra won’t be able to know if you want to use the default value for cascade, in which case you want to put the space or if you want to use your own value, in which case you want to use an =.

Maybe not adding the space in this case would be slightly more informative?

@mpuckett159
Copy link
Contributor

I think this is something that needs to be solved with user alerting, ie. the interactive delete flag that @ardaguclu is working on, coupled with the kuberc stuff I've been working in. That will force users to verify destructive actions and would catch something like this, assuming the user doesn't just pass a autoyes to everything.

@salavessa
Copy link

In your example @salavessa, if you trigger completion: kubectl delete sts my-sts --cascade<TAB>, cobra won’t be able to know if you want to use the default value for cascade, in which case you want to put the space or if you want to use your own value, in which case you want to use an =.

Maybe not adding the space in this case would be slightly more informative?

@marckhouzam

I'm not very concerned about the use of --cascade<SPACE> with auto-complete. What I believe is inconsistent is that --cascade orphan is not doing the same as --cascade=orphan.

For --cascade it feels redundant to have it with a default value because the delete will perform exactly the same way with or without it, i.e. kubectl delete sts my-sts and kubectl delete sts my-sts --cascade do exactly the same.

What I would expect is that --cascade to behave the same way as --namespace option in terms of reading a value with a space or with the equals sign, and it would only use the "default" value when --cascade it's not present:

$ kubectl get pod --namespace
error: flag needs an argument: --namespace
See 'kubectl get --help' for usage.

$ kubectl get pod --namespace dummy
No resources found in dummy namespace.

$ kubectl get pod --namespace=dummy
No resources found in dummy namespace.

For --dry-run it's a bit different because the default option when the option is present (--dry-run=client) is not the same of when the option is not present (--dry-run=none), i.e. currently kubectl delete sts my-sts behaves differently from kubectl delete sts my-sts --dry-run. But I still believe that --dry-run client should behave the same as --dry-run=client.

Thanks

varshaprasad96 added a commit to varshaprasad96/k8s-enhancements that referenced this issue Sep 22, 2023
This PR introduces a proposal to address the issue
kubernetes/kubectl#1442.

It intends to:
1. Ensure consistency by requiring that all flags are
   accepted as key-value pairs.
2. Remove the use of `NoOptDefVal` from kubectl.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
varshaprasad96 added a commit to varshaprasad96/k8s-enhancements that referenced this issue Sep 22, 2023
This PR introduces a proposal to address the issue
kubernetes/kubectl#1442.

It intends to:
1. Ensure consistency by requiring that all flags are
   accepted as key-value pairs.
2. Remove the use of `NoOptDefVal` from kubectl.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
varshaprasad96 added a commit to varshaprasad96/k8s-enhancements that referenced this issue Sep 22, 2023
This PR introduces a proposal to address the issue
kubernetes/kubectl#1442.

It intends to:
1. Ensure consistency by requiring that all flags are
   accepted as key-value pairs.
2. Remove the use of `NoOptDefVal` from kubectl.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
varshaprasad96 added a commit to varshaprasad96/k8s-enhancements that referenced this issue Sep 22, 2023
This PR introduces a proposal to address the issue
kubernetes/kubectl#1442.

It intends to:
1. Ensure consistency by requiring that all flags are
   accepted as key-value pairs.
2. Remove the use of `NoOptDefVal` from kubectl.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
varshaprasad96 added a commit to varshaprasad96/k8s-enhancements that referenced this issue Sep 28, 2023
This PR introduces a proposal to address the issue
kubernetes/kubectl#1442.

It intends to:
1. Ensure consistency by requiring that all flags are
   accepted as key-value pairs.
2. Remove the use of `NoOptDefVal` from kubectl.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jul 2, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 30, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 30, 2024
@devoxel
Copy link

devoxel commented Dec 24, 2024

I just bumped into a highly undesired behavior performing a kubectl delete sts my-sts --cascade orphan (tested in kubectl versions v1.26.6 and v1.27.3).

I did this in production and caused a (small internal) outage.

In most CLI's I have used ["--cascade=orphan"] would be the same as ["--cascade","orphan"]. It's a pretty razor thin margin that forgetting a single character means potentially wiping out your whole production workload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
Development

No branches or pull requests

9 participants