From e6c8e6d3825739652246870bee1351b766127923 Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Mon, 20 Feb 2023 18:36:25 -0800 Subject: [PATCH] Improved handling of PR merging (#22) * This PR reduces the reliance on GitHub's automerge feature by adding improved handling of PR merging in the syncer. * If a PR exists the syncer will attempt to merge it and wait for it to merge this allows the syncer to push any changes that might be blocked on the pending PR * After creating a new PR with the latest changes the sycner will wait for the PR to be merged. This provides better handling in the case of dev takeover when automerge isn't (or can't be enabled) on the repository. There appears to be a bit of a race condition where the PR may not be merged right away even thought it should be immediately mergeable but this PR fixes that by having a retry loop that will wait for the PR to me merged and periodically retry merging * Fix #21 * Fix #17 --- Makefile | 4 ++ cmd/sanitizer/app/sanitizer.go | 2 +- cmd/sanitizer/app/sanitizer_test.go | 2 +- pkg/github/merge.go | 40 +++++++++++------ pkg/github/prs.go | 66 ++++++++++++++++++++++++++--- pkg/gitops/syncer.go | 30 +++++++++---- pkg/gitutil/gitutil.go | 2 +- 7 files changed, 116 insertions(+), 30 deletions(-) diff --git a/Makefile b/Makefile index d807f8a..c19dc29 100644 --- a/Makefile +++ b/Makefile @@ -8,9 +8,13 @@ tidy-go: gofmt -s -w . goimports -w . +tidy: tidy-go + lint-go: # golangci-lint automatically searches up the root tree for configuration files. golangci-lint run +lint: lint-go + test-go: go test -v ./... \ No newline at end of file diff --git a/cmd/sanitizer/app/sanitizer.go b/cmd/sanitizer/app/sanitizer.go index 0b1cc61..c61f783 100644 --- a/cmd/sanitizer/app/sanitizer.go +++ b/cmd/sanitizer/app/sanitizer.go @@ -177,7 +177,7 @@ func isUnsafeError(e error) bool { func (s *Sanitizer) cleanFile(path string, dest string) error { s.log.Info("Cleaning target", "target", path) - b, err := ioutil.ReadFile(path) + b, err := os.ReadFile(path) if err != nil { return errors.Wrapf(err, "Failed to clean file; %v", path) } diff --git a/cmd/sanitizer/app/sanitizer_test.go b/cmd/sanitizer/app/sanitizer_test.go index 59ca987..0b43337 100644 --- a/cmd/sanitizer/app/sanitizer_test.go +++ b/cmd/sanitizer/app/sanitizer_test.go @@ -12,7 +12,7 @@ import ( // Not really a unittest as we try to sanitize the hydros directory func Test_sanitize(t *testing.T) { - dir, err := ioutil.TempDir("", "testSanitize") + dir, err := os.MkdirTemp("", "testSanitize") if err != nil { t.Fatalf("Failed to create temporary directory; error %v", err) } diff --git a/pkg/github/merge.go b/pkg/github/merge.go index b2f3378..251f568 100644 --- a/pkg/github/merge.go +++ b/pkg/github/merge.go @@ -20,7 +20,14 @@ import ( "go.uber.org/zap" ) +// PRMergeState is different states the PR can be in. It aggregates information from multiple +// fields in the GitHub API (e.g. pr.State and pr.MergeStateStatus and pr.IsInMergeQueue) +type PRMergeState string + const ( + // MergeStateStatusBehind and other + // values correspond to enum values for the field pr.MergeStateStatus + // https://docs.github.com/en/graphql/reference/enums#mergestatestatus MergeStateStatusBehind = "BEHIND" MergeStateStatusBlocked = "BLOCKED" MergeStateStatusClosed = "CLOSED" @@ -29,6 +36,12 @@ const ( MergeStateStatusHasHooks = "HAS_HOOKS" MergeStateStatusMerged = "MERGED" MergeStateStatusUnstable = "UNSTABLE" + + MergedState PRMergeState = "MERGED" + EnqueuedState PRMergeState = "ENQUEUED" + ClosedState PRMergeState = "CLOSED" + UnknownState PRMergeState = "UNKNOWN" + BlockedState PRMergeState = "BLOCKED" ) // MergeOptions are the options used to construct a context. @@ -70,26 +83,26 @@ func (m *prMerger) inMergeQueue() error { } // Merge the pull request. -func (m *prMerger) merge() error { +func (m *prMerger) merge() (PRMergeState, error) { log := m.log pr := m.pr if pr.State == MergeStateStatusClosed { log.Info("PR can't be merged it has been closed") - return errors.Errorf("Can't merge PR %v it has been closed", pr.URL) + return ClosedState, errors.Errorf("Can't merge PR %v it has been closed", pr.URL) } if pr.State == MergeStateStatusMerged { log.Info("PR has already been merged") - return nil + return MergedState, nil } if err := m.inMergeQueue(); err != nil { log.Info("PR is already in merge queue") - return nil + return EnqueuedState, nil } if reason, blocked := blockedReason(m.pr.MergeStateStatus); blocked { log.Info("PR merging is blocked", "reason", reason) - return errors.Errorf("PR merging is blocked; MergeStateStatus: %v reason: %v", m.pr.MergeStateStatus, reason) + return BlockedState, errors.Errorf("PR merging is blocked; MergeStateStatus: %v reason: %v", m.pr.MergeStateStatus, reason) } payload := mergePayload{ @@ -119,16 +132,19 @@ func (m *prMerger) merge() error { err := mergePullRequest(m.HttpClient, payload) if err != nil { - return err + return UnknownState, err } + var state PRMergeState if payload.auto { log.Info("Pull request was added to merge queue and will be automatically merged when all requirements are met") + state = EnqueuedState } else { log.Info("pull request was merged", "title", m.pr.Title) + state = MergedState } - return nil + return state, nil } type addAcceptHeaderTransport struct { @@ -169,17 +185,13 @@ func newPRMerger(client *http.Client, repo ghrepo.Interface, number int) (*prMer // client - http client to use to talk to github // repo - the repo that owns the PR // number - the PR number to merge -func MergePR(client *http.Client, repo ghrepo.Interface, number int) error { +func MergePR(client *http.Client, repo ghrepo.Interface, number int) (PRMergeState, error) { m, err := newPRMerger(client, repo, number) if err != nil { - return err + return UnknownState, err } - if err := m.merge(); err != nil { - return err - } - - return err + return m.merge() } // blockedReason translates various MergeStateStatus GraphQL values into human-readable reason diff --git a/pkg/github/prs.go b/pkg/github/prs.go index d4b46bc..bbf1129 100644 --- a/pkg/github/prs.go +++ b/pkg/github/prs.go @@ -594,13 +594,69 @@ func (h *RepoHelper) Dir() string { // MergePR tries to merge the PR. This means either // 1. enabling auto merge if a merge queue is required // 2. merging right away if able -func (h *RepoHelper) MergePR(prNumber int) error { +func (h *RepoHelper) MergePR(prNumber int) (PRMergeState, error) { client := &http.Client{Transport: h.transport} - if err := MergePR(client, h.baseRepo, prNumber); err != nil { - h.log.Error(err, "Failed to merge PR (or enable auto merge)") - return errors.Wrapf(err, "Failed to merge PR (or enable auto merge)") + + return MergePR(client, h.baseRepo, prNumber) +} + +// MergeAndWait merges the PR and waits for it to be merged. +func (h *RepoHelper) MergeAndWait(prNumber int, timeout time.Duration) (PRMergeState, error) { + done := false + log := h.log.WithValues("number", prNumber) + wait := 10 * time.Second + for endTime := time.Now().Add(timeout); endTime.After(time.Now()) && !done; { + state := func() PRMergeState { + pr, err := h.FetchPR(prNumber) + if err != nil { + log.Error(err, "Failed to fetch PR; unable to confirm if its been merged") + return UnknownState + } + + if pr.State == MergeStateStatusMerged { + log.Info("PR has been merged", "pr", pr.URL) + return MergedState + } + if pr.IsInMergeQueue { + log.Info("PR is in merge queue", "pr", pr.URL) + return EnqueuedState + } + + log.Info("PR is not in merge queue; attempting to merge", "pr", pr.URL) + state, err := h.MergePR(pr.Number) + if err != nil { + log.Error(err, "Failed to merge pr", "number", pr.Number, "url", pr.URL) + } + return state + }() + + switch state { + case ClosedState: + fallthrough + case MergedState: + return state, nil + case EnqueuedState: + fallthrough + case UnknownState: + fallthrough + case BlockedState: + fallthrough + default: + if endTime.After(time.Now().Add(wait)) { + time.Sleep(wait) + } + } } - return nil + + return UnknownState, errors.Errorf("Timed out waiting for PR to merge") +} + +func (h *RepoHelper) FetchPR(prNumber int) (*api.PullRequest, error) { + // We need to set the appropriate header in oder to get merge queue status. + transport := &addAcceptHeaderTransport{T: h.transport} + client := &http.Client{Transport: transport} + fields := []string{"id", "number", "state", "title", "lastCommit", "mergeStateStatus", "headRepositoryOwner", "headRefName", "baseRefName", "headRefOid"} + return fetchPR(client, h.baseRepo, prNumber, fields) } func fetchPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.PullRequest, error) { diff --git a/pkg/gitops/syncer.go b/pkg/gitops/syncer.go index 389abcf..efd3909 100644 --- a/pkg/gitops/syncer.go +++ b/pkg/gitops/syncer.go @@ -301,14 +301,16 @@ func (s *Syncer) RunOnce(force bool) error { if existingPR != nil { log.Info("PR Already Exists; attempting to merge it.", "pr", existingPR.URL) - if err := s.repoHelper.MergePR(existingPR.Number); err != nil { - log.Error(err, "Failed to merge or enable automerge on pr", "number", existingPR.Number, "pr", existingPR.URL) + state, err := s.repoHelper.MergeAndWait(existingPR.Number, 3*time.Minute) + if err != nil { + log.Error(err, "Failed to Merge existing PR unable to continue with sync", "number", existingPR.Number, "pr", existingPR.URL) return err } - // TODO(jeremy): Ideally if the PR is merged immediately we should keep going with the sync. Since - // there could be new changes to sync. On the other hand if it was enqueued then we should return because - // we want to wait for it to merge. - return nil + + if state != github.ClosedState && state != github.MergedState { + log.Info("PR hasn't been merged; unable to continue with the sync", "number", existingPR.Number, "pr", existingPR.URL, "state", state) + return errors.Errorf("Existing PR %v is blocking sync", existingPR.URL) + } } if err := s.cloneRepos(); err != nil { @@ -552,10 +554,22 @@ func (s *Syncer) RunOnce(force bool) error { // EnableAutoMerge or merge the PR automatically. If you don't want the PR to be automerged you should // set up appropriate branch protections e.g. require approvers. - if err := s.repoHelper.MergePR(pr.Number); err != nil { - log.Error(err, "Failed to merge or enable automerge on pr", "number", pr.Number, "url", pr.URL) + // Wait up to 1 minute to try to merge the PR + // TODO(jeremy): This is mostly for dev takeover in the event we can't rely on automerge e.g. because + // we have a private repository for which we can't enable automerge. In this case we want to wait and + // retry the merge until its merged or we timeout. We may want to refactor this code so we can invoke the + // polling only in the case of being called with the takeover command. + // If the PR can't be merged does it make sense to report an error? in the case of long running tests + // The syncer can return and the PR will be merged either 1) when syncer is rerun or 2) by auto merge if enabled + // The desired behavior is potentially different in the takeover and non takeover setting. + state, err := s.repoHelper.MergeAndWait(pr.Number, 1*time.Minute) + if err != nil { + log.Error(err, "Failed to merge pr", "number", pr.Number, "url", pr.URL) return err } + if state != github.MergedState && state != github.ClosedState { + return fmt.Errorf("Failed to merge pr; state: %v", state) + } log.Info("Sync succeeded") return nil diff --git a/pkg/gitutil/gitutil.go b/pkg/gitutil/gitutil.go index e816ce3..a53e2d5 100644 --- a/pkg/gitutil/gitutil.go +++ b/pkg/gitutil/gitutil.go @@ -50,7 +50,7 @@ func AddGitignoreToWorktree(wt *git.Worktree, repositoryPath string) error { return nil } readFile, err := os.Open(path) - defer readFile.Close() + util.DeferIgnoreError(readFile.Close) if err != nil { return errors.Wrapf(err, "Failed to read .gitignore: %s", path)