-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix diagnostic messages on update environment #422
Conversation
3dca0e6
to
1cdee31
Compare
cmd/esc/cli/client/client.go
Outdated
@@ -560,7 +560,7 @@ func (pc *client) UpdateEnvironmentWithRevision( | |||
}) | |||
if err != nil { | |||
var diags *EnvironmentErrorResponse | |||
if errors.As(err, &diags) && diags.Code == http.StatusBadRequest && len(diags.Diagnostics) != 0 { | |||
if errors.As(err, &diags) && (diags.Code == http.StatusBadRequest || len(diags.Diagnostics) != 0) { |
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.
This is because on certain 4xx errors, the pulumi service returns an object without {..., "code": 4xx
}
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.
Hmm, should we fix this in PS? Sounds like it's not following a required API contract?
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 initially considered this but I think it's a much bigger lift - we follow this "pattern" in many routes where certain errors will return code
and message
by default, but otherwise not.
I think perhaps a better fix is to check the http response status code instead. @pgavlin I notice that we don't return the http response on 4xx/5xx errors, is this intentional?
1cdee31
to
cf8b473
Compare
cmd/esc/cli/client/client.go
Outdated
@@ -560,7 +560,7 @@ func (pc *client) UpdateEnvironmentWithRevision( | |||
}) | |||
if err != nil { | |||
var diags *EnvironmentErrorResponse | |||
if errors.As(err, &diags) && diags.Code == http.StatusBadRequest && len(diags.Diagnostics) != 0 { | |||
if errors.As(err, &diags) && (diags.Code == http.StatusBadRequest || len(diags.Diagnostics) != 0) { |
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.
Hmm, should we fix this in PS? Sounds like it's not following a required API contract?
cf8b473
to
bf047fa
Compare
bf047fa
to
0136bbb
Compare
Fixes #370
When updating an env with an invalid definition,
UpdateEnvironmentWithRevision
should return no error but with diagnosticsTesting
Because of this issue, we previously wouldn't reach here (which requires non-error but with diagnostics), but with this change, now we can, because
UpdateEnvironmentWithRevision
will now return no error but with diagnostics on invalid definitionsWhen updating an env with an invalid definition:
Before:
Now: