Better error message when checking rollout status for StatefulSet wit… #66983
Conversation
k8s-ci-robot
added
release-note
|
@mortent thank you for your first contribution! /sig cli edit: a release note might not be needed here. waiting on others to confirm. |
k8s-ci-robot
added
sig/cli
|
/retest |
|
/assign @janetkuo |
pkg/kubectl/rollout_status.go
Outdated
| @@ -126,7 +126,7 @@ func (s *StatefulSetStatusViewer) Status(namespace, name string, revision int64) | |||
| return "", false, err | |||
| } | |||
| if sts.Spec.UpdateStrategy.Type == apps.OnDeleteStatefulSetStrategyType { | |||
| return "", true, fmt.Errorf("%s updateStrategy does not have a Status", apps.OnDeleteStatefulSetStrategyType) | |||
| return "", true, fmt.Errorf("Status is not available for the OnDelete strategy type") | |||
There was a problem hiding this comment.
I think we should use != instead, i.e.
if sts.Spec.UpdateStrategy.Type != apps.RollingUpdateStatefulSetStrategyType {
return "", true, fmt.Errorf("rollout status is only available for %s strategy type", apps.RollingUpdateStatefulSetStrategyType)Note that errors should not be capitalized, because it's usually appended to another error like this fmt.Errorf("failed to do this: %v", err). Using rollout status instead of status to avoid users confusing status command with status API field.
Please also update the error message in DaemonSet to make them consistent.
There was a problem hiding this comment.
@janetkuo PR is updated.
mortent
force-pushed
the
BetterRolloutStatusMsgForStatefulSet
branch
2 times, most recently
from
48af149 to
df77a47
Compare
…h OnDelete strategy type
mortent
force-pushed
the
BetterRolloutStatusMsgForStatefulSet
branch
from
df77a47 to
8e49727
Compare
|
/retest |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: janetkuo, mortent 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 |
k8s-ci-robot
added
the
approved
|
/retest |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
…h OnDelete strategy type
What this PR does / why we need it: The error message when checking the rollout status for a StatefulSet with the OnDelete strategy type can be confusing (ref #64500). It gives the impression that something has gone wrong when the issue is simply that there is no rollout status. The error message is updated to use similar language as for DaemonSet in the same situation.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Special notes for your reviewer:
Release note: