-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[server][github] record "pull" webhook events #13292
Conversation
started the job as gitpod-build-at-pull-events.1 because the annotations in the pull request description changed |
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.
Couple of questions to help me understand this PR better.
res.redirect(`https://github.com/apps/${slug}/installations/new?state=${state}`); | ||
} catch (error) { | ||
console.error(error, { error }); | ||
res.status(500).send("GitHub App is not configured."); |
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.
Are all errors 500 errors? Can it happen that const data = await gh.apps.getAuthenticated();
gives us a Not Found?
I'm trying to understand if all errors are in fact Server Errors, or if there are modes which fail due to user errors etc. Always returning a 500 has the obvious impact to our SLOs and may not always communicate the underlying problem to the user well.
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.
Can it happen that const data = await gh.apps.getAuthenticated(); gives us a Not Found?
Don't think so. If auth
succeed, it should be an in scope of an authenticated app, thus getAuthenticated
should return its details.
https://docs.github.com/en/rest/apps/apps#get-the-authenticated-app
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'm trying to understand if all errors are in fact Server Errors
👍🏻
In that case we're navigating to /reconfigure
, but there is an issue with GH App settings. Example for this is in preview enviroments, or self-hosted installations.
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.
What about if auth
doesn't succeed? Does it still make sense to return a 500, even if the reason is that the user doesn't exist or it has an invalid access token?
I'd expect here that we only return a 500 if we fail for a reason that's not expected - Users not being found or not existing are expected reasons.
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.
Actually, there was a GH issue to do exactly this. I've hard times to find it, though.
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.
What about if auth doesn't succeed? Does it still make sense to return a 500, even if the reason is that the user doesn't exist or it has an invalid access token?
@easyCZ, I see. app.auth
is the request to use the configured GH App secrets to do the app authentication. Here it's the GH App of Gitpod which needs to do the handshake, in order to be auth'd for operations. That's not related to users. More on this: https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps#authenticating-as-a-github-app
If app authentication is failing, reason is most likely the app secrets aren't set up properly, or the app is not configured. Otherwise this endpoint should not be navigable from UI.
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.
To sum up: the code 500 isn't new here.
The effect of this change:
- readable error message to users, instead of error stack trace
- logging of the actual error/reason in logs, instead of swallowing 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.
Thanks for the clarifications, appreciate transferring context and helping me understand the impact better. While I'd like that we report more accurate status codes, it's not a reason to block this PR.
Description
Before this PR, only push events were recorded. Because Pull Requests updates are also triggers for prebuilds, they should be registered as well.
Related Issue(s)
Fixes #12817
How to test
Inspect
<PREVIEW-URL>/t/<TEAM>/<PROJECT>/events
for events recorded for updates of pull requests onPROJECT
repository.Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide