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

don't parse error messages as json #694

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

bobcallaway
Copy link
Member

Summary

I don't think we need to pass a parsed JSON object up to the caller in the context of RekorClientError

@woodruffw thoughts?

woodruffw
woodruffw previously approved these changes Jun 28, 2023
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Agreed!

Does the Rekor REST API specify anywhere that it may return JSON vs. plain text on error codes? I don't think it's a significant issue, but if we ever elaborate on this exception type it'd be nice to know what we can rely on/assume.

@woodruffw
Copy link
Member

Nonblocking, but mind adding a small CHANGELOG entry as well? Something under the "Fixed" category to the effect of "Fixed an assumption about Rekor error responses that was causing crashes"

If not, I can add it later.

@bobcallaway
Copy link
Member Author

Nonblocking, but mind adding a small CHANGELOG entry as well? Something under the "Fixed" category to the effect of "Fixed an assumption about Rekor error responses that was causing crashes"

If not, I can add it later.

I'll add it, no problem.

@bobcallaway
Copy link
Member Author

Agreed!

Does the Rekor REST API specify anywhere that it may return JSON vs. plain text on error codes? I don't think it's a significant issue, but if we ever elaborate on this exception type it'd be nice to know what we can rely on/assume.

not really? The Content-Type on the response is text/html; charset=UTF-8 so while I don't think we're out of HTTP spec, I'm not sure where we'd declare it (perhaps in the openapi.yaml file?)

Signed-off-by: Bob Callaway <[email protected]>
@woodruffw woodruffw merged commit b40f5fe into sigstore:main Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api Public APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants