Implementing GitHub webhook handler #331
Conversation
Docs-only changes do not require a changelog entry, as we reserve the changelog for changes to the product. Edit the changelog workflow to ignore docs-only changes.
Automatically add the `no-changelog` label for docs-only changes. This reduces friction for docs authors, since docs-only changes usually don't require changelog entries. Do this instead of automatically skipping the changelog check for docs-only changes, since our release workflow often requires us to add or check the `no-changelog` label manually.
…ocs-no-changelog Do not require changelog entry for docs
In `CheckDocsPathsForMissingRedirects`, a `trace.Wrap` call mistakenly includes an unrelated error as an argument, and the error is either nil or handled earlier in the function. This means that docs pull requests with insufficient redirects still cause the workflow to pass. This change fixes the issue and adds testing for error handling in `CheckDocsPathsForMissingRedirects`. As part of this, it improves some other error messages returned by the function.
Signed-off-by: Fred Heinecke <fred.heinecke@goteleport.com>
There was a problem hiding this comment.
Overall LGTM. We might want to merge the base of this PR into master though, or its PR will become massive by the time we're done.
@r0mant, would you rather we file a bunch of smaller broken/development/not building PRs (which you'll need to review as only eng leads are codeowners), or one massive one later?
| head.GitHubHookInstallationTargetID = r.Header.Get("X-GitHub-Hook-Installation-Target-ID") | ||
| head.HubSignature256 = r.Header.Get("X-Hub-Signature-256") | ||
|
|
||
| payload, err := github.ValidatePayload(r, h.secretToken) // If secretToken is empty, the signature will not be verified. |
There was a problem hiding this comment.
I think that we should error if the secret token is empty and a signature is provided. If this happens, we've probably misconfigured something.
libs/github/webhook/webhook.go
Outdated
|
|
||
| payload, err := github.ValidatePayload(r, h.secretToken) // If secretToken is empty, the signature will not be verified. | ||
| if err != nil { | ||
| h.log.Warn("webhook validation failed", "headers", head) |
There was a problem hiding this comment.
In all these cases we should probably include some information about the request itself in the log, for debugging purposes. Or maybe the handler (or possibly the ingress gateway) should be able to log the entire request if debug logging is enabled?
| }) | ||
| mux.Handle("/webhook", webhook.NewHandler( | ||
| eventProcessor, | ||
| webhook.WithSecretToken([]byte("secret-token")), // TODO: get from config |
There was a problem hiding this comment.
Maybe this should always take a string? It's a string when configured with GH, and I don't see when we would want to pass a byte array to this func.
| ghes.srv.Shutdown(context.Background()) // Ignore error - we're already handling one | ||
| case <-ctx.Done(): | ||
| err = ghes.srv.Shutdown(context.Background()) |
There was a problem hiding this comment.
These should probably take a time-limited shutdown context stemming from context.Background().
| deployReviewChan <- event | ||
| return nil | ||
| default: | ||
| return trace.Errorf("unknown event type: %T", event) |
There was a problem hiding this comment.
This library only exists for Teleport error handling, so we probably shouldn't be using it anymore outside of the product (here and elsewhere)
The workflow currently fails if the event is not a pull request, since it attempts to list files for a PR with number 0 and receives a 404 from the GitHub API. Edit the workflow so it does not return an error if the event is not a PR.
Make the missing reviewer error message easier to read
…8-docpaths-nopr [bot] Allow for non-PR events in docpaths
The docpaths workflow currently blocks pull requests that rename or delete partials, which is not intended: the docs engine does not generate routes for partials. This change skips redirect checking for any file path that includes the `/includes/` segment. This follows the logic of the docs engine, which checks whether a page path includes the `/includes/` segment and, if not, moves it into the directory that Docusaurus uses to generate page routes.
…5-docpaths [bot] Ignore includes in docpaths
…laintext-1 Support decoding `--reviewers` as plain-text JSON
Signed-off-by: Fred Heinecke <fred.heinecke@goteleport.com>
doggydogworld
force-pushed
the
gus/additions-to-approval-service
branch
from
709d8d1 to
3df2705
Compare
Created an implementation for the GitHub webhook. It is just a small wrapper around the go-github library.
Made some changes to the main function to use
errgroupto run theeventSourcesconcurrently. Theerrgrouplibrary handles propagating cancellations that result from errors and is similar in functionality to waitgroups. In the event of an error from one of theeventSources, theerrgroupwill cancel the context which all goroutines in the group can react to appropriately.