-
Notifications
You must be signed in to change notification settings - Fork 365
Improve error messaging for creating a Deployment that will violate quotas with high max-in-flight #4402
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: main
Are you sure you want to change the base?
Conversation
…uotas with high max-in-flight Fix error message to be more descriptive
…quotas with high max-in-flight Fix Rubocop errors
…uotas with high max-in-flight Only update the error message for rolling deployments based on code review
…uotas with high max-in-flight Revert changes that affect all actions since we only want to update rolling reployments
…uotas with high max-in-flight Fix rubocop errors
…uotas with high max-in-flight Update error message for space_app_instance_limit_exceeded
…uotas with high max-in-flight Fix Rubocop errors
…uotas with high max-in-flight Cleanup for Rubocop errors
…uotas with high max-in-flight Update error message for org errors during rolling deployment
…uotas with high max-in-flight Fix merge issues
…uotas with high max-in-flight Fix Rubocop error
end | ||
|
||
def handle_deployment_create_error(e, app) | ||
space_quota_errors = [:space_quota_exceeded.to_s, :space_app_instance_limit_exceeded.to_s] |
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 instead of making symbols and turning them to Strings, you could make an array of literals with this syntax:
%w(space_quota_exceeded space_app_instance_limit_exceeded)
elsif org_quota_errors.any? { |substring| e.message.include?(substring) } | ||
org_error_msg_1 = " for organization #{app.organization.name}. " | ||
org_error_msg_2 = "This organization's quota may not be large enough to support rolling deployments or your configured max-in-flight." | ||
error = DeploymentCreate::Error.new(e.message + org_error_msg_1 + org_error_msg_2) |
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.
I wonder if it's worth all this extra logic to make the space/org distinction. What if you checked for any quota error and simply added One or more quotas may not be large enough to support rolling deployments or your configured max-in-flight.
to then end? Could simplify the code a bunch. What do you think?
Tested this out:
Definitely is more helpful in the context of deployments. 👍 |
Fix error message to be more descriptive
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
When creating a Deployment, it can be unclear that a quota violation is due to a high max-in-flight. Enhance the error message with better description and suggestions for remediation.
Previously the error was "memory space_quota_exceeded". With the change it's "memory space_quota_exceeded for space space1. This space's quota may not be large enough to support rolling deployments or your configured max-in-flight."
For an org level quota error after change is as follows "memory quota_exceeded for organization org1. This organization's quota may not be large enough to support rolling deployments or your configured max-in-flight."
An explanation of the use cases your change solves
Links to any other associated PRs
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
main
branchI have run all the unit tests using
bundle exec rake
I have run CF Acceptance Tests