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

[public-api] Remove google.Status from proto responses #9606

Merged
merged 1 commit into from
May 3, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Apr 28, 2022

Description

  • Removes google.Status from proto responses. This is redundant as the gRPC response already has the google.Status in it. It's either in the form of an Error, or an OK which doesn't need the status. If we need more detailed status errors, we can use the native error (and status) model https://cloud.google.com/apis/design/errors, with rich and detailed error messages serialized in the Error.details field
  • Removes linter which was enforcing this (if we want a linter for something else, we can restore from git)

Related Issue(s)

How to test

  • CI runs
  • cd components/public-api && ./generate.sh works

Release Notes

NONE

Documentation

NONE

@easyCZ easyCZ force-pushed the mp/public-api-remove-gp-lint branch from 748e3d7 to 7776dec Compare April 28, 2022 12:52
@easyCZ easyCZ marked this pull request as ready for review April 28, 2022 12:52
@easyCZ easyCZ requested a review from a team April 28, 2022 12:52
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 28, 2022
@csweichel
Copy link
Contributor

The intent of shipping google.Status was provide structured errors from day 1. If we remove it, we'll have a harder time adding structured errors back in on a later day.

Not hell-bent on the google.Status implementation, but there's been plenty a discussion on this topic.
Let's catch up on this in the sync call tomorrow.

@easyCZ
Copy link
Member Author

easyCZ commented Apr 28, 2022

So google.Status represents the Response Status, similar to HTTP Response Codes. It is not intended to communicate the underlying resource status, just the ability to respond to a request. For that, we should define our own (and we already do in the form of WorkspaceStatus and similar).

To put it differently, we wouldn't include

{ statusCode: 404, statusMessage: Not Found }

In a regular HTTP response, but that's what we're doing here with google.Status.

@geropl
Copy link
Member

geropl commented Apr 29, 2022

My understanding from the Offsite, after some lengthy discussions, was:

  • we all want structured error types
  • we have two suggestions on how to implement that. We try out both with small examples, and judge based on how the client code looks like. IIRC the options were:
    • map errors to google.Status
    • use a custom field as initially suggested here (but in a slightly improved way)

My understanding was that for both approaches we do not need the explicity response_status. 🤔

Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

@easyCZ and I had an offline discussion where we came to the conclusion that:

  • google.Status does not serve the purpose we had intended, i.e. it's too generic to provide meaningful structured errors,
  • we'll want to keep things simple and go with gRPCs built-in errors for now - we can add structured errors down the road if we need to.

@easyCZ
Copy link
Member Author

easyCZ commented May 2, 2022

/unhold

@easyCZ
Copy link
Member Author

easyCZ commented May 2, 2022

/hold

@easyCZ easyCZ force-pushed the mp/public-api-remove-gp-lint branch from 7776dec to 8a20df8 Compare May 2, 2022 07:59
@easyCZ
Copy link
Member Author

easyCZ commented May 2, 2022

/unhold

@easyCZ easyCZ force-pushed the mp/public-api-remove-gp-lint branch 2 times, most recently from d4d0162 to bea7a54 Compare May 2, 2022 18:48
@easyCZ easyCZ force-pushed the mp/public-api-remove-gp-lint branch from bea7a54 to e4d09bc Compare May 2, 2022 18:50
@easyCZ
Copy link
Member Author

easyCZ commented May 3, 2022

/werft run

👍 started the job as gitpod-build-mp-public-api-remove-gp-lint.8
(with .werft/ from main)

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM (see discussion ☝️ )

@roboquat roboquat merged commit daba3bb into main May 3, 2022
@roboquat roboquat deleted the mp/public-api-remove-gp-lint branch May 3, 2022 07:30
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XXL team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants