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
api: ValidateRestartPolicy: improve errors for invalid policies #46352
Conversation
945e816
to
b951def
Compare
Actually, I think it can be slightly more terse; reading it now, I see "restart-policy" is repeated in the error. Let me change "can only be used with the 'on-failure' restart policy" to "can only be used with 'on-failure'" |
b951def
to
fdac4ea
Compare
Updated 👍 |
Ah, dang. Missed a test somewhere;
|
Make the error message slightly clearer on "what" part is not valid, and provide suggestions on what are acceptable values. Before this change: docker create --restart=always:3 busybox Error response from daemon: invalid restart policy: maximum retry count cannot be used with restart policy 'always' docker create --restart=always:-1 busybox Error response from daemon: invalid restart policy: maximum retry count cannot be used with restart policy 'always' docker create --restart=unknown busybox Error response from daemon: invalid restart policy 'unknown' After this change: docker create --restart=always:3 busybox Error response from daemon: invalid restart policy: maximum retry count can only be used with 'on-failure' docker create --restart=always:-1 busybox Error response from daemon: invalid restart policy: maximum retry count can only be used with 'on-failure' and cannot be negative docker create --restart=unknown busybox Error response from daemon: invalid restart policy: unknown policy 'unknown'; use one of 'no', 'always', 'on-failure', or 'unless-stopped' Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
fdac4ea
to
f6f6c32
Compare
if policy.MaximumRetryCount < 0 { | ||
msg += " and cannot be negative" | ||
} | ||
return &errInvalidParameter{fmt.Errorf(msg)} |
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:
return &errInvalidParameter{fmt.Errorf(msg)} | |
return &errInvalidParameter{errors.New(msg)} |
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.
Ah, yes; was considering that, but we didn't import "errors"
yet, so thought I'd keep it at fmt.Errorf()
😅
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.
Works for me :)
Make the error message slightly clearer on "what" part is not valid, and provide suggestions on what are acceptable values.
Before this change:
After this change:
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)