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

Deployment.concurrency_limit cannot be zero. #15228

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

collincchoy
Copy link
Contributor

@collincchoy collincchoy commented Sep 4, 2024

This PR tweaks API schemas for the recently added Deployment.concurrency_limit field to narrow allowed values to exclude the literal value 0 as it's confusing how it intersects with concurrency_limit=None and disabling a deployment.

Important

This is technically a breaking api change and is also inconsistent with Work pool concurrency limits which similarly allow 0.
We want to do this while this feature is still rather new/in-development and to pre-empt confusion with disabling deployments.

Also worth noting that the UI will include client-side validation for this greater-than-0 behavior already

Happy to close this small PR though if we think this additional validation isn't worth it

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

Copy link

codspeed-hq bot commented Sep 4, 2024

CodSpeed Performance Report

Merging #15228 will not alter performance

Comparing non-zero-deployment-concurrency-limits (b0158d3) with main (522c254)

Summary

✅ 3 untouched benchmarks

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me! thank you

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch @collincchoy

@collincchoy collincchoy merged commit ce50e5e into main Sep 5, 2024
39 checks passed
@collincchoy collincchoy deleted the non-zero-deployment-concurrency-limits branch September 5, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants