From 83d0b04532597fc4f6faae9e0dc7b8ceac792d69 Mon Sep 17 00:00:00 2001 From: David McPike <7037366+mcdafydd@users.noreply.github.com> Date: Sun, 28 Jul 2019 10:45:48 -0500 Subject: [PATCH] Remove Project member from models Repo struct --- server/events/event_parser_test.go | 15 +++---- server/events/models/models.go | 38 ++-------------- server/events/models/models_test.go | 7 +-- server/events/vcs/azuredevops_client.go | 58 +++++++++++++++++++------ 4 files changed, 56 insertions(+), 62 deletions(-) diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index ba6be1712..f4412481b 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -1087,8 +1087,7 @@ func TestParseAzureDevopsRepo(t *testing.T) { r, err := parser.ParseAzureDevopsRepo(&repo) Ok(t, err) Equals(t, models.Repo{ - Owner: "owner", - Project: "project", + Owner: "owner/project", FullName: "owner/project/repo", CloneURL: "https://azuredevops-user:azuredevops-token@dev.azure.com/owner/project/_git/repo", SanitizedCloneURL: "https://azuredevops-user:@dev.azure.com/owner/project/_git/repo", @@ -1105,8 +1104,7 @@ func TestParseAzureDevopsRepo(t *testing.T) { r, err = parser.ParseAzureDevopsRepo(&repo) Ok(t, err) Equals(t, models.Repo{ - Owner: "owner", - Project: "project", + Owner: "owner/project", FullName: "owner/project/repo", CloneURL: "https://azuredevops-user:azuredevops-token@dev.azure.com/owner/project/_git/repo", SanitizedCloneURL: "https://azuredevops-user:@dev.azure.com/owner/project/_git/repo", @@ -1173,8 +1171,7 @@ func TestParseAzureDevopsWorkItemCommentedEvent(t *testing.T) { for _, ref := range pullRefs { Ok(t, err) Equals(t, models.Repo{ - Owner: "owner", - Project: "project", + Owner: "owner/project", FullName: "owner/project/repo", CloneURL: "https://azuredevops-user:azuredevops-token@dev.azure.com/owner/project/_git/repo", SanitizedCloneURL: "https://azuredevops-user:@dev.azure.com/owner/project/_git/repo", @@ -1221,8 +1218,7 @@ func TestParseAzureDevopsPullEvent(t *testing.T) { actPull, evType, actBaseRepo, actHeadRepo, actUser, err := parser.ParseAzureDevopsPullEvent(ADPullEvent) Ok(t, err) expBaseRepo := models.Repo{ - Owner: "owner", - Project: "project", + Owner: "owner/project", FullName: "owner/project/repo", CloneURL: "https://azuredevops-user:azuredevops-token@dev.azure.com/owner/project/_git/repo", SanitizedCloneURL: "https://azuredevops-user:@dev.azure.com/owner/project/_git/repo", @@ -1324,8 +1320,7 @@ func TestParseAzureDevopsPull(t *testing.T) { actPull, actBaseRepo, actHeadRepo, err := parser.ParseAzureDevopsPull(&ADPull) Ok(t, err) expBaseRepo := models.Repo{ - Owner: "owner", - Project: "project", + Owner: "owner/project", FullName: "owner/project/repo", CloneURL: "https://azuredevops-user:azuredevops-token@dev.azure.com/owner/project/_git/repo", SanitizedCloneURL: "https://azuredevops-user:@dev.azure.com/owner/project/_git/repo", diff --git a/server/events/models/models.go b/server/events/models/models.go index f49429beb..7194a0d6f 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -42,9 +42,6 @@ type Repo struct { // subgroups or Azure Devops Team Projects. This may contain spaces in // the case of Bitbucket Server. Owner string - // Project is juts the project name, only required for Azure Devops. This will never - // have /'s in it. - Project string // Name is just the repo name, ex. "atlantis". This will never have // /'s in it. Name string @@ -125,19 +122,14 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU sanitizedCloneURL := strings.Replace(cloneURL, "https://", "https://"+redactedAuth, -1) sanitizedCloneURL = strings.Replace(sanitizedCloneURL, "http://", "http://"+redactedAuth, -1) - // Get the owner, project, and repo names from the full name. - var owner, project, repo string - if vcsHostType == AzureDevops { - owner, project, repo = SplitAzureDevopsRepoFullName(repoFullName) - } else { - owner, repo = SplitRepoFullName(repoFullName) - } + // Get the owner and repo names from the full name. + owner, repo := SplitRepoFullName(repoFullName) if owner == "" || repo == "" { return Repo{}, fmt.Errorf("invalid repo format %q, owner %q or repo %q was empty", repoFullName, owner, repo) } // Only GitLab repos can have /'s in their owners. // This is for GitLab subgroups and Azure Devops Team Projects. - if strings.Contains(owner, "/") && vcsHostType != Gitlab { + if strings.Contains(owner, "/") && vcsHostType != Gitlab && vcsHostType != AzureDevops { return Repo{}, fmt.Errorf("invalid repo format %q, owner %q should not contain any /'s", repoFullName, owner) } if strings.Contains(repo, "/") { @@ -147,7 +139,6 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU return Repo{ FullName: repoFullName, Owner: owner, - Project: project, Name: repo, CloneURL: authedCloneURL, SanitizedCloneURL: sanitizedCloneURL, @@ -391,29 +382,6 @@ func SplitRepoFullName(repoFullName string) (owner string, repo string) { return repoFullName[:lastSlashIdx], repoFullName[lastSlashIdx+1:] } -// SplitAzureDevopsRepoFullName splits a repo full name up into its owner, -// repo and project name segments. If the repoFullName is malformed, may -// return empty strings for owner, repo, or project. Azure Devops uses -// repoFullName format owner/project/repo. -// -// Ex. runatlantis/atlantis => (runatlantis, atlantis) -// gitlab/subgroup/runatlantis/atlantis => (gitlab/subgroup/runatlantis, atlantis) -// azuredevops/project/atlantis => (azuredevops, project, atlantis) -func SplitAzureDevopsRepoFullName(repoFullName string) (owner string, project string, repo string) { - firstSlashIdx := strings.Index(repoFullName, "/") - lastSlashIdx := strings.LastIndex(repoFullName, "/") - slashCount := strings.Count(repoFullName, "/") - if lastSlashIdx == -1 || lastSlashIdx == len(repoFullName)-1 { - return "", "", "" - } - if firstSlashIdx != lastSlashIdx && slashCount == 2 { - return repoFullName[:firstSlashIdx], - repoFullName[firstSlashIdx+1 : lastSlashIdx], - repoFullName[lastSlashIdx+1:] - } - return repoFullName[:lastSlashIdx], "", repoFullName[lastSlashIdx+1:] -} - // ProjectResult is the result of executing a plan/apply for a specific project. type ProjectResult struct { Command CommandName diff --git a/server/events/models/models_test.go b/server/events/models/models_test.go index 78281499a..3d35ef9df 100644 --- a/server/events/models/models_test.go +++ b/server/events/models/models_test.go @@ -19,6 +19,7 @@ import ( "testing" "github.com/runatlantis/atlantis/server/events/models" + "github.com/runatlantis/atlantis/server/events/vcs" . "github.com/runatlantis/atlantis/testing" ) @@ -43,8 +44,8 @@ func TestNewRepo_CloneURLWrongRepo(t *testing.T) { } func TestNewRepo_EmptyAzureDevopsProject(t *testing.T) { - _, err := models.NewRepo(models.AzureDevops, "owner/repo", "https://dev.azure.com/notowner/project/_git/repo", "u", "p") - ErrEquals(t, "AzureDevops project name can't be empty", err) + _, err := models.NewRepo(models.AzureDevops, "", "https://dev.azure.com/notowner/project/_git/repo", "u", "p") + ErrEquals(t, "repoFullName can't be empty", err) } // For bitbucket server we don't validate the clone URL because the callers @@ -334,7 +335,7 @@ func TestAzureDevopsSplitRepoFullName(t *testing.T) { for _, c := range cases { t.Run(c.input, func(t *testing.T) { - owner, project, repo := models.SplitAzureDevopsRepoFullName(c.input) + owner, project, repo := vcs.SplitAzureDevopsRepoFullName(c.input) Equals(t, c.expOwner, owner) Equals(t, c.expProject, project) Equals(t, c.expRepo, repo) diff --git a/server/events/vcs/azuredevops_client.go b/server/events/vcs/azuredevops_client.go index 144bd6265..7c3b8e108 100644 --- a/server/events/vcs/azuredevops_client.go +++ b/server/events/vcs/azuredevops_client.go @@ -73,17 +73,16 @@ func NewAzureDevopsClient(hostname string, org string, username string, project // The names include the path to the file from the repo root, ex. parent/child/file.txt. func (g *AzureDevopsClient) GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error) { var files []string - commitIDResponse := new(azuredevops.GitPullRequest) opts := azuredevops.PullRequestGetOptions{ IncludeWorkItemRefs: true, } - commitIDResponse, _, _ = g.Client.PullRequests.GetWithRepo(g.ctx, repo.Owner, repo.Project, repo.Name, pull.Num, &opts) + owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) + commitIDResponse, _, _ := g.Client.PullRequests.GetWithRepo(g.ctx, owner, project, repoName, pull.Num, &opts) commitID := commitIDResponse.GetLastMergeSourceCommit().GetCommitID() - r := new(azuredevops.GitCommitChanges) - r, _, _ = g.Client.Git.GetChanges(g.ctx, repo.Owner, repo.Project, repo.Name, commitID) + r, _, _ := g.Client.Git.GetChanges(g.ctx, owner, project, repoName, commitID) for _, change := range r.Changes { item := change.GetItem() @@ -132,7 +131,8 @@ func (g *AzureDevopsClient) CreateComment(repo models.Repo, pullNum int, comment opts := azuredevops.PullRequestGetOptions{ IncludeWorkItemRefs: true, } - pull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, repo.Owner, repo.Project, repo.Name, pullNum, &opts) + owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) + pull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, owner, project, repoName, pullNum, &opts) if err != nil { return err } @@ -147,7 +147,8 @@ func (g *AzureDevopsClient) CreateComment(repo models.Repo, pullNum int, comment workItemComment := azuredevops.WorkItemComment{ Text: &s, } - _, _, err := g.Client.WorkItems.CreateComment(g.ctx, repo.Owner, repo.Project, workItemID, &workItemComment) + owner, project, _ := SplitAzureDevopsRepoFullName(repo.FullName) + _, _, err := g.Client.WorkItems.CreateComment(g.ctx, owner, project, workItemID, &workItemComment) if err != nil { return err } @@ -166,7 +167,8 @@ func (g *AzureDevopsClient) CreateComment(repo models.Repo, pullNum int, comment return err } opts := azuredevops.WorkItemCommentListOptions{} - r, _, err := g.Client.WorkItems.ListComments(g.ctx, repo.Owner, repo.Project, workItemID, &opts) + owner, project, _ := SplitAzureDevopsRepoFullName(repo.FullName) + r, _, err := g.Client.WorkItems.ListComments(g.ctx, owner, project, workItemID, &opts) if err != nil { return err } @@ -180,7 +182,7 @@ func (g *AzureDevopsClient) CreateComment(repo models.Repo, pullNum int, comment workItemComment := azuredevops.WorkItemComment{ Text: &s, } - _, _, err = g.Client.WorkItems.CreateComment(g.ctx, repo.Owner, repo.Project, workItemID, &workItemComment) + _, _, err = g.Client.WorkItems.CreateComment(g.ctx, owner, project, workItemID, &workItemComment) if err != nil { return err } @@ -199,7 +201,8 @@ func (g *AzureDevopsClient) PullIsApproved(repo models.Repo, pull models.PullReq opts := azuredevops.PullRequestGetOptions{ IncludeWorkItemRefs: true, } - adPull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, repo.Owner, repo.Project, repo.Name, pull.Num, &opts) + owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) + adPull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, owner, project, repoName, pull.Num, &opts) if err != nil { return false, errors.Wrap(err, "getting pull request") } @@ -221,7 +224,8 @@ func (g *AzureDevopsClient) PullIsMergeable(repo models.Repo, pull models.PullRe opts := azuredevops.PullRequestGetOptions{ IncludeWorkItemRefs: true, } - adPull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, repo.Owner, repo.Project, repo.Name, pull.Num, &opts) + owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) + adPull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, owner, project, repoName, pull.Num, &opts) if err != nil { return false, errors.Wrap(err, "getting pull request") } @@ -237,7 +241,9 @@ func (g *AzureDevopsClient) GetPullRequest(repo models.Repo, num int) (*azuredev opts := azuredevops.PullRequestGetOptions{ IncludeWorkItemRefs: true, } - pull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, repo.Owner, repo.Project, repo.Name, num, &opts) + + owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) + pull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, owner, project, repoName, num, &opts) return pull, err } @@ -304,11 +310,12 @@ func (g *AzureDevopsClient) MergePull(pull models.PullRequest) error { mergePull.AutoCompleteSetBy = &id mergePull.CompletionOptions = &completionOpts + owner, project, repoName := SplitAzureDevopsRepoFullName(pull.BaseRepo.FullName) mergeResult, _, err := g.Client.PullRequests.Merge( g.ctx, - pull.BaseRepo.Owner, - pull.BaseRepo.Project, - pull.BaseRepo.Name, + owner, + project, + repoName, pull.Num, mergePull, completionOpts, @@ -322,3 +329,26 @@ func (g *AzureDevopsClient) MergePull(pull models.PullRequest) error { } return nil } + +// SplitAzureDevopsRepoFullName splits a repo full name up into its owner, +// repo and project name segments. If the repoFullName is malformed, may +// return empty strings for owner, repo, or project. Azure Devops uses +// repoFullName format owner/project/repo. +// +// Ex. runatlantis/atlantis => (runatlantis, atlantis) +// gitlab/subgroup/runatlantis/atlantis => (gitlab/subgroup/runatlantis, atlantis) +// azuredevops/project/atlantis => (azuredevops, project, atlantis) +func SplitAzureDevopsRepoFullName(repoFullName string) (owner string, project string, repo string) { + firstSlashIdx := strings.Index(repoFullName, "/") + lastSlashIdx := strings.LastIndex(repoFullName, "/") + slashCount := strings.Count(repoFullName, "/") + if lastSlashIdx == -1 || lastSlashIdx == len(repoFullName)-1 { + return "", "", "" + } + if firstSlashIdx != lastSlashIdx && slashCount == 2 { + return repoFullName[:firstSlashIdx], + repoFullName[firstSlashIdx+1 : lastSlashIdx], + repoFullName[lastSlashIdx+1:] + } + return repoFullName[:lastSlashIdx], "", repoFullName[lastSlashIdx+1:] +}