Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 28, 2023

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2023

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
opts/parse.go 0.00% 2 Missing ⚠️
cli/compose/convert/service.go 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@thaJeztah thaJeztah force-pushed the restart_policy_more_validate branch 5 times, most recently from cfd30ac to ced6336 Compare August 28, 2023 21:05
@thaJeztah thaJeztah marked this pull request as ready for review August 29, 2023 12:24
@thaJeztah
Copy link
Member Author

⚠️ For reviewers; I'm leaning two ways on this PR;

  • 👍 validate early
  • 👎 there IS a potential (although unlikely that these combinations could be supported by a future API version
  • 👍 then again, the CLI does not support "future" versions of the API
  • 👍 and .. there's already code invalidating "unknown" restart policies
  • 🤷‍♂️ so I guess this doesn't make things worse

@thaJeztah thaJeztah modified the milestones: 25.0.0, 26.0.0 Jan 19, 2024
vvoland
vvoland previously approved these changes Feb 21, 2024
@thaJeztah
Copy link
Member Author

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.

@vvoland
Copy link
Collaborator

vvoland commented Feb 22, 2024

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.

@vvoland vvoland modified the milestones: 26.0.0, 27.0.0 Mar 14, 2024
@vvoland vvoland modified the milestones: 27.0.0, v-future Jun 20, 2024
@vvoland
Copy link
Collaborator

vvoland commented Jun 20, 2024

@thaJeztah are we still considering this?

@thaJeztah
Copy link
Member Author

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants