diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index fc6e80207c..bc6cec31ac 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -112,6 +112,8 @@ func NewGithubClient(hostname string, credentials GithubCredentials, logger logg func (g *GithubClient) GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error) { var files []string nextPage := 0 + +listloop: for { opts := github.ListOptions{ PerPage: 300, @@ -119,24 +121,42 @@ func (g *GithubClient) GetModifiedFiles(repo models.Repo, pull models.PullReques if nextPage != 0 { opts.Page = nextPage } - g.logger.Debug("GET /repos/%v/%v/pulls/%d/files", repo.Owner, repo.Name, pull.Num) - pageFiles, resp, err := g.client.PullRequests.ListFiles(g.ctx, repo.Owner, repo.Name, pull.Num, &opts) - if err != nil { - return files, err - } - for _, f := range pageFiles { - files = append(files, f.GetFilename()) + // GitHub has started to return 404's sometimes. They've got some + // eventual consistency issues going on so we're just going to attempt + // up to 5 times for each page with exponential backoff. + maxAttempts := 5 + attemptDelay := 0 * time.Second + for i := 0; i < maxAttempts; i++ { + // First don't sleep, then sleep 1, 3, 7, etc. + time.Sleep(attemptDelay) + attemptDelay = 2*attemptDelay + 1*time.Second + + g.logger.Debug("[attempt %d] GET /repos/%v/%v/pulls/%d/files", i+1, repo.Owner, repo.Name, pull.Num) + pageFiles, resp, err := g.client.PullRequests.ListFiles(g.ctx, repo.Owner, repo.Name, pull.Num, &opts) + if err != nil { + ghErr, ok := err.(*github.ErrorResponse) + if ok && ghErr.Response.StatusCode == 404 { + // (hopefully) transient 404, retry after backoff + continue + } + // something else, give up + return files, err + } + for _, f := range pageFiles { + files = append(files, f.GetFilename()) - // If the file was renamed, we'll want to run plan in the directory - // it was moved from as well. - if f.GetStatus() == "renamed" { - files = append(files, f.GetPreviousFilename()) + // If the file was renamed, we'll want to run plan in the directory + // it was moved from as well. + if f.GetStatus() == "renamed" { + files = append(files, f.GetPreviousFilename()) + } } - } - if resp.NextPage == 0 { + if resp.NextPage == 0 { + break listloop + } + nextPage = resp.NextPage break } - nextPage = resp.NextPage } return files, nil } diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index bb4f55703e..88f0888d3f 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -907,3 +907,49 @@ func TestGithubClient_Retry404(t *testing.T) { Ok(t, err) Equals(t, 3, numCalls) } + +// Test that we retry the get pull request files call if it 404s. +func TestGithubClient_Retry404Files(t *testing.T) { + var numCalls = 0 + + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + + switch r.Method + " " + r.RequestURI { + case "GET /api/v3/repos/runatlantis/atlantis/pulls/1/files?per_page=300": + defer r.Body.Close() // nolint: errcheck + numCalls++ + if numCalls < 3 { + w.WriteHeader(404) + } else { + w.WriteHeader(200) + } + return + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{"user", "pass"}, logging.NewNoopLogger(t)) + Ok(t, err) + defer disableSSLVerification()() + repo := models.Repo{ + FullName: "runatlantis/atlantis", + Owner: "runatlantis", + Name: "atlantis", + CloneURL: "", + SanitizedCloneURL: "", + VCSHost: models.VCSHost{ + Type: models.Github, + Hostname: "github.com", + }, + } + pr := models.PullRequest{Num: 1} + _, err = client.GetModifiedFiles(repo, pr) + Ok(t, err) + Equals(t, 3, numCalls) +}