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

Re-enable shorthand HTTP API #5168

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

tdeebswihart
Copy link
Contributor

@tdeebswihart tdeebswihart commented Nov 29, 2023

What changed?

This re-enables the shorthand payload format for our HTTP API.

Why?

This is a business requirement for our HTTP API.

How did you test it?

I updated our HTTP API tests to test all four mimetypes we support

Potential risks

None

Is hotfix candidate?

No.

service/frontend/protojson_marshaler.go Outdated Show resolved Hide resolved
service/frontend/protojson_marshaler.go Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

nit: I still see a couple of references to gogo in comments in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have enough context to know whether the claims there are still true. @cretz, as the author, do the claims about gogo-status-compatible but not grpc-status-compatible in errorHandler still hold?

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

I only had a couple of nits.

Not super familiar with this part of the codebase but the changes LGTM.

@tdeebswihart tdeebswihart marked this pull request as ready for review November 29, 2023 17:12
@tdeebswihart tdeebswihart requested a review from a team as a code owner November 29, 2023 17:12
@tdeebswihart
Copy link
Contributor Author

I'm setting this to automerge when tests pass to unblock other work that's hurt by the api-go changes. This should be the last time I break things

@tdeebswihart tdeebswihart enabled auto-merge (squash) November 29, 2023 17:14
@tdeebswihart tdeebswihart merged commit 9806d87 into main Nov 29, 2023
10 checks passed
@tdeebswihart tdeebswihart deleted the tdeebswihart/shorthand-http-api branch November 29, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants