Skip to content
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

opts: ParseRestartPolicy: improve validation of max restart-counts #4535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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

Merging #4535 (8c8fdf2) into master (79f24c7) will decrease coverage by 59.41%.
The diff coverage is n/a.

❗ Current head 8c8fdf2 differs from pull request most recent head eced5eb. Consider uploading reports for the commit eced5eb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #4535       +/-   ##
==========================================
- Coverage   59.40%       0   -59.41%     
==========================================
  Files         288       0      -288     
  Lines       24797       0    -24797     
==========================================
- Hits        14730       0    -14730     
+ Misses       9186       0     -9186     
+ Partials      881       0      -881     

@thaJeztah thaJeztah force-pushed the restart_policy_more_validate branch 4 times, most recently from c963a9a to cfd30ac Compare August 28, 2023 21:04
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 <[email protected]>
@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
@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

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