-
Notifications
You must be signed in to change notification settings - Fork 59
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
Do Cloud SDK validation before local run. #2211
Conversation
@@ -32,7 +32,7 @@ | |||
CloudSdkMessageBundle.message("appengine.cloudsdk.java.components.missing") | |||
+ "\n" | |||
+ CloudSdkMessageBundle.message("appengine.cloudsdk.java.components.howtoinstall"), | |||
false), | |||
true), |
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.
so will now cause any usage of the Cloud Sdk without the java component to show an error to the user?
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 don't completely remember the behavior before; if we were requiring the appengine java component for all custom sdk usages).
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.
Looks like this field was never used before!
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.
It was never required to have App Engine Java to set the custom SDK path, but if you do select custom SDK without App Engine component in Cloud SDK settings, it will show you an error message (not preventing from setting the path though). So I guess we sort of softly required it.
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.
ah thats right. so now theres the potential that users that previously were successfully doing things like flex deployments without the java components installed will be getting an error?
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.
oh but this PR is only affecting local run..nm
Fixes #2209.
Local run (probably a managed SDK refactoring regression) does not do Cloud SDK validation before attempting launching local dev server aside from Managed SDK checks. This results in errors not handled if custom SDK is misconfigured.
Result (same custom SDK without App engine java component as in the issue):
Error is properly shown without throwing unhandled exception. When SDK is fixed, error does not show.
To make sure App Engine Java component missing is an error, it's set as an error in
CloudSdkValidationResult
rather than a warning.