Remove specific command suggestion from kubectl rollout undo error message

[lojies]

What type of PR is this?

/kind bug

What this PR does / why we need it:

add namespace in error message command

Which issue(s) this PR fixes:

Fixes #111794

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

[k8s-ci-robot]

Please note that we're already in Test Freeze for the release-1.25 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.25.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Aug 11 01:31:23 UTC 2022.

[k8s-ci-robot]

@lojies: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[lojies]

/test pull-kubernetes-verify

[ardaguclu]

This change sounds reasonable to me.

Would it be better to also update examples?

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_undo.go

Lines 59 to 67 in c4f8c57

	undoExample = templates.Examples(`
	# Roll back to the previous deployment
	kubectl rollout undo deployment/abc
	# Roll back to daemonset revision 3
	kubectl rollout undo daemonset/abc --to-revision=3
	# Roll back to the previous deployment with dry-run
	kubectl rollout undo --dry-run=server deployment/abc`)

[lojies]

This change sounds reasonable to me.

Would it be better to also update examples?

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/rollout/rollout_undo.go

Lines 59 to 67 in c4f8c57

	undoExample = templates.Examples(`
	# Roll back to the previous deployment
	kubectl rollout undo deployment/abc
	# Roll back to daemonset revision 3
	kubectl rollout undo daemonset/abc --to-revision=3
	# Roll back to the previous deployment with dry-run
	kubectl rollout undo --dry-run=server deployment/abc`)

It seems like no namespace added in all commands' examples, i'm not sure if it's better to update this command only.

[ardaguclu]

You are right.

/lgtm

[VirrageS]

Are you sure that in case of default namespace the error will be "-n default" and not "-n "?

[brianpursley]

This is a good point. The namespace variable here is coming from the object's metadata.namespace, and reading this comment makes me THINK it should be OK, but it doesn't clear things up 100%:

An empty namespace is equivalent to the "default" namespace, but "default" is the canonical representation

This implies, to me, that empty will be change to "default". But on the other hand it implies that it COULD be empty and still be valid. Am I reading this correctly? It sounds like we might want to check if namespace is empty, and use default if so, just to be safe.

Also, we can't just omit the -n in this case, even though for many people "default" is their default namespace, you can configure a kubectl context to use a different namespace, so it would get complicated to try to figure out whether "default" is their default namespace in their current kubectl context.

[brianpursley]

There is part of me that thinks maybe we should not be telling people exactly which command to run. Maybe we should be just saying:

...resume it first with 'kubectl rollout resume'

and not try to build up the entire command for them.

Is kubectl trying to do too much here?

For example, we don't know if they are supposed to be using --context=production or something like that. Telling them to run a specific complete command seems like it could be a somewhat dangerous thing.

I think there is a difference between this and help text examples, because people understand that they need to adapt help text examples to their use case, but when we are including their deployment name and namespace in the completely runnable command, it implies that we are telling them that it is correct to run that specific command.

Am I making sense? Thoughts about this?

[lojies]

I tried and we got -n default.This can be run correctly, but as @brianpursley mentioned, it seems like a little strange and is maybe still not a complete command.

[root@lzx-22:/root]$ kubectl  rollout undo deployment kubia
error: you cannot rollback a paused deployment; resume it first with 'kubectl rollout resume deployment/kubia -n default' and try again

Are you sure that in case of default namespace the error will be "-n default" and not "-n "?

[lojies]

@brianpursley, I agree with you. If we cannot give the complete command, i think it's better to just use kubectl rollout resume.

[KnVerey]

Telling them to run a specific complete command seems like it could be a somewhat dangerous thing.

👍 I also agree with you, Brian. As you're saying, it's not helpful for it to look runnable and complete while potentially not being exactly what they need to run under the circumstances.

[lojies]

@brianpursley @VirrageS, so is it ok that change like this?

return "", fmt.Errorf("you cannot rollback a paused deployment; resume it first with 'kubectl rollout resume' and try again", name)

[brianpursley]

@brianpursley @VirrageS, so is it ok that change like this?

return "", fmt.Errorf("you cannot rollback a paused deployment; resume it first with 'kubectl rollout resume' and try again", name)

Yeah, the wording is good, but obviously you would not need to pass name as an argument anymore because the error message is just a plain string now.

[lojies]

@brianpursley @VirrageS, so is it ok that change like this?

return "", fmt.Errorf("you cannot rollback a paused deployment; resume it first with 'kubectl rollout resume' and try again", name)

Yeah, the wording is good, but obviously you would not need to pass name as an argument anymore because the error message is just a plain string now.

Yes, i have changed this, thanks.

[brianpursley]

/retitle Remove specific command suggestion from kubectl rollout undo error message

[brianpursley]

/lgtm
/approve

Thanks @lojies

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, lojies

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:

• staging/src/k8s.io/kubectl/OWNERS [brianpursley]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment