-
Notifications
You must be signed in to change notification settings - Fork 2.1k
opts: ParseRestartPolicy: improve validation of max restart-counts #4535
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
base: master
Are you sure you want to change the base?
Conversation
d40f601 to
618bbfe
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
cfd30ac to
ced6336
Compare
|
|
Honestly, I'm still a bit on the fence on this one, and not sure how much validation we can do on the client side (as "what's supported" may change on the daemon side, and even between API versions). To some extent considering if we should go the reverse; just parse the general format, and let the daemon return errors where things are invalid. |
|
I'm kinda neutral here - even if we'd change anything, that would still be gated by the API version which would require a newer CLI anyway. But on the other hand... We will have the error message even without it, so there's no real gain for the UX. |
|
@thaJeztah are we still considering this? |
|
I need to look at this one again; it's probably not urgent, so might be fine for later |
Use the new container.ValidateRestartPolicy utility to verify if a max-restart-count is allowed for the given restart-policy. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
ced6336 to
1fb63eb
Compare
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)