-
Notifications
You must be signed in to change notification settings - Fork 92
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
Add a package for wpt.fyi GitHub Checks #726
Conversation
Staging instance deployed by Travis CI! |
Staging instance deployed by Travis CI! |
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.
First round of review (without the cryptography part)
api/checks/routes.go
Outdated
// RegisterRoutes adds route handlers for webhooks. | ||
func RegisterRoutes() { | ||
// GitHub webhook for creating custom status checks. | ||
shared.AddRoute("/api/webhook/status", "api-webhook-check", checkWebhookHandler) |
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.
The inconsistency between the two strings is confusing. Is it a (legacy) status or a check?
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.
It's a check, and that's what the endpoint should be (Fixed).
|
||
// checkWebhookHandler listens for check_suite and check_run events, | ||
// responding to requested and rerequested events. | ||
func checkWebhookHandler(w http.ResponseWriter, r *http.Request) { |
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 function looks very similar to the one in taskcluster.go
, which is fine for now, but it'd be good to add a TODO to point out the potential refactoring opportunity.
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.
Already underway; See #721
api/checks/suites.go
Outdated
} | ||
|
||
// PendingCheckRun loads the CheckSuite(s), if any, for the given SHAs, and creates a pending | ||
// check_run for the given browser name for each CheckSuite. |
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.
Clarify the return value (i.e. whether a new run is created; also explain when it will create a run).
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.
Done.
api/checks/suites.go
Outdated
} | ||
|
||
// CompleteCheckRun creates a complete check_run for the given browser on GitHub, | ||
// for the given CheckSuite |
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.
Ditto.
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.
Done.
api/checks/suites.go
Outdated
return &suite, err | ||
} | ||
|
||
// GetSuitesForSHA returns any CheckSuite entities associated with the given SHA. |
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.
IIRC, the convention is to put the documentation for exported methods on the public interface?
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.
Done.
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 reviewed the authentication flow to my best effort; it LGTM.
So sad we don't have an up-to-date octokit for Go.
api/checks/webhook.go
Outdated
claims := &jwt.StandardClaims{ | ||
IssuedAt: now.Unix(), | ||
ExpiresAt: now.Add(time.Minute * 10).Unix(), | ||
Issuer: "19965", |
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 magic number is unfortunate, but at least we should have a comment here.
Description
Adds the main logic needed to create basic
in_progress
orcompleted
GitHubcheck_runs
.