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)