From 2f5da26ff2a271b26dd03e2d7b774b39703b9751 Mon Sep 17 00:00:00 2001 From: Kent Rancourt Date: Thu, 7 Nov 2024 07:55:43 -0500 Subject: [PATCH] fix(controller): fix pr already open issue with git-open-pr step (#2896) Signed-off-by: Kent Rancourt --- internal/controller/git/work_tree.go | 2 +- internal/controller/promotions/promotions.go | 5 +- internal/directives/git_pr_opener.go | 196 +++++++++++++++---- internal/directives/git_pr_opener_test.go | 71 ++++++- internal/directives/git_pr_waiter.go | 33 ++-- internal/directives/git_pr_waiter_test.go | 56 ++---- internal/gitprovider/github/github.go | 173 +++++++++------- internal/gitprovider/github/github_test.go | 2 +- internal/gitprovider/gitlab/gitlab.go | 180 +++++++++-------- internal/gitprovider/gitlab/gitlab_test.go | 64 ++---- internal/gitprovider/gitprovider.go | 151 ++++++++------ internal/gitprovider/registry.go | 46 +++-- 12 files changed, 587 insertions(+), 392 deletions(-) diff --git a/internal/controller/git/work_tree.go b/internal/controller/git/work_tree.go index 257e14649..9ff77ec43 100644 --- a/internal/controller/git/work_tree.go +++ b/internal/controller/git/work_tree.go @@ -262,7 +262,7 @@ func (w *workTree) CurrentBranch() (string, error) { if err != nil { return "", fmt.Errorf("error checking current branch for repo %q: %w", w.url, err) } - return string(res), nil + return strings.TrimSpace(string(res)), nil } func (w *workTree) DeleteBranch(branch string) error { diff --git a/internal/controller/promotions/promotions.go b/internal/controller/promotions/promotions.go index 407bb0d5f..ceb88e331 100644 --- a/internal/controller/promotions/promotions.go +++ b/internal/controller/promotions/promotions.go @@ -485,9 +485,10 @@ func (r *reconciler) promote( } if err := os.Mkdir(promoCtx.WorkDir, 0o700); err == nil { // If we're working with a fresh directory, we should start the promotion - // process again from the beginning. + // process again from the beginning, but we DON'T clear shared state. This + // allows individual steps to self-discover that they've run before and + // examine the results of their own previous execution. promoCtx.StartFromStep = 0 - promoCtx.State = nil } else if !os.IsExist(err) { return nil, fmt.Errorf("error creating working directory: %w", err) } diff --git a/internal/directives/git_pr_opener.go b/internal/directives/git_pr_opener.go index 47b8c6308..b386ed20c 100644 --- a/internal/directives/git_pr_opener.go +++ b/internal/directives/git_pr_opener.go @@ -3,7 +3,9 @@ package directives import ( "context" "fmt" + "slices" "strings" + "time" "github.com/xeipuuv/gojsonschema" @@ -70,7 +72,23 @@ func (g *gitPROpener) runPromotionStep( stepCtx *PromotionStepContext, cfg GitOpenPRConfig, ) (PromotionStepResult, error) { - sourceBranch, err := getSourceBranch(stepCtx.SharedState, cfg) + // Short-circuit if shared state has output from a previous execution of this + // step that contains a PR number. + prNumber, err := g.getPRNumber(stepCtx, stepCtx.SharedState) + if err != nil { + return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, + fmt.Errorf("error getting PR number from shared state: %w", err) + } + if prNumber != -1 { + return PromotionStepResult{ + Status: kargoapi.PromotionPhaseSucceeded, + Output: map[string]any{ + prNumberKey: prNumber, + }, + }, nil + } + + sourceBranch, err := g.getSourceBranch(stepCtx.SharedState, cfg) if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf("error determining source branch: %w", err) @@ -112,7 +130,7 @@ func (g *gitPROpener) runPromotionStep( } defer repo.Close() - gpOpts := &gitprovider.GitProviderOptions{ + gpOpts := &gitprovider.Options{ InsecureSkipTLSVerify: cfg.InsecureSkipTLSVerify, } if repoCreds != nil { @@ -121,28 +139,37 @@ func (g *gitPROpener) runPromotionStep( if cfg.Provider != nil { gpOpts.Name = string(*cfg.Provider) } - gitProviderSvc, err := gitprovider.NewGitProviderService(cfg.RepoURL, gpOpts) + gitProvider, err := gitprovider.New(cfg.RepoURL, gpOpts) if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf("error creating git provider service: %w", err) } - // Short-circuit if a pull request already exists with the same head commit - mustOpen, err := mustOpenPR( + // If a PR somehow exists that is identical to the one we would open, we can + // potentially just adopt it. + pr, err := g.getExistingPR( ctx, repo, - gitProviderSvc, - sourceBranch, + gitProvider, cfg.TargetBranch, ) if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, - fmt.Errorf("error determining if pull request must be opened: %w", err) + fmt.Errorf("error determining if pull request already exists: %w", err) } - if !mustOpen { - return PromotionStepResult{Status: kargoapi.PromotionPhaseSucceeded}, nil + if pr != nil && (pr.Open || pr.Merged) { // Excludes PR that is both closed AND unmerged + return PromotionStepResult{ + Status: kargoapi.PromotionPhaseSucceeded, + Output: map[string]any{ + prNumberKey: pr.Number, + }, + }, nil } + // If we get to here, we either did not find an existing PR like the one we're + // about to create, or we found one that is closed and not merged, which means + // we're free to create a new one. + // Get the title from the commit message of the head of the source branch // BEFORE we move on to ensuring the existence of the target branch because // that may involve creating a new branch and committing to it. @@ -154,7 +181,7 @@ func (g *gitPROpener) runPromotionStep( ) } - if err = ensureRemoteTargetBranch( + if err = g.ensureRemoteTargetBranch( repo, cfg.TargetBranch, cfg.CreateTargetBranch, @@ -177,16 +204,15 @@ func (g *gitPROpener) runPromotionStep( ) } - pr, err := gitProviderSvc.CreatePullRequest( + if pr, err = gitProvider.CreatePullRequest( ctx, - gitprovider.CreatePullRequestOpts{ + &gitprovider.CreatePullRequestOpts{ Head: sourceBranch, Base: cfg.TargetBranch, Title: title, Description: description, }, - ) - if err != nil { + ); err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf("error creating pull request: %w", err) } @@ -198,7 +224,49 @@ func (g *gitPROpener) runPromotionStep( }, nil } -func getSourceBranch(sharedState State, cfg GitOpenPRConfig) (string, error) { +// getPRNumber checks shared state for output from a previous execution of this +// step. If any is found and it contains a PR number, that number is returned. +// 0 is returned if no PR number is found in the shared state. An error is +// returned if the PR number is found but is neither an int64 nor a float64. +func (g *gitPROpener) getPRNumber( + stepCtx *PromotionStepContext, + sharedState State, +) (int64, error) { + stepOutput, exists := sharedState.Get(stepCtx.Alias) + if !exists { + return -1, nil + } + stepOutputMap, ok := stepOutput.(map[string]any) + if !ok { + return -1, fmt.Errorf( + "output from step with alias %q is not a map[string]any", + stepCtx.Alias, + ) + } + prNumberAny, exists := stepOutputMap[prNumberKey] + if !exists { + return -1, nil + } + // If the state was rehydrated from PromotionStatus, which makes use of + // apiextensions.JSON, the PR number will be a float64. Otherwise, it will be + // an int64. We need to handle both cases. + switch prNumber := prNumberAny.(type) { + case int64: + return prNumber, nil + case float64: + return int64(prNumber), nil + default: + return -1, fmt.Errorf( + "PR number in output from step with alias %q is not an int64", + stepCtx.Alias, + ) + } +} + +func (g *gitPROpener) getSourceBranch( + sharedState State, + cfg GitOpenPRConfig, +) (string, error) { sourceBranch := cfg.SourceBranch if cfg.SourceBranchFromStep != "" { stepOutput, exists := sharedState.Get(cfg.SourceBranchFromStep) @@ -235,7 +303,10 @@ func getSourceBranch(sharedState State, cfg GitOpenPRConfig) (string, error) { // ensureRemoteTargetBranch ensures the existence of a remote branch. If the // branch does not exist, an empty orphaned branch is created and pushed to the // remote. -func ensureRemoteTargetBranch(repo git.Repo, branch string, create bool) error { +func (g *gitPROpener) ensureRemoteTargetBranch( + repo git.Repo, + branch string, create bool, +) error { exists, err := repo.RemoteBranchExists(branch) if err != nil { return fmt.Errorf( @@ -275,38 +346,89 @@ func ensureRemoteTargetBranch(repo git.Repo, branch string, create bool) error { return nil } -// mustOpenPR determines if a pull request must be opened. It returns true if no -// PR exists for the head commit of the source branch to the target branch. -// Whether the PR is open or closed is irrelevant as we must NOT create a new PR -// if one already exists for the same head commit and has already been closed. -func mustOpenPR( +// getExistingPR searches for an existing pull request from the head of the +// repo's current branch to the target branch. If a PR is found, it is returned. +// If no PR is found, nil is returned. +func (g *gitPROpener) getExistingPR( ctx context.Context, repo git.Repo, - gitProviderSvc gitprovider.GitProviderService, - sourceBranch, + gitProv gitprovider.Interface, targetBranch string, -) (bool, error) { +) (*gitprovider.PullRequest, error) { commitID, err := repo.LastCommitID() if err != nil { - return false, fmt.Errorf("error getting last commit ID: %w", err) + return nil, fmt.Errorf("error getting last commit ID: %w", err) + } + sourceBranch, err := repo.CurrentBranch() + if err != nil { + return nil, fmt.Errorf("error getting current branch: %w", err) } - prs, err := gitProviderSvc.ListPullRequests( + // Find any existing PRs that are identical to the one we might open. + prs, err := gitProv.ListPullRequests( ctx, - gitprovider.ListPullRequestOpts{ - Base: targetBranch, - Head: sourceBranch, + &gitprovider.ListPullRequestOptions{ + BaseBranch: targetBranch, + HeadBranch: sourceBranch, + HeadCommit: commitID, }, ) if err != nil { - return false, fmt.Errorf("error listing pull requests: %w", err) + return nil, fmt.Errorf("error listing pull requests: %w", err) } if len(prs) == 0 { - return true, nil + return nil, nil } - for _, pr := range prs { - if pr.HeadSHA == commitID { - return false, nil + // If promotion names are incorporated into PR source branches, it's highly + // unlikely that we would have found more than one PR matching the search + // criteria. Accounting for the possibility of users specifying their own + // source branch names using an expression, although still unlikely, there is + // somewhat more of a possibility of multiple PRs being found. In this case, + // we need to determine which PR is best to "adopt" as a proxy for the PR we + // would have otherwise opened. This requires sorting the PRs in a particular + // order. + g.sortPullRequests(prs) + return &prs[0], nil +} + +// sortPullRequests is a specialized sorting function that sorts pull requests +// in the following order: open PRs first, then closed PRs that have been +// merged, then closed PRs that have not been merged. Within each of those +// categories, PRs are sorted by creation time in descending order. +func (g *gitPROpener) sortPullRequests(prs []gitprovider.PullRequest) { + slices.SortFunc(prs, func(lhs, rhs gitprovider.PullRequest) int { + switch { + case lhs.Open && !rhs.Open: + // If the first PR is open and the second is not, the first PR should + // come first. + return -1 + case rhs.Open && !lhs.Open: + // If the second PR is open and the first is not, the second PR should + // come first. + return 1 + case !lhs.Open && !rhs.Open: + // If both PRs are closed, one is merged and one is not, the merged PR + // should come first. + if lhs.Merged && !rhs.Merged { + return -1 + } + if rhs.Merged && !lhs.Merged { + return 1 + } + // If we get to here, both PRs are closed and neither is merged. Fall + // through to the default case. + fallthrough + default: + // If we get to here, both PRs are open or both are closed and neither is + // merged. The most recently opened PR should come first. + var ltime time.Time + if lhs.CreatedAt != nil { + ltime = *lhs.CreatedAt + } + var rtime time.Time + if rhs.CreatedAt != nil { + rtime = *rhs.CreatedAt + } + return rtime.Compare(ltime) } - } - return true, nil + }) } diff --git a/internal/directives/git_pr_opener_test.go b/internal/directives/git_pr_opener_test.go index 1e2a6e92a..00d0e511e 100644 --- a/internal/directives/git_pr_opener_test.go +++ b/internal/directives/git_pr_opener_test.go @@ -4,7 +4,9 @@ import ( "context" "fmt" "net/http/httptest" + "slices" "testing" + "time" "github.com/sosedoff/gitkit" "github.com/stretchr/testify/require" @@ -158,25 +160,25 @@ func Test_gitPROpener_runPromotionStep(t *testing.T) { // Set up a fake git provider const fakeGitProviderName = "fake" const testPRNumber int64 = 42 - gitprovider.RegisterProvider( + gitprovider.Register( fakeGitProviderName, - gitprovider.ProviderRegistration{ - NewService: func( + gitprovider.Registration{ + NewProvider: func( string, - *gitprovider.GitProviderOptions, - ) (gitprovider.GitProviderService, error) { - return &gitprovider.FakeGitProviderService{ + *gitprovider.Options, + ) (gitprovider.Interface, error) { + return &gitprovider.Fake{ ListPullRequestsFn: func( context.Context, - gitprovider.ListPullRequestOpts, - ) ([]*gitprovider.PullRequest, error) { + *gitprovider.ListPullRequestOptions, + ) ([]gitprovider.PullRequest, error) { // Avoid opening of a PR being short-circuited by simulating // conditions where the PR in question doesn't already exist. return nil, nil }, CreatePullRequestFn: func( context.Context, - gitprovider.CreatePullRequestOpts, + *gitprovider.CreatePullRequestOpts, ) (*gitprovider.PullRequest, error) { return &gitprovider.PullRequest{Number: testPRNumber}, nil }, @@ -223,3 +225,54 @@ func Test_gitPROpener_runPromotionStep(t *testing.T) { require.NoError(t, err) require.True(t, exists) } + +func Test_gitPROpener_sortPullRequests(t *testing.T) { + newer := time.Now() + older := newer.Add(-time.Hour) + // These are laid out in the exact opposite order of how they should be + // sorted. After sorting, we can assert the order is correct by comparing to + // the reversed list. + orig := []gitprovider.PullRequest{ + { + Number: 6, + Open: false, + Merged: false, + CreatedAt: &older, + }, + { + Number: 5, + Open: false, + Merged: false, + CreatedAt: &newer, + }, + { + Number: 4, + Open: false, + Merged: true, + CreatedAt: &older, + }, + { + Number: 3, + Open: false, + Merged: true, + CreatedAt: &newer, + }, + { + Number: 2, + Open: true, + Merged: false, + CreatedAt: &older, + }, + { + Number: 1, + Open: true, + Merged: false, + CreatedAt: &newer, + }, + } + sorted := make([]gitprovider.PullRequest, len(orig)) + copy(sorted, orig) + (&gitPROpener{}).sortPullRequests(sorted) + slices.Reverse(orig) + require.Equal(t, orig, sorted) +} diff --git a/internal/directives/git_pr_waiter.go b/internal/directives/git_pr_waiter.go index 40eb17bab..dc2c2b79f 100644 --- a/internal/directives/git_pr_waiter.go +++ b/internal/directives/git_pr_waiter.go @@ -64,10 +64,10 @@ func (g *gitPRWaiter) runPromotionStep( stepCtx *PromotionStepContext, cfg GitWaitForPRConfig, ) (PromotionStepResult, error) { - prNumber, err := getPRNumber(stepCtx.SharedState, cfg) + prNumber, err := g.getPRNumber(stepCtx.SharedState, cfg) if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, - fmt.Errorf("error getting PR number: %w", err) + fmt.Errorf("error getting PR number from shared state: %w", err) } var repoCreds *git.RepoCredentials @@ -89,7 +89,7 @@ func (g *gitPRWaiter) runPromotionStep( } } - gpOpts := &gitprovider.GitProviderOptions{ + gpOpts := &gitprovider.Options{ InsecureSkipTLSVerify: cfg.InsecureSkipTLSVerify, } if repoCreds != nil { @@ -98,42 +98,41 @@ func (g *gitPRWaiter) runPromotionStep( if cfg.Provider != nil { gpOpts.Name = string(*cfg.Provider) } - gitProviderSvc, err := gitprovider.NewGitProviderService(cfg.RepoURL, gpOpts) + gitProv, err := gitprovider.New(cfg.RepoURL, gpOpts) if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf("error creating git provider service: %w", err) } - pr, err := gitProviderSvc.GetPullRequest(ctx, prNumber) + pr, err := gitProv.GetPullRequest(ctx, prNumber) if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf("error getting pull request %d: %w", prNumber, err) } - if pr.IsOpen() { - return PromotionStepResult{Status: kargoapi.PromotionPhaseRunning}, nil - } - merged, err := gitProviderSvc.IsPullRequestMerged(ctx, prNumber) - if err != nil { - return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf( - "error checking if pull request %d was merged: %w", - prNumber, err, - ) + if pr.Open { + return PromotionStepResult{Status: kargoapi.PromotionPhaseRunning}, nil } - if !merged { + if !pr.Merged { return PromotionStepResult{ Status: kargoapi.PromotionPhaseFailed, Message: fmt.Sprintf("pull request %d was closed without being merged", prNumber), }, err } - return PromotionStepResult{ Status: kargoapi.PromotionPhaseSucceeded, Output: map[string]any{commitKey: pr.MergeCommitSHA}, }, nil } -func getPRNumber(sharedState State, cfg GitWaitForPRConfig) (int64, error) { +// getPRNumber checks shared state for output from a previous step and returns +// any PR number from that output. If no such output is found, the output +// contains no PR number, or the PR number is not an int64 or float64, then an +// error is returned. +func (g *gitPRWaiter) getPRNumber( + sharedState State, + cfg GitWaitForPRConfig, +) (int64, error) { if cfg.PRNumberFromStep == "" { return cfg.PRNumber, nil } diff --git a/internal/directives/git_pr_waiter_test.go b/internal/directives/git_pr_waiter_test.go index 721ff501c..e4610f374 100644 --- a/internal/directives/git_pr_waiter_test.go +++ b/internal/directives/git_pr_waiter_test.go @@ -100,12 +100,12 @@ func Test_gitPRWaiter_validate(t *testing.T) { func Test_gitPRWaiter_runPromotionStep(t *testing.T) { testCases := []struct { name string - provider gitprovider.GitProviderService + provider gitprovider.Interface assertions func(*testing.T, PromotionStepResult, error) }{ { name: "error finding PR", - provider: &gitprovider.FakeGitProviderService{ + provider: &gitprovider.Fake{ GetPullRequestFn: func( context.Context, int64, @@ -121,13 +121,13 @@ func Test_gitPRWaiter_runPromotionStep(t *testing.T) { }, { name: "PR is open", - provider: &gitprovider.FakeGitProviderService{ + provider: &gitprovider.Fake{ GetPullRequestFn: func( context.Context, int64, ) (*gitprovider.PullRequest, error) { return &gitprovider.PullRequest{ - State: gitprovider.PullRequestStateOpen, + Open: true, }, nil }, }, @@ -136,42 +136,18 @@ func Test_gitPRWaiter_runPromotionStep(t *testing.T) { require.Equal(t, kargoapi.PromotionPhaseRunning, res.Status) }, }, - { - name: "error checking if PR was merged", - provider: &gitprovider.FakeGitProviderService{ - GetPullRequestFn: func( - context.Context, - int64, - ) (*gitprovider.PullRequest, error) { - return &gitprovider.PullRequest{ - State: gitprovider.PullRequestStateClosed, - }, nil - }, - IsPullRequestMergedFn: func(context.Context, int64) (bool, error) { - return false, errors.New("something went wrong") - }, - }, - assertions: func(t *testing.T, res PromotionStepResult, err error) { - require.ErrorContains(t, err, "error checking if pull request") - require.ErrorContains(t, err, "was merged") - require.ErrorContains(t, err, "something went wrong") - require.Equal(t, kargoapi.PromotionPhaseErrored, res.Status) - }, - }, { name: "PR is closed and not merged", - provider: &gitprovider.FakeGitProviderService{ + provider: &gitprovider.Fake{ GetPullRequestFn: func( context.Context, int64, ) (*gitprovider.PullRequest, error) { return &gitprovider.PullRequest{ - State: gitprovider.PullRequestStateClosed, + Open: false, + Merged: false, }, nil }, - IsPullRequestMergedFn: func(context.Context, int64) (bool, error) { - return false, nil - }, }, assertions: func(t *testing.T, res PromotionStepResult, err error) { require.NoError(t, err) @@ -181,18 +157,16 @@ func Test_gitPRWaiter_runPromotionStep(t *testing.T) { }, { name: "PR is closed and merged", - provider: &gitprovider.FakeGitProviderService{ + provider: &gitprovider.Fake{ GetPullRequestFn: func( context.Context, int64, ) (*gitprovider.PullRequest, error) { return &gitprovider.PullRequest{ - State: gitprovider.PullRequestStateClosed, + Open: false, + Merged: true, }, nil }, - IsPullRequestMergedFn: func(context.Context, int64) (bool, error) { - return true, nil - }, }, assertions: func(t *testing.T, res PromotionStepResult, err error) { require.NoError(t, err) @@ -211,13 +185,13 @@ func Test_gitPRWaiter_runPromotionStep(t *testing.T) { // care of that problem testGitProviderName := uuid.NewString() - gitprovider.RegisterProvider( + gitprovider.Register( testGitProviderName, - gitprovider.ProviderRegistration{ - NewService: func( + gitprovider.Registration{ + NewProvider: func( string, - *gitprovider.GitProviderOptions, - ) (gitprovider.GitProviderService, error) { + *gitprovider.Options, + ) (gitprovider.Interface, error) { return testCase.provider, nil }, }, diff --git a/internal/gitprovider/github/github.go b/internal/gitprovider/github/github.go index 5df7461da..7964027e4 100644 --- a/internal/gitprovider/github/github.go +++ b/internal/gitprovider/github/github.go @@ -15,50 +15,48 @@ import ( "github.com/akuity/kargo/internal/gitprovider" ) -const ( - GitProviderServiceName = "github" -) +const ProviderName = "github" -var ( - githubRegistration = gitprovider.ProviderRegistration{ - Predicate: func(repoURL string) bool { - u, err := url.Parse(repoURL) - if err != nil { - return false - } - // We assume that any hostname with the word "github" in the hostname, - // can use this provider. NOTE: we will miss cases where the host is - // GitHub but doesn't incorporate the word "github" in the hostname. - // e.g. 'git.mycompany.com' - return strings.Contains(u.Host, GitProviderServiceName) - }, - NewService: func( - repoURL string, - opts *gitprovider.GitProviderOptions, - ) (gitprovider.GitProviderService, error) { - return NewGitHubProvider(repoURL, opts) - }, - } -) +var registration = gitprovider.Registration{ + Predicate: func(repoURL string) bool { + u, err := url.Parse(repoURL) + if err != nil { + return false + } + // We assume that any hostname with the word "github" in it can use this + // provider. NOTE: We will miss cases where the host is GitHub Enterprise + // but doesn't incorporate the word "github" in the hostname. e.g. + // 'git.mycompany.com' + return strings.Contains(u.Host, ProviderName) + }, + NewProvider: func( + repoURL string, + opts *gitprovider.Options, + ) (gitprovider.Interface, error) { + return NewProvider(repoURL, opts) + }, +} func init() { - gitprovider.RegisterProvider(GitProviderServiceName, githubRegistration) + gitprovider.Register(ProviderName, registration) } -type GitHubProvider struct { // nolint: revive +// provider is a GitHub implementation of gitprovider.Interface. +type provider struct { // nolint: revive owner string repo string client *github.Client } -func NewGitHubProvider( +// NewProvider returns a GitHub-based implementation of gitprovider.Interface. +func NewProvider( repoURL string, - opts *gitprovider.GitProviderOptions, -) (gitprovider.GitProviderService, error) { + opts *gitprovider.Options, +) (gitprovider.Interface, error) { if opts == nil { - opts = &gitprovider.GitProviderOptions{} + opts = &gitprovider.Options{} } - host, owner, repo, err := parseGitHubURL(repoURL) + host, owner, repo, err := parseRepoURL(repoURL) if err != nil { return nil, err } @@ -80,20 +78,24 @@ func NewGitHubProvider( if opts.Token != "" { client = client.WithAuthToken(opts.Token) } - return &GitHubProvider{ + return &provider{ owner: owner, repo: repo, client: client, }, nil } -func (g *GitHubProvider) CreatePullRequest( +// CreatePullRequest implements gitprovider.Interface. +func (p *provider) CreatePullRequest( ctx context.Context, - opts gitprovider.CreatePullRequestOpts, + opts *gitprovider.CreatePullRequestOpts, ) (*gitprovider.PullRequest, error) { - ghPR, _, err := g.client.PullRequests.Create(ctx, - g.owner, - g.repo, + if opts == nil { + opts = &gitprovider.CreatePullRequestOpts{} + } + ghPR, _, err := p.client.PullRequests.Create(ctx, + p.owner, + p.repo, &github.NewPullRequest{ Title: &opts.Title, Head: &opts.Head, @@ -105,64 +107,94 @@ func (g *GitHubProvider) CreatePullRequest( if err != nil { return nil, err } - return convertGithubPR(ghPR), nil + if ghPR == nil { + return nil, fmt.Errorf("unexpected nil pull request") + } + pr := convertGithubPR(*ghPR) + return &pr, nil } -func (g *GitHubProvider) GetPullRequest( +// GetPullRequest implements gitprovider.Interface. +func (p *provider) GetPullRequest( ctx context.Context, id int64, ) (*gitprovider.PullRequest, error) { - ghPR, _, err := g.client.PullRequests.Get(ctx, g.owner, g.repo, int(id)) + ghPR, _, err := p.client.PullRequests.Get(ctx, p.owner, p.repo, int(id)) if err != nil { return nil, err } - return convertGithubPR(ghPR), nil + if ghPR == nil { + return nil, fmt.Errorf("unexpected nil pull request") + } + pr := convertGithubPR(*ghPR) + return &pr, nil } -func (g *GitHubProvider) ListPullRequests( +// ListPullRequests implements gitprovider.Interface. +func (p *provider) ListPullRequests( ctx context.Context, - opts gitprovider.ListPullRequestOpts, -) ([]*gitprovider.PullRequest, error) { + opts *gitprovider.ListPullRequestOptions, +) ([]gitprovider.PullRequest, error) { + if opts == nil { + opts = &gitprovider.ListPullRequestOptions{} + } + if opts.State == "" { + opts.State = gitprovider.PullRequestStateOpen + } listOpts := github.PullRequestListOptions{ - Head: opts.Head, - Base: opts.Base, + Head: opts.HeadBranch, + Base: opts.BaseBranch, + ListOptions: github.ListOptions{ + PerPage: 100, // Max + }, } switch opts.State { - case "", gitprovider.PullRequestStateOpen: - listOpts.State = "open" + case gitprovider.PullRequestStateAny: + listOpts.State = "all" case gitprovider.PullRequestStateClosed: listOpts.State = "closed" + case gitprovider.PullRequestStateOpen: + listOpts.State = "open" + default: + return nil, fmt.Errorf("unknown pull request state %q", opts.State) } - ghPRs, _, err := g.client.PullRequests.List(ctx, g.owner, g.repo, &listOpts) - if err != nil { - return nil, err - } - prs := make([]*gitprovider.PullRequest, len(ghPRs)) - for i, ghPR := range ghPRs { - prs[i] = convertGithubPR(ghPR) + prs := []gitprovider.PullRequest{} + for { + ghPRs, res, err := p.client.PullRequests.List(ctx, p.owner, p.repo, &listOpts) + if err != nil { + return nil, err + } + for _, ghPR := range ghPRs { + if opts.HeadCommit == "" || ptr.Deref(ghPR.Head.SHA, "") == opts.HeadCommit { + prs = append(prs, convertGithubPR(*ghPR)) + } + } + if res == nil || res.NextPage == 0 { + break + } + listOpts.Page = res.NextPage } + return prs, nil } -func convertGithubPR(ghPR *github.PullRequest) *gitprovider.PullRequest { - var prState gitprovider.PullRequestState - switch ptr.Deref(ghPR.State, "") { - case "open": - prState = gitprovider.PullRequestStateOpen - case "closed": - prState = gitprovider.PullRequestStateClosed - } - return &gitprovider.PullRequest{ +func convertGithubPR(ghPR github.PullRequest) gitprovider.PullRequest { + pr := gitprovider.PullRequest{ Number: int64(ptr.Deref(ghPR.Number, 0)), URL: ptr.Deref(ghPR.HTMLURL, ""), - State: prState, + Open: ptr.Deref(ghPR.State, "closed") == "open", + Merged: ghPR.MergedAt != nil, MergeCommitSHA: ptr.Deref(ghPR.MergeCommitSHA, ""), Object: ghPR, HeadSHA: ptr.Deref(ghPR.Head.SHA, ""), } + if ghPR.CreatedAt != nil { + pr.CreatedAt = &ghPR.CreatedAt.Time + } + return pr } -func parseGitHubURL(repoURL string) (string, string, string, error) { +func parseRepoURL(repoURL string) (string, string, string, error) { u, err := url.Parse(git.NormalizeURL(repoURL)) if err != nil { return "", "", "", fmt.Errorf("error parsing github repository URL %q: %w", u, err) @@ -174,12 +206,3 @@ func parseGitHubURL(repoURL string) (string, string, string, error) { } return u.Host, parts[0], parts[1], nil } - -func (g *GitHubProvider) IsPullRequestMerged(ctx context.Context, id int64) (bool, error) { - // https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#check-if-a-pull-request-has-been-merged - merged, _, err := g.client.PullRequests.IsMerged(ctx, g.owner, g.repo, int(id)) - if err != nil { - return false, err - } - return merged, nil -} diff --git a/internal/gitprovider/github/github_test.go b/internal/gitprovider/github/github_test.go index ddc88108c..f35753b90 100644 --- a/internal/gitprovider/github/github_test.go +++ b/internal/gitprovider/github/github_test.go @@ -45,7 +45,7 @@ func TestParseGitHubURL(t *testing.T) { } for _, testCase := range testCases { t.Run(testCase.url, func(t *testing.T) { - host, owner, repo, err := parseGitHubURL(testCase.url) + host, owner, repo, err := parseRepoURL(testCase.url) if testCase.errExpected { require.Error(t, err) } else { diff --git a/internal/gitprovider/gitlab/gitlab.go b/internal/gitprovider/gitlab/gitlab.go index f4f6f9788..ee52e2919 100644 --- a/internal/gitprovider/gitlab/gitlab.go +++ b/internal/gitprovider/gitlab/gitlab.go @@ -14,34 +14,30 @@ import ( "github.com/akuity/kargo/internal/gitprovider" ) -const ( - GitProviderServiceName = "gitlab" -) +const ProviderName = "gitlab" -var ( - registration = gitprovider.ProviderRegistration{ - Predicate: func(repoURL string) bool { - u, err := url.Parse(repoURL) - if err != nil { - return false - } - // We assume that any hostname with the word "gitlab" in the hostname, - // can use this provider. NOTE: we will miss cases where the host is - // Gitlab but doesn't incorporate the word "gitlab" in the hostname. - // e.g. 'git.mycompany.com' - return strings.Contains(u.Host, GitProviderServiceName) - }, - NewService: func( - repoURL string, - opts *gitprovider.GitProviderOptions, - ) (gitprovider.GitProviderService, error) { - return NewGitLabProvider(repoURL, opts) - }, - } -) +var registration = gitprovider.Registration{ + Predicate: func(repoURL string) bool { + u, err := url.Parse(repoURL) + if err != nil { + return false + } + // We assume that any hostname with the word "gitlab" in it, can use this + // provider. NOTE: We will miss cases where the host is self-hosted Gitlab + // but doesn't incorporate the word "gitlab" in the hostname. e.g. + // 'git.mycompany.com' + return strings.Contains(u.Host, ProviderName) + }, + NewProvider: func( + repoURL string, + opts *gitprovider.Options, + ) (gitprovider.Interface, error) { + return NewProvider(repoURL, opts) + }, +} func init() { - gitprovider.RegisterProvider(GitProviderServiceName, registration) + gitprovider.Register(ProviderName, registration) } type mergeRequestClient interface { @@ -65,23 +61,21 @@ type mergeRequestClient interface { ) (*gitlab.MergeRequest, *gitlab.Response, error) } -type gitLabClient struct { // nolint: revive - mergeRequests mergeRequestClient -} - -type gitLabProvider struct { // nolint: revive +// provider is a GitLab-based implementation of gitprovider.Interface. +type provider struct { // nolint: revive projectName string - client *gitLabClient + client mergeRequestClient } -func NewGitLabProvider( +// NewProvider returns a GitLab-based implementation of gitprovider.Interface. +func NewProvider( repoURL string, - opts *gitprovider.GitProviderOptions, -) (gitprovider.GitProviderService, error) { + opts *gitprovider.Options, +) (gitprovider.Interface, error) { if opts == nil { - opts = &gitprovider.GitProviderOptions{} + opts = &gitprovider.Options{} } - host, projectName, err := parseGitLabURL(repoURL) + host, projectName, err := parseRepoURL(repoURL) if err != nil { return nil, err } @@ -108,17 +102,21 @@ func NewGitLabProvider( if err != nil { return nil, err } - return &gitLabProvider{ + return &provider{ projectName: projectName, - client: &gitLabClient{mergeRequests: client.MergeRequests}, + client: client.MergeRequests, }, nil } -func (g *gitLabProvider) CreatePullRequest( +// CreatePullRequest implements gitprovider.Interface. +func (p *provider) CreatePullRequest( _ context.Context, - opts gitprovider.CreatePullRequestOpts, + opts *gitprovider.CreatePullRequestOpts, ) (*gitprovider.PullRequest, error) { - glMR, _, err := g.client.mergeRequests.CreateMergeRequest(g.projectName, &gitlab.CreateMergeRequestOptions{ + if opts == nil { + opts = &gitprovider.CreatePullRequestOpts{} + } + glMR, _, err := p.client.CreateMergeRequest(p.projectName, &gitlab.CreateMergeRequestOptions{ Title: &opts.Title, Description: &opts.Description, SourceBranch: &opts.Head, @@ -128,76 +126,94 @@ func (g *gitLabProvider) CreatePullRequest( if err != nil { return nil, err } - return convertGitlabMR(glMR), nil + if glMR == nil { + return nil, fmt.Errorf("unexpected nil merge request") + } + pr := convertGitlabMR(*glMR) + return &pr, nil } -func (g *gitLabProvider) GetPullRequest( +// GetPullRequest implements gitprovider.Interface. +func (p *provider) GetPullRequest( _ context.Context, id int64, ) (*gitprovider.PullRequest, error) { - glMR, err := g.getMergeRequest(id) + glMR, _, err := p.client.GetMergeRequest(p.projectName, int(id), nil) if err != nil { return nil, err } - return convertGitlabMR(glMR), nil + if glMR == nil { + return nil, fmt.Errorf("unexpected nil merge request") + } + pr := convertGitlabMR(*glMR) + return &pr, nil } -func (g *gitLabProvider) ListPullRequests( +// ListPullRequests implements gitprovider.Interface. +func (p *provider) ListPullRequests( _ context.Context, - opts gitprovider.ListPullRequestOpts, -) ([]*gitprovider.PullRequest, error) { - listOpts := &gitlab.ListProjectMergeRequestsOptions{ - SourceBranch: &opts.Head, - TargetBranch: &opts.Base, + opts *gitprovider.ListPullRequestOptions, +) ([]gitprovider.PullRequest, error) { + if opts == nil { + opts = &gitprovider.ListPullRequestOptions{} } - glMRs, _, err := g.client.mergeRequests.ListProjectMergeRequests(g.projectName, listOpts) - if err != nil { - return nil, err + if opts.State == "" { + opts.State = gitprovider.PullRequestStateOpen + } + switch opts.State { + case gitprovider.PullRequestStateAny, gitprovider.PullRequestStateClosed, + gitprovider.PullRequestStateOpen: + default: + return nil, fmt.Errorf("unknown pull request state %q", opts.State) + } + listOpts := &gitlab.ListProjectMergeRequestsOptions{ + SourceBranch: &opts.HeadBranch, + TargetBranch: &opts.BaseBranch, + ListOptions: gitlab.ListOptions{ + // The max isn't documented, but this doesn't produce an error. + PerPage: 100, + }, } - prs := make([]*gitprovider.PullRequest, 0, len(glMRs)) - for _, glMR := range glMRs { - if (opts.State == gitprovider.PullRequestStateOpen) == isMROpen(glMR) { - prs = append(prs, convertGitlabMR(glMR)) + prs := []gitprovider.PullRequest{} + for { + glMRs, res, err := p.client.ListProjectMergeRequests(p.projectName, listOpts) + if err != nil { + return nil, err + } + for _, glMR := range glMRs { + if (opts.State == gitprovider.PullRequestStateAny || + ((opts.State == gitprovider.PullRequestStateOpen) == isMROpen(*glMR))) && + (opts.HeadCommit == "" || glMR.SHA == opts.HeadCommit) { + prs = append(prs, convertGitlabMR(*glMR)) + } } + if res == nil || res.NextPage == 0 { + break + } + listOpts.Page = res.NextPage } return prs, nil } -func (g *gitLabProvider) IsPullRequestMerged(_ context.Context, id int64) (bool, error) { - glMR, err := g.getMergeRequest(id) - if err != nil { - return false, err - } - return glMR.State == "merged", nil -} - -func convertGitlabMR(glMR *gitlab.MergeRequest) *gitprovider.PullRequest { - var prState gitprovider.PullRequestState - if isMROpen(glMR) { - prState = gitprovider.PullRequestStateOpen - } else { - prState = gitprovider.PullRequestStateClosed - } - return &gitprovider.PullRequest{ +func convertGitlabMR(glMR gitlab.MergeRequest) gitprovider.PullRequest { + fmt.Println(glMR.MergeCommitSHA) + return gitprovider.PullRequest{ Number: int64(glMR.IID), URL: glMR.WebURL, - State: prState, + Open: isMROpen(glMR), + Merged: glMR.State == "merged", MergeCommitSHA: glMR.MergeCommitSHA, Object: glMR, HeadSHA: glMR.SHA, + CreatedAt: glMR.CreatedAt, } } -func isMROpen(glMR *gitlab.MergeRequest) bool { +func isMROpen(glMR gitlab.MergeRequest) bool { return glMR.State == "opened" || glMR.State == "locked" } -func (g *gitLabProvider) getMergeRequest(id int64) (*gitlab.MergeRequest, error) { - glMR, _, err := g.client.mergeRequests.GetMergeRequest(g.projectName, int(id), nil) - return glMR, err -} - -func parseGitLabURL(repoURL string) (string, string, error) { +func parseRepoURL(repoURL string) (string, string, error) { u, err := url.Parse(git.NormalizeURL(repoURL)) if err != nil { return "", "", fmt.Errorf("error parsing gitlab repository URL %q: %w", u, err) diff --git a/internal/gitprovider/gitlab/gitlab_test.go b/internal/gitprovider/gitlab/gitlab_test.go index 670aa9d06..fc95d8a53 100644 --- a/internal/gitprovider/gitlab/gitlab_test.go +++ b/internal/gitprovider/gitlab/gitlab_test.go @@ -58,11 +58,9 @@ func TestCreatePullRequest(t *testing.T) { WebURL: "url", }, } - g := gitLabProvider{ + g := provider{ projectName: testProjectName, - client: &gitLabClient{ - mergeRequests: mockClient, - }, + client: mockClient, } opts := gitprovider.CreatePullRequestOpts{ @@ -71,7 +69,7 @@ func TestCreatePullRequest(t *testing.T) { Title: "title", Description: "desc", } - pr, err := g.CreatePullRequest(context.Background(), opts) + pr, err := g.CreatePullRequest(context.Background(), &opts) require.NoError(t, err) require.Equal(t, testProjectName, mockClient.pid) @@ -83,7 +81,7 @@ func TestCreatePullRequest(t *testing.T) { require.Equal(t, int64(mockClient.mr.IID), pr.Number) require.Equal(t, mockClient.mr.MergeCommitSHA, pr.MergeCommitSHA) require.Equal(t, mockClient.mr.WebURL, pr.URL) - require.Equal(t, gitprovider.PullRequestStateClosed, pr.State) + require.False(t, pr.Open) } func TestGetPullRequest(t *testing.T) { @@ -95,11 +93,9 @@ func TestGetPullRequest(t *testing.T) { WebURL: "url", }, } - g := gitLabProvider{ + g := provider{ projectName: testProjectName, - client: &gitLabClient{ - mergeRequests: mockClient, - }, + client: mockClient, } pr, err := g.GetPullRequest(context.Background(), 1) @@ -109,7 +105,7 @@ func TestGetPullRequest(t *testing.T) { require.Equal(t, int64(mockClient.mr.IID), pr.Number) require.Equal(t, mockClient.mr.MergeCommitSHA, pr.MergeCommitSHA) require.Equal(t, mockClient.mr.WebURL, pr.URL) - require.Equal(t, gitprovider.PullRequestStateClosed, pr.State) + require.False(t, pr.Open) } func TestListPullRequests(t *testing.T) { @@ -121,49 +117,27 @@ func TestListPullRequests(t *testing.T) { WebURL: "url", }, } - g := gitLabProvider{ + g := provider{ projectName: testProjectName, - client: &gitLabClient{ - mergeRequests: mockClient, - }, + client: mockClient, } - opts := gitprovider.ListPullRequestOpts{ - Head: "head", - Base: "base", + opts := gitprovider.ListPullRequestOptions{ + State: gitprovider.PullRequestStateAny, + HeadBranch: "head", + BaseBranch: "base", } - prs, err := g.ListPullRequests(context.Background(), opts) - + prs, err := g.ListPullRequests(context.Background(), &opts) require.NoError(t, err) + require.Equal(t, testProjectName, mockClient.pid) - require.Equal(t, opts.Head, *mockClient.listOpts.SourceBranch) - require.Equal(t, opts.Base, *mockClient.listOpts.TargetBranch) + require.Equal(t, opts.HeadBranch, *mockClient.listOpts.SourceBranch) + require.Equal(t, opts.BaseBranch, *mockClient.listOpts.TargetBranch) require.Equal(t, int64(mockClient.mr.IID), prs[0].Number) require.Equal(t, mockClient.mr.MergeCommitSHA, prs[0].MergeCommitSHA) require.Equal(t, mockClient.mr.WebURL, prs[0].URL) - require.Equal(t, gitprovider.PullRequestStateClosed, prs[0].State) -} - -func TestIsPullRequestMerged(t *testing.T) { - require.True(t, isPullRequestMerged("merged")) - require.False(t, isPullRequestMerged("closed")) - require.False(t, isPullRequestMerged("locked")) - require.False(t, isPullRequestMerged("opened")) -} - -func isPullRequestMerged(state string) bool { - mockClient := &mockGitLabClient{ - mr: &gitlab.MergeRequest{ - IID: 1, - MergeCommitSHA: "sha", - State: state, - WebURL: "url", - }, - } - g := gitLabProvider{client: &gitLabClient{mergeRequests: mockClient}} - res, _ := g.IsPullRequestMerged(context.Background(), 1) - return res + require.False(t, prs[0].Open) } func TestParseGitLabURL(t *testing.T) { @@ -189,7 +163,7 @@ func TestParseGitLabURL(t *testing.T) { } for _, testCase := range testCases { t.Run(testCase.url, func(t *testing.T) { - host, projectName, err := parseGitLabURL(testCase.url) + host, projectName, err := parseRepoURL(testCase.url) require.NoError(t, err) require.Equal(t, testCase.expectedHost, host) require.Equal(t, expectedProjectName, projectName) diff --git a/internal/gitprovider/gitprovider.go b/internal/gitprovider/gitprovider.go index cc61d8976..c83dd8091 100644 --- a/internal/gitprovider/gitprovider.go +++ b/internal/gitprovider/gitprovider.go @@ -2,10 +2,29 @@ package gitprovider import ( "context" + "time" ) -// GitProviderOptions contains the options for a GitProvider. -type GitProviderOptions struct { // nolint: revive +// PullRequestState represents the state of a pull request. e.g. Closed, Open, +// etc. +type PullRequestState string + +const ( + // PullRequestStateAny represents all pull requests, regardless of state. + PullRequestStateAny PullRequestState = "Any" + // PullRequestStateClosed represents pull requests that are (logically) + // closed. Depending on the underlying provider, this may encompass pull + // requests in other states such as "merged" or "declined". + PullRequestStateClosed PullRequestState = "Closed" + // PullRequestStateOpen represents pull requests that are (logically) open. + // Depending on the underlying provider, this may encompass pull requests in + // other states such as "draft" or "ready for review". + PullRequestStateOpen PullRequestState = "Open" +) + +// Options encapsulates options used in instantiating any implementation +// of Interface. +type Options struct { // Name specifies which Git provider to use when that information cannot be // inferred from the repository URL. Name string @@ -17,100 +36,116 @@ type GitProviderOptions struct { // nolint: revive InsecureSkipTLSVerify bool } -// GitProviderService is an abstracted interface for a git providers (GitHub, GitLab, BitBucket) -// when interacting against a single git repository (e.g. managing pull requests). -type GitProviderService interface { // nolint: revive - // CreatePullRequest creates a pull request - CreatePullRequest(ctx context.Context, opts CreatePullRequestOpts) (*PullRequest, error) +// Interface is an abstracted interface for interacting with a single repository +// hosted by some a Git hosting provider (e.g. GitHub, GitLab, BitBucket, +// etc.). +type Interface interface { + // CreatePullRequest creates a pull request. + CreatePullRequest(context.Context, *CreatePullRequestOpts) (*PullRequest, error) // Get gets an existing pull request by ID - GetPullRequest(ctx context.Context, number int64) (*PullRequest, error) - - // ListPullRequests lists pull requests by the given options - ListPullRequests(ctx context.Context, opts ListPullRequestOpts) ([]*PullRequest, error) + GetPullRequest(context.Context, int64) (*PullRequest, error) - // IsPullRequestMerged returns whether or not the pull request was merged - IsPullRequestMerged(ctx context.Context, number int64) (bool, error) + // ListPullRequests lists pull requests by the given options. Implementations + // have no obligation to sort the results in any particular order, due mainly + // to differences in the underlying provider APIs. It is the responsibility of + // the caller to sort the results as needed. + ListPullRequests(context.Context, *ListPullRequestOptions) ([]PullRequest, error) } +// CreatePullRequestOpts encapsulates the options used when creating a pull +// request. type CreatePullRequestOpts struct { - Head string - Base string - Title string + // Title is the title of the pull request. + Title string + // Description is the body of the pull request. Description string + // Head is the name of the source branch. + Head string + // Base is the name of the target branch. + Base string } -type ListPullRequestOpts struct { - // State is the pull request state (one of: Open, Closed). Defaults to Open +// ListPullRequestOptions encapsulates the options used when listing pull +// requests. +type ListPullRequestOptions struct { + // State is the pull request state (one of: Any, Closed, or Open). State PullRequestState - Head string - Base string + // HeadBranch is the name of the source branch. A non-empty value will limit + // results to pull requests with the given source branch. + HeadBranch string + // HeadCommit is the SHA of the commit at the head of the source branch. A + // non-empty value will limit results to pull requests with the given head + // commit. + HeadCommit string + // BaseBranch is the name of the target branch. A non-empty value will limit + // results to pull requests with the given target branch. + BaseBranch string } -type PullRequestState string - -const ( - PullRequestStateOpen PullRequestState = "Open" - PullRequestStateClosed PullRequestState = "Closed" -) - +// PullRequest is an abstracted representation of a Git hosting provider's pull +// request object (or equivalent; e.g. a GitLab merge request). type PullRequest struct { - // Number is the numeric pull request number (not an ID) - // Pull requests numbers are unique only within a repository + // Number is the pull request number, which is unique only within a single + // repository. Number int64 `json:"id"` - // URL is the url to the pull request + // URL is the URL to the pull request. URL string `json:"url"` - // State is the pull request state (one of: Open, Closed) - State PullRequestState `json:"state"` - // MergeCommitSHA is the SHA of the merge commit + // Open is true if the pull request is logically open. Depending on the + // underlying Git hosting provider, this may encompass pull requests in other + // states such as "draft" or "ready for review". + Open bool `json:"open"` + // Merged is true if the pull request was merged. + Merged bool `json:"merged"` + // MergeCommitSHA is the SHA of the merge commit. MergeCommitSHA string `json:"mergeCommitSHA"` - // Object is the underlying object from the provider + // Object is the underlying object from the Git hosting provider. Object any `json:"-"` - // HeadSHA is the SHA of the head commit + // HeadSHA is the SHA of the commit at the head of the source branch. HeadSHA string `json:"headSHA"` + // CreatedAt is the time the pull request was created. + CreatedAt *time.Time `json:"createdAt"` } -func (pr *PullRequest) IsOpen() bool { - return pr.State == PullRequestStateOpen -} - -type FakeGitProviderService struct { +// Fake is a fake implementation of the provider Interface used to facilitate +// testing. +type Fake struct { + // CreatePullRequestFn defines the functionality of the CreatePullRequest + // method. CreatePullRequestFn func( context.Context, - CreatePullRequestOpts, + *CreatePullRequestOpts, ) (*PullRequest, error) - GetPullRequestFn func(context.Context, int64) (*PullRequest, error) + // GetPullRequestFn defines the functionality of the GetPullRequest method. + GetPullRequestFn func(context.Context, int64) (*PullRequest, error) + // ListPullRequestsFn defines the functionality of the ListPullRequests + // method. ListPullRequestsFn func( context.Context, - ListPullRequestOpts, - ) ([]*PullRequest, error) - IsPullRequestMergedFn func(context.Context, int64) (bool, error) + *ListPullRequestOptions, + ) ([]PullRequest, error) } -func (f *FakeGitProviderService) CreatePullRequest( +// CreatePullRequest implements gitprovider.Interface. +func (f *Fake) CreatePullRequest( ctx context.Context, - opts CreatePullRequestOpts, + opts *CreatePullRequestOpts, ) (*PullRequest, error) { return f.CreatePullRequestFn(ctx, opts) } -func (f *FakeGitProviderService) GetPullRequest( +// GetPullRequest implements gitprovider.Interface. +func (f *Fake) GetPullRequest( ctx context.Context, number int64, ) (*PullRequest, error) { return f.GetPullRequestFn(ctx, number) } -func (f *FakeGitProviderService) ListPullRequests( +// ListPullRequests implements gitprovider.Interface. +func (f *Fake) ListPullRequests( ctx context.Context, - opts ListPullRequestOpts, -) ([]*PullRequest, error) { + opts *ListPullRequestOptions, +) ([]PullRequest, error) { return f.ListPullRequestsFn(ctx, opts) } - -func (f *FakeGitProviderService) IsPullRequestMerged( - ctx context.Context, - number int64, -) (bool, error) { - return f.IsPullRequestMergedFn(ctx, number) -} diff --git a/internal/gitprovider/registry.go b/internal/gitprovider/registry.go index 116fe68a1..b1f89d86e 100644 --- a/internal/gitprovider/registry.go +++ b/internal/gitprovider/registry.go @@ -1,48 +1,46 @@ package gitprovider -import ( - "fmt" -) +import "fmt" -// ProviderRegistration holds details on how to instantiate the correct git provider -// based on parameters (i.e. repo URL). It allows programs to selectively register -// GitProviderService implementations by anonymously importing implementation packages. -type ProviderRegistration struct { - // Predicate is a function which should return true if the given repoURL is appropriate - // for the provider to handle (e.g. github.com is the domain name) +// Registration holds details on how to instantiate a correct implementation of +// Interface based on parameters (i.e. repo URL). It allows programs to +// selectively register implementations by anonymously importing their packages. +type Registration struct { + // Predicate is a function which should return true if the given repoURL is + // appropriate for the provider to handle (e.g. github.com is the domain + // name). Predicate func(repoURL string) bool - // NewService instantiates the git provider - NewService func(repoURL string, opts *GitProviderOptions) (GitProviderService, error) + // NewProvider instantiates the registered provider implementation. + NewProvider func(repoURL string, opts *Options) (Interface, error) } -var ( - // registeredProviders is a mapping between provider name and provider registration - registeredProviders = map[string]ProviderRegistration{} -) +// registeredProviders is a mapping between provider name and provider registration +var registeredProviders = map[string]Registration{} -// NewGitProviderService returns an implementation of the GitProviderService -// interface. -func NewGitProviderService(repoURL string, opts *GitProviderOptions) (GitProviderService, error) { +// New returns an implementation of Interface suitable for the provided +// repository URL and options. It will return an error if no suitable +// implementation is found. +func New(repoURL string, opts *Options) (Interface, error) { if opts == nil { - opts = &GitProviderOptions{} + opts = &Options{} } if opts.Name != "" { if reg, found := registeredProviders[opts.Name]; found { - return reg.NewService(repoURL, opts) + return reg.NewProvider(repoURL, opts) } return nil, fmt.Errorf("No registered providers with name %q", opts.Name) } for _, reg := range registeredProviders { if reg.Predicate(repoURL) { - return reg.NewService(repoURL, opts) + return reg.NewProvider(repoURL, opts) } } return nil, fmt.Errorf("No registered providers for %s", repoURL) } -// RegisterProvider is called by provider implementation packages to register themselves as -// a git provider. -func RegisterProvider(name string, reg ProviderRegistration) { +// Register is called by provider implementation packages to register themselves +// as a git provider. +func Register(name string, reg Registration) { if _, alreadyRegistered := registeredProviders[name]; alreadyRegistered { panic(fmt.Sprintf("Provider %q already registered", name)) }