From 581551daf9e2d07589d5a8bdf55fef33874e9465 Mon Sep 17 00:00:00 2001 From: Marius Hennecke Date: Thu, 22 Apr 2021 23:20:02 +0200 Subject: [PATCH 1/2] get modified ado pr files from commit and changes endpoint, fixes #1397 --- server/events/vcs/azuredevops_client.go | 49 ++++++----- server/events/vcs/azuredevops_client_test.go | 90 ++++++++++++++------ 2 files changed, 91 insertions(+), 48 deletions(-) diff --git a/server/events/vcs/azuredevops_client.go b/server/events/vcs/azuredevops_client.go index beaf847921..fd905a1e66 100644 --- a/server/events/vcs/azuredevops_client.go +++ b/server/events/vcs/azuredevops_client.go @@ -57,33 +57,40 @@ func NewAzureDevopsClient(hostname string, userName string, token string) (*Azur // relative to the repo root, e.g. parent/child/file.txt. func (g *AzureDevopsClient) GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error) { var files []string + filesSet := make(map[string]bool) - opts := azuredevops.PullRequestGetOptions{ - IncludeWorkItemRefs: true, - } owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) - commitIDResponse, _, _ := g.Client.PullRequests.GetWithRepo(g.ctx, owner, project, repoName, pull.Num, &opts) - - commitID := commitIDResponse.GetLastMergeSourceCommit().GetCommitID() - - r, _, _ := g.Client.Git.GetChanges(g.ctx, owner, project, repoName, commitID) - for _, change := range r.Changes { - item := change.GetItem() - // Convert the path to a relative path from the repo's root. - relativePath := filepath.Clean("./" + item.GetPath()) - files = append(files, relativePath) - - // If the file was renamed, we'll want to run plan in the directory - // it was moved from as well. - changeType := azuredevops.Rename.String() - if change.ChangeType == &changeType { - // Convert the path to a relative path from the repo's root. - relativePath = filepath.Clean("./" + change.GetSourceServerItem()) - files = append(files, relativePath) + commitRefs, _, _ := g.Client.PullRequests.ListCommits(g.ctx, owner, project, repoName, pull.Num) + _ = commitRefs + for _, ref := range commitRefs { + commitID := *ref.CommitID + + r, _, _ := g.Client.Git.GetChanges(g.ctx, owner, project, repoName, commitID) + + for _, change := range r.Changes { + item := change.GetItem() + if *item.GitObjectType == "blob" { + // Convert the path to a relative path from the repo's root. + relativePath := filepath.Clean("./" + item.GetPath()) + filesSet[relativePath] = true + + // If the file was renamed, we'll want to run plan in the directory + // it was moved from as well. + changeType := azuredevops.Rename.String() + if change.ChangeType == &changeType { + // Convert the path to a relative path from the repo's root. + relativePath = filepath.Clean("./" + change.GetSourceServerItem()) + filesSet[relativePath] = true + } + } } } + for file := range filesSet { + files = append(files, file) + } + return files, nil } diff --git a/server/events/vcs/azuredevops_client_test.go b/server/events/vcs/azuredevops_client_test.go index b127d71829..c3c4958e41 100644 --- a/server/events/vcs/azuredevops_client_test.go +++ b/server/events/vcs/azuredevops_client_test.go @@ -13,7 +13,6 @@ import ( "github.com/mcdafydd/go-azuredevops/azuredevops" "github.com/runatlantis/atlantis/server/events/models" "github.com/runatlantis/atlantis/server/events/vcs" - "github.com/runatlantis/atlantis/server/events/vcs/fixtures" . "github.com/runatlantis/atlantis/testing" ) @@ -236,36 +235,73 @@ func TestAzureDevopsClient_UpdateStatus(t *testing.T) { // GetModifiedFiles should make multiple requests if more than one page // and concat results. func TestAzureDevopsClient_GetModifiedFiles(t *testing.T) { - itemRespTemplate := `{ - "changes": [ - { - "item": { - "gitObjectType": "blob", - "path": "%s", - "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/278d5cd2-584d-4b63-824a-2ba458937249/items/MyWebSite/MyWebSite/%s?versionType=Commit" - }, - "changeType": "add" - }, - { - "item": { - "gitObjectType": "blob", - "path": "%s", - "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/278d5cd2-584d-4b63-824a-2ba458937249/items/MyWebSite/MyWebSite/%s?versionType=Commit" + commitsResp := `{ + "count": 2, + "value": [ + { + "commitId": "f14e5c5227145a262c455b95696ca5647567390e", + "author": { + "name": "Jamal Hartnett", + "email": "fabrikamfiber4@hotmail.com", + "date": "2000-01-01T01:00:00Z" + }, + "committer": { + "name": "Jamal Hartnett", + "email": "fabrikamfiber4@hotmail.com", + "date": "2000-01-01T01:00:00Z" + }, + "comment": "foo bar 1", + "url": "https://dev.azure.com/fabrikam/1fbf87b9-64d3-4dea-94ff-352ce2647578/_apis/git/repositories/278d5cd2-584d-4b63-824a-2ba458937249/commits/f14e5c5227145a262c455b95696ca5647567390e" + }, + { + "commitId": "39480e5a326c08131981f0bc9d6d411441ad73be", + "author": { + "name": "Jamal Hartnett", + "email": "fabrikamfiber4@hotmail.com", + "date": "2000-01-01T01:00:00Z" + }, + "committer": { + "name": "Jamal Hartnett", + "email": "fabrikamfiber4@hotmail.com", + "date": "2000-01-01T01:00:00Z" + }, + "comment": "foo bar 2", + "url": "https://dev.azure.com/fabrikam/1fbf87b9-64d3-4dea-94ff-352ce2647578/_apis/git/repositories/278d5cd2-584d-4b63-824a-2ba458937249/commits/39480e5a326c08131981f0bc9d6d411441ad73be" + } + ]}` + changesRespTemplate := `{ + "changeCounts": { + "Edit": 1 }, - "changeType": "add" - } -]}` - resp := fmt.Sprintf(itemRespTemplate, "/file1.txt", "/file1.txt", "/file2.txt", "/file2.txt") + "changes": [ + { + "item": { + "objectId": "9347d78903e37be34de91a8c35818561baa6913f", + "originalObjectId": "573530927d35a9289083224c429bc15ee7f667e2", + "gitObjectType": "blob", + "commitId": "%[1]s", + "path": "%[2]s", + "url": "https://dev.azure.com/fabrikam/1fbf87b9-64d3-4dea-94ff-352ce2647578/_apis/git/repositories/278d5cd2-584d-4b63-824a-2ba458937249/commits/items/%[2]s?versionType=Commit&version=%[1]s" + }, + "changeType": "edit" + } + ] + }` + testServer := httptest.NewTLSServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.RequestURI { - // The first request should hit this URL. - case "/owner/project/_apis/git/repositories/repo/pullrequests/1?api-version=5.1-preview.1&includeWorkItemRefs=true": - w.Write([]byte(fixtures.ADPullJSON)) // nolint: errcheck - // The second should hit this URL. - case "/owner/project/_apis/git/repositories/repo/commits/b60280bc6e62e2f880f1b63c1e24987664d3bda3/changes?api-version=5.1-preview.1": - // We write a header that means there's an additional page. - w.Write([]byte(resp)) // nolint: errcheck + // The first should hit this URL. + case "/owner/project/_apis/git/repositories/repo/pullrequests/1/commits?api-version=5.1-preview.1": + w.Write([]byte(commitsResp)) // nolint: errcheck + return + // get changes for first commit + case "/owner/project/_apis/git/repositories/repo/commits/f14e5c5227145a262c455b95696ca5647567390e/changes?api-version=5.1-preview.1": + w.Write([]byte(fmt.Sprintf(changesRespTemplate, "f14e5c5227145a262c455b95696ca5647567390e", "/file1.txt"))) // nolint: errcheck + return + // get changes for second commit + case "/owner/project/_apis/git/repositories/repo/commits/39480e5a326c08131981f0bc9d6d411441ad73be/changes?api-version=5.1-preview.1": + w.Write([]byte(fmt.Sprintf(changesRespTemplate, "39480e5a326c08131981f0bc9d6d411441ad73be", "/file2.txt"))) // nolint: errcheck return default: t.Errorf("got unexpected request at %q", r.RequestURI) From 22b4524bbcc6b51602daf92ea19fdbece9741651 Mon Sep 17 00:00:00 2001 From: Marius Hennecke Date: Wed, 5 May 2021 23:20:17 +0200 Subject: [PATCH 2/2] Add missing error handling --- server/events/vcs/azuredevops_client.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/server/events/vcs/azuredevops_client.go b/server/events/vcs/azuredevops_client.go index fd905a1e66..7d7932ed71 100644 --- a/server/events/vcs/azuredevops_client.go +++ b/server/events/vcs/azuredevops_client.go @@ -61,16 +61,28 @@ func (g *AzureDevopsClient) GetModifiedFiles(repo models.Repo, pull models.PullR owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName) - commitRefs, _, _ := g.Client.PullRequests.ListCommits(g.ctx, owner, project, repoName, pull.Num) - _ = commitRefs + commitRefs, resp, err := g.Client.PullRequests.ListCommits(g.ctx, owner, project, repoName, pull.Num) + if err != nil { + return nil, errors.Wrap(err, "getting list of pull request commits") + } + if resp.StatusCode != http.StatusOK { + return nil, errors.Wrapf(err, "http response code %d getting list of pull request commits", resp.StatusCode) + } + for _, ref := range commitRefs { - commitID := *ref.CommitID + commitID := ref.GetCommitID() - r, _, _ := g.Client.Git.GetChanges(g.ctx, owner, project, repoName, commitID) + r, resp, err := g.Client.Git.GetChanges(g.ctx, owner, project, repoName, commitID) + if err != nil { + return nil, errors.Wrap(err, "getting commit changes") + } + if resp.StatusCode != http.StatusOK { + return nil, errors.Wrapf(err, "http response code %d getting commit changes", resp.StatusCode) + } for _, change := range r.Changes { item := change.GetItem() - if *item.GitObjectType == "blob" { + if item.GetGitObjectType() == "blob" { // Convert the path to a relative path from the repo's root. relativePath := filepath.Clean("./" + item.GetPath()) filesSet[relativePath] = true