From 27007e88884461edd610ee637bf1e1c6844c50f9 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Sun, 2 Feb 2025 12:29:18 -0500 Subject: [PATCH 1/8] chore: Remove usage of errors.Cause (#5291) Signed-off-by: Luke Massa --- server/events/project_command_builder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/events/project_command_builder.go b/server/events/project_command_builder.go index f626d4b605..f66d2f659f 100644 --- a/server/events/project_command_builder.go +++ b/server/events/project_command_builder.go @@ -825,7 +825,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectCommand(ctx *command.Context, // use the default repository workspace because it is the only one guaranteed to have an atlantis.yaml, // other workspaces will not have the file if they are using pre_workflow_hooks to generate it dynamically repoDir, err := p.WorkingDir.GetWorkingDir(ctx.Pull.BaseRepo, ctx.Pull, DefaultWorkspace) - if os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { return projCtx, errors.New("no working directory found–did you run plan?") } else if err != nil { return projCtx, err From aa4a98032eb43f562d325466ff89b310de8dc8b7 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Sun, 2 Feb 2025 12:44:39 -0500 Subject: [PATCH 2/8] chore: Remove dependency on multierror (#5275) Signed-off-by: Luke Massa --- go.mod | 2 +- .../exp-output-approve-policies.txt | 5 +---- server/core/runtime/policy/conftest_client.go | 21 +++++++------------ server/events/project_command_runner.go | 21 +++++++++---------- 4 files changed, 20 insertions(+), 29 deletions(-) diff --git a/go.mod b/go.mod index 03730a8f60..d17aafafc9 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,6 @@ require ( github.com/gorilla/mux v1.8.1 github.com/gorilla/websocket v1.5.3 github.com/hashicorp/go-getter/v2 v2.2.3 - github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-version v1.7.0 github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/hashicorp/hc-install v0.9.0 @@ -96,6 +95,7 @@ require ( github.com/gorilla/css v1.0.1 // indirect github.com/hashicorp/errwrap v1.1.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect + github.com/hashicorp/go-multierror v1.1.1 // indirect github.com/hashicorp/go-retryablehttp v0.7.7 // indirect github.com/hashicorp/go-safetemp v1.0.0 // indirect github.com/hashicorp/hcl v1.0.0 // indirect diff --git a/server/controllers/events/testdata/test-repos/policy-checks-diff-owner/exp-output-approve-policies.txt b/server/controllers/events/testdata/test-repos/policy-checks-diff-owner/exp-output-approve-policies.txt index b842f99682..017f43b8b8 100644 --- a/server/controllers/events/testdata/test-repos/policy-checks-diff-owner/exp-output-approve-policies.txt +++ b/server/controllers/events/testdata/test-repos/policy-checks-diff-owner/exp-output-approve-policies.txt @@ -6,10 +6,7 @@ Ran Approve Policies for 1 projects: ### 1. dir: `.` workspace: `default` **Approve Policies Error** ``` -1 error occurred: - * policy set: test_policy user runatlantis is not a policy owner - please contact policy owners to approve failing policies - - +policy set: test_policy user runatlantis is not a policy owner - please contact policy owners to approve failing policies ``` #### Policy Approval Status: ``` diff --git a/server/core/runtime/policy/conftest_client.go b/server/core/runtime/policy/conftest_client.go index dd69bba4cd..88dc3bb173 100644 --- a/server/core/runtime/policy/conftest_client.go +++ b/server/core/runtime/policy/conftest_client.go @@ -2,6 +2,7 @@ package policy import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -12,9 +13,9 @@ import ( "regexp" "github.com/hashicorp/go-getter/v2" - "github.com/hashicorp/go-multierror" + version "github.com/hashicorp/go-version" - "github.com/pkg/errors" + "github.com/runatlantis/atlantis/server/core/config/valid" "github.com/runatlantis/atlantis/server/core/runtime/cache" runtime_models "github.com/runatlantis/atlantis/server/core/runtime/models" @@ -139,7 +140,7 @@ func (c ConfTestVersionDownloader) downloadConfTestVersion(v *version.Version, d fullSrcURL := fmt.Sprintf("%s?checksum=file:%s", binURL, checksumURL) if err := c.downloader.GetAny(destPath, fullSrcURL); err != nil { - return runtime_models.LocalFilePath(""), errors.Wrapf(err, "downloading conftest version %s at %q", v.String(), fullSrcURL) + return runtime_models.LocalFilePath(""), fmt.Errorf("downloading conftest version %s at %q: %w", v.String(), fullSrcURL, err) } binPath := filepath.Join(destPath, "conftest") @@ -212,9 +213,9 @@ func (c *ConfTestExecutorWorkflow) Run(ctx command.ProjectContext, executablePat if cmdErr != nil { // Since we're running conftest for each policyset, individual command errors should be concatenated. if isValidConftestOutput(cmdOutput) { - combinedErr = multierror.Append(combinedErr, fmt.Errorf("policy_set: %s: conftest: some policies failed", policySet.Name)) + combinedErr = errors.Join(combinedErr, fmt.Errorf("policy_set: %s: conftest: some policies failed", policySet.Name)) } else { - combinedErr = multierror.Append(combinedErr, fmt.Errorf("policy_set: %s: conftest: %s", policySet.Name, cmdOutput)) + combinedErr = errors.Join(combinedErr, fmt.Errorf("policy_set: %s: conftest: %s", policySet.Name, cmdOutput)) } } @@ -247,13 +248,7 @@ func (c *ConfTestExecutorWorkflow) Run(ctx command.ProjectContext, executablePat policyCheckResultFile := filepath.Join(workdir, ctx.GetPolicyCheckResultFileName()) err = os.WriteFile(policyCheckResultFile, marshaledStatus, 0600) - combinedErr = multierror.Append(combinedErr, err) - - // Multierror will wrap combined errors in a way that the upstream functions won't be able to read it as nil. - // Let's pass nil back if there are no wrapped errors. - if errors.Unwrap(combinedErr) == nil { - combinedErr = nil - } + combinedErr = errors.Join(combinedErr, err) output := string(marshaledStatus) @@ -306,7 +301,7 @@ func getDefaultVersion() (*version.Version, error) { wrappedVersion, err := version.NewVersion(defaultVersion) if err != nil { - return nil, errors.Wrapf(err, "wrapping version %s", defaultVersion) + return nil, fmt.Errorf("wrapping version %s: %w", defaultVersion, err) } return wrappedVersion, nil } diff --git a/server/events/project_command_runner.go b/server/events/project_command_runner.go index 0558fe35aa..523dbd87ac 100644 --- a/server/events/project_command_runner.go +++ b/server/events/project_command_runner.go @@ -15,13 +15,12 @@ package events import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" "strings" - "github.com/hashicorp/go-multierror" - "github.com/pkg/errors" "github.com/runatlantis/atlantis/server/core/config/valid" "github.com/runatlantis/atlantis/server/core/runtime" "github.com/runatlantis/atlantis/server/events/command" @@ -347,7 +346,7 @@ func (p *DefaultProjectCommandRunner) doApprovePolicies(ctx command.ProjectConte // Acquire Atlantis lock for this repo/dir/workspace. lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocksMode == valid.RepoLocksOnPlanMode) if err != nil { - return nil, "", errors.Wrap(err, "acquiring lock") + return nil, "", fmt.Errorf("acquiring lock: %w", err) } if !lockAttempt.LockAcquired { return nil, lockAttempt.LockFailureReason, nil @@ -408,10 +407,10 @@ func (p *DefaultProjectCommandRunner) doApprovePolicies(ctx command.ProjectConte } // User matches the author and prevent self approve is set to true } else if isOwner && !ignorePolicy && ctx.User.Username == ctx.Pull.Author && policySet.PreventSelfApprove { - prjErr = multierror.Append(prjErr, fmt.Errorf("policy set: %s the author of pr %s matches the command commenter user %s - please contact another policy owners to approve failing policies", policySet.Name, ctx.Pull.Author, ctx.User.Username)) + prjErr = errors.Join(prjErr, fmt.Errorf("policy set: %s the author of pr %s matches the command commenter user %s - please contact another policy owners to approve failing policies", policySet.Name, ctx.Pull.Author, ctx.User.Username)) // User is not authorized to approve policy set. } else if !ignorePolicy { - prjErr = multierror.Append(prjErr, fmt.Errorf("policy set: %s user %s is not a policy owner - please contact policy owners to approve failing policies", policySet.Name, ctx.User.Username)) + prjErr = errors.Join(prjErr, fmt.Errorf("policy set: %s user %s is not a policy owner - please contact policy owners to approve failing policies", policySet.Name, ctx.User.Username)) } // Still bubble up this failure, even if policy set is not targeted. if !policyStatus.Passed && (prjPolicyStatus[i].Approvals != policySet.ApproveCount) { @@ -449,7 +448,7 @@ func (p *DefaultProjectCommandRunner) doPolicyCheck(ctx command.ProjectContext) lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocksMode == valid.RepoLocksOnPlanMode) if err != nil { - return nil, "", errors.Wrap(err, "acquiring lock") + return nil, "", fmt.Errorf("acquiring lock: %w", err) } if !lockAttempt.LockAcquired { return nil, lockAttempt.LockFailureReason, nil @@ -502,7 +501,7 @@ func (p *DefaultProjectCommandRunner) doPolicyCheck(ctx command.ProjectContext) } // Exclude errors for failed policies if !strings.Contains(err.Error(), "some policies failed") { - errs = multierror.Append(errs, err) + errs = errors.Join(errs, err) } } @@ -567,7 +566,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model // Acquire Atlantis lock for this repo/dir/workspace. lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocksMode == valid.RepoLocksOnPlanMode) if err != nil { - return nil, "", errors.Wrap(err, "acquiring lock") + return nil, "", fmt.Errorf("acquiring lock: %w", err) } if !lockAttempt.LockAcquired { return nil, lockAttempt.LockFailureReason, nil @@ -644,7 +643,7 @@ func (p *DefaultProjectCommandRunner) doApply(ctx command.ProjectContext) (apply // Acquire Atlantis lock for this repo/dir/workspace. lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocksMode == valid.RepoLocksOnApplyMode) if err != nil { - return "", "", errors.Wrap(err, "acquiring lock") + return "", "", fmt.Errorf("acquiring lock: %w", err) } if !lockAttempt.LockAcquired { return "", lockAttempt.LockFailureReason, nil @@ -724,7 +723,7 @@ func (p *DefaultProjectCommandRunner) doImport(ctx command.ProjectContext) (out // Acquire Atlantis lock for this repo/dir/workspace. lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocksMode != valid.RepoLocksDisabledMode) if err != nil { - return nil, "", errors.Wrap(err, "acquiring lock") + return nil, "", fmt.Errorf("acquiring lock: %w", err) } if !lockAttempt.LockAcquired { return nil, lockAttempt.LockFailureReason, nil @@ -765,7 +764,7 @@ func (p *DefaultProjectCommandRunner) doStateRm(ctx command.ProjectContext) (out // Acquire Atlantis lock for this repo/dir/workspace. lockAttempt, err := p.Locker.TryLock(ctx.Log, ctx.Pull, ctx.User, ctx.Workspace, models.NewProject(ctx.Pull.BaseRepo.FullName, ctx.RepoRelDir, ctx.ProjectName), ctx.RepoLocksMode != valid.RepoLocksDisabledMode) if err != nil { - return nil, "", errors.Wrap(err, "acquiring lock") + return nil, "", fmt.Errorf("acquiring lock: %w", err) } if !lockAttempt.LockAcquired { return nil, lockAttempt.LockFailureReason, nil From c2c25aeef2af1dbd3bb6798aed5633166aa0b3c3 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Sun, 2 Feb 2025 22:32:50 -0500 Subject: [PATCH 3/8] fix: Minor bug in determining teams for gitlab (#5294) Signed-off-by: Luke Massa --- server/events/vcs/gitlab_client.go | 4 +- server/events/vcs/gitlab_client_test.go | 106 ++++++++++++------ .../vcs/testdata/gitlab-user-multiple.json | 20 ++++ .../events/vcs/testdata/gitlab-user-none.json | 1 + 4 files changed, 97 insertions(+), 34 deletions(-) create mode 100644 server/events/vcs/testdata/gitlab-user-multiple.json create mode 100644 server/events/vcs/testdata/gitlab-user-none.json diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 2ac61a403f..68f78a3829 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -637,10 +637,10 @@ func (g *GitlabClient) GetTeamNamesForUser(logger logging.SimpleLogging, _ model if err != nil { return nil, errors.Wrapf(err, "GET /users returned: %d", resp.StatusCode) } else if len(users) == 0 { - return nil, errors.Wrap(err, "GET /users returned no user") + return nil, errors.New("GET /users returned no user") } else if len(users) > 1 { // Theoretically impossible, just being extra safe - return nil, errors.Wrap(err, "GET /users returned more than 1 user") + return nil, errors.New("GET /users returned more than 1 user") } userID := users[0].ID for _, groupName := range g.ConfiguredGroups { diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index 49c9a0f8f0..aaa85b0e5b 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -1121,40 +1121,82 @@ func TestGitlabClient_GetTeamNamesForUser(t *testing.T) { userSuccess, err := os.ReadFile("testdata/gitlab-user-success.json") Ok(t, err) - configuredGroups := []string{"someorg/group1", "someorg/group2", "someorg/group3", "someorg/group4"} - testServer := httptest.NewServer( - http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.RequestURI { - case "/api/v4/users?username=testuser": - w.WriteHeader(http.StatusOK) - w.Write(userSuccess) // nolint: errcheck - case "/api/v4/groups/someorg%2Fgroup1/members/123", "/api/v4/groups/someorg%2Fgroup2/members/123": - w.WriteHeader(http.StatusOK) - w.Write(groupMembershipSuccess) // nolint: errcheck - case "/api/v4/groups/someorg%2Fgroup3/members/123": - http.Error(w, "forbidden", http.StatusForbidden) - case "/api/v4/groups/someorg%2Fgroup4/members/123": - http.Error(w, "not found", http.StatusNotFound) - default: - t.Errorf("got unexpected request at %q", r.RequestURI) - http.Error(w, "not found", http.StatusNotFound) - } - })) - internalClient, err := gitlab.NewClient("token", gitlab.WithBaseURL(testServer.URL)) + userEmpty, err := os.ReadFile("testdata/gitlab-user-none.json") Ok(t, err) - client := &GitlabClient{ - Client: internalClient, - Version: nil, - ConfiguredGroups: configuredGroups, + + multipleUsers, err := os.ReadFile("testdata/gitlab-user-multiple.json") + Ok(t, err) + + configuredGroups := []string{"someorg/group1", "someorg/group2", "someorg/group3", "someorg/group4"} + + cases := []struct { + userName string + expErr string + expTeams []string + }{ + { + userName: "testuser", + expTeams: []string{"someorg/group1", "someorg/group2"}, + }, + { + userName: "none", + expErr: "GET /users returned no user", + }, + { + userName: "multiuser", + expErr: "GET /users returned more than 1 user", + }, } + for _, c := range cases { + t.Run(c.userName, func(t *testing.T) { + + testServer := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/v4/users?username=testuser": + w.WriteHeader(http.StatusOK) + w.Write(userSuccess) // nolint: errcheck + case "/api/v4/users?username=none": + w.WriteHeader(http.StatusOK) + w.Write(userEmpty) // nolint: errcheck + case "/api/v4/users?username=multiuser": + w.WriteHeader(http.StatusOK) + w.Write(multipleUsers) // nolint: errcheck + case "/api/v4/groups/someorg%2Fgroup1/members/123", "/api/v4/groups/someorg%2Fgroup2/members/123": + w.WriteHeader(http.StatusOK) + w.Write(groupMembershipSuccess) // nolint: errcheck + case "/api/v4/groups/someorg%2Fgroup3/members/123": + http.Error(w, "forbidden", http.StatusForbidden) + case "/api/v4/groups/someorg%2Fgroup4/members/123": + http.Error(w, "not found", http.StatusNotFound) + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + } + })) + internalClient, err := gitlab.NewClient("token", gitlab.WithBaseURL(testServer.URL)) + Ok(t, err) + client := &GitlabClient{ + Client: internalClient, + Version: nil, + ConfiguredGroups: configuredGroups, + } + + teams, err := client.GetTeamNamesForUser( + logger, + models.Repo{ + Owner: "someorg", + }, models.User{ + Username: c.userName, + }) + if c.expErr == "" { + Ok(t, err) + Equals(t, c.expTeams, teams) + } else { + ErrContains(t, c.expErr, err) + + } - teams, err := client.GetTeamNamesForUser( - logger, - models.Repo{ - Owner: "someorg", - }, models.User{ - Username: "testuser", }) - Ok(t, err) - Equals(t, []string{"someorg/group1", "someorg/group2"}, teams) + } } diff --git a/server/events/vcs/testdata/gitlab-user-multiple.json b/server/events/vcs/testdata/gitlab-user-multiple.json new file mode 100644 index 0000000000..dfd5373067 --- /dev/null +++ b/server/events/vcs/testdata/gitlab-user-multiple.json @@ -0,0 +1,20 @@ +[ + { + "id": 123, + "username": "multiuser", + "name": "Multiple User 1", + "state": "active", + "locked": false, + "avatar_url": "https://gitlab.com/uploads/-/system/user/avatar/123/avatar.png", + "web_url": "https://gitlab.com/multiuser" + }, + { + "id": 124, + "username": "multiuser", + "name": "Multiple User 2", + "state": "active", + "locked": false, + "avatar_url": "https://gitlab.com/uploads/-/system/user/avatar/124/avatar.png", + "web_url": "https://gitlab.com/multiuser" + } +] diff --git a/server/events/vcs/testdata/gitlab-user-none.json b/server/events/vcs/testdata/gitlab-user-none.json new file mode 100644 index 0000000000..fe51488c70 --- /dev/null +++ b/server/events/vcs/testdata/gitlab-user-none.json @@ -0,0 +1 @@ +[] From 618d5acd4545cf3d0f8c773ca4d5e146b982466e Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Mon, 3 Feb 2025 03:34:41 +0000 Subject: [PATCH 4/8] chore(deps): update github/codeql-action action to v3.28.6 in .github/workflows/scorecard.yml (main) (#5295) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- .github/workflows/scorecard.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index df1fdaed38..e63e974909 100644 --- a/.github/workflows/scorecard.yml +++ b/.github/workflows/scorecard.yml @@ -56,6 +56,6 @@ jobs: # Upload the results to GitHub's code scanning dashboard. - name: 'Upload to code-scanning' - uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0 + uses: github/codeql-action/upload-sarif@17a820bf2e43b47be2c72b39cc905417bc1ab6d0 # v3.28.6 with: sarif_file: results.sarif From 9dfc3eaf297677cc59c1defb7659cdbe60f44600 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 4 Feb 2025 00:38:47 +0000 Subject: [PATCH 5/8] chore(deps): update ghcr.io/runatlantis/atlantis:latest docker digest to 9145bab in dockerfile.dev (main) (#5300) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- Dockerfile.dev | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile.dev b/Dockerfile.dev index d053237b55..ae10a42252 100644 --- a/Dockerfile.dev +++ b/Dockerfile.dev @@ -1,3 +1,3 @@ -FROM ghcr.io/runatlantis/atlantis:latest@sha256:f95cdf8f42370abf68d9e095930de8812e3b2a0dd66c9ffc39a69c8f49727989 +FROM ghcr.io/runatlantis/atlantis:latest@sha256:9145babb08e8a3e80e6367af8bf8e443d75be622864ce80a0410a40de5e0f4ac COPY atlantis /usr/local/bin/atlantis WORKDIR /atlantis/src From 16a9f10c5edd263d5e772ad5e4d8f1a342e03996 Mon Sep 17 00:00:00 2001 From: Simon Heather <32168619+X-Guardian@users.noreply.github.com> Date: Tue, 4 Feb 2025 20:02:26 +0000 Subject: [PATCH 6/8] chore: Add Close Calls to BoltDB Tests to fix Windows Test Errors (#5289) Signed-off-by: X-Guardian <32168619+X-Guardian@users.noreply.github.com> --- server/core/db/boltdb.go | 4 +++ server/core/db/boltdb_test.go | 8 ++++- server/events/apply_command_runner_test.go | 3 ++ server/events/command_runner_test.go | 42 ++++++++++++++++++++++ server/events/delete_lock_command_test.go | 3 ++ server/events/plan_command_runner_test.go | 9 +++++ server/events/pull_closed_executor_test.go | 12 +++++++ 7 files changed, 80 insertions(+), 1 deletion(-) diff --git a/server/core/db/boltdb.go b/server/core/db/boltdb.go index 79e0798c29..f670b37585 100644 --- a/server/core/db/boltdb.go +++ b/server/core/db/boltdb.go @@ -504,3 +504,7 @@ func (b *BoltDB) projectResultToProject(p command.ProjectResult) models.ProjectS Status: p.PlanStatus(), } } + +func (b *BoltDB) Close() error { + return b.db.Close() +} diff --git a/server/core/db/boltdb_test.go b/server/core/db/boltdb_test.go index f69ca8e8d8..13495ff5ed 100644 --- a/server/core/db/boltdb_test.go +++ b/server/core/db/boltdb_test.go @@ -489,6 +489,7 @@ func TestPullStatus_UpdateGet(t *testing.T) { Status: models.ErroredPlanStatus, }, }, status.Projects) + b.Close() } // Test we can create a status, delete it, and then we shouldn't be able to getCommandLock @@ -533,6 +534,7 @@ func TestPullStatus_UpdateDeleteGet(t *testing.T) { maybeStatus, err := b.GetPullStatus(pull) Ok(t, err) Assert(t, maybeStatus == nil, "exp nil") + b.Close() } // Test we can create a status, update a specific project's status within that @@ -597,6 +599,7 @@ func TestPullStatus_UpdateProject(t *testing.T) { Status: models.AppliedPlanStatus, }, }, status.Projects) // nolint: staticcheck + b.Close() } // Test that if we update an existing pull status and our new status is for a @@ -659,6 +662,7 @@ func TestPullStatus_UpdateNewCommit(t *testing.T) { Status: models.AppliedPlanStatus, }, }, maybeStatus.Projects) + b.Close() } // Test that if we update an existing pull status via Apply and our new status is for a @@ -771,6 +775,7 @@ func TestPullStatus_UpdateMerge_Apply(t *testing.T) { }, }, updateStatus.Projects) } + b.Close() } // Test that if we update one existing policy status via approve_policies and our new status is for a @@ -885,6 +890,7 @@ func TestPullStatus_UpdateMerge_ApprovePolicies(t *testing.T) { }, }, updateStatus.Projects) } + b.Close() } // newTestDB returns a TestDB using a temporary path. @@ -925,6 +931,6 @@ func newTestDB2(t *testing.T) *db.BoltDB { } func cleanupDB(db *bolt.DB) { - os.Remove(db.Path()) // nolint: errcheck db.Close() // nolint: errcheck + os.Remove(db.Path()) // nolint: errcheck } diff --git a/server/events/apply_command_runner_test.go b/server/events/apply_command_runner_test.go index 6ef5873c90..a4579018f8 100644 --- a/server/events/apply_command_runner_test.go +++ b/server/events/apply_command_runner_test.go @@ -140,6 +140,9 @@ func TestApplyCommandRunner_IsSilenced(t *testing.T) { // create an empty DB tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) vcsClient := setup(t, func(tc *TestConfig) { diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go index 4e32994824..5927b6d970 100644 --- a/server/events/command_runner_test.go +++ b/server/events/command_runner_test.go @@ -86,6 +86,9 @@ func setup(t *testing.T, options ...func(testConfig *TestConfig)) *vcsmocks.Mock // create an empty DB tmp := t.TempDir() defaultBoltDB, err := db.New(tmp) + t.Cleanup(func() { + defaultBoltDB.Close() + }) Ok(t, err) testConfig := &TestConfig{ @@ -773,6 +776,9 @@ func TestRunAutoplanCommand_DeletePlans(t *testing.T) { setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -800,6 +806,9 @@ func TestRunAutoplanCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_Fal setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -828,6 +837,9 @@ func TestRunAutoplanCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_Tru setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -852,6 +864,9 @@ func TestRunCommentCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_Fals setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -874,6 +889,9 @@ func TestRunCommentCommand_FailedPreWorkflowHook_FailOnPreWorkflowHookError_True setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -892,6 +910,9 @@ func TestRunGenericPlanCommand_DeletePlans(t *testing.T) { setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -914,6 +935,9 @@ func TestRunSpecificPlanCommandDoesnt_DeletePlans(t *testing.T) { setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -934,6 +958,9 @@ func TestRunAutoplanCommandWithError_DeletePlans(t *testing.T) { tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -986,6 +1013,9 @@ func TestRunGenericPlanCommand_DiscardApprovals(t *testing.T) { tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -1011,6 +1041,9 @@ func TestFailedApprovalCreatesFailedStatusUpdate(t *testing.T) { setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -1057,6 +1090,9 @@ func TestApprovedPoliciesUpdateFailedPolicyStatus(t *testing.T) { setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -1113,6 +1149,9 @@ func TestApplyMergeablityWhenPolicyCheckFails(t *testing.T) { setup(t) tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB @@ -1191,6 +1230,9 @@ func TestRunApply_DiscardedProjects(t *testing.T) { defer func() { autoMerger.GlobalAutomerge = false }() tmp := t.TempDir() boltDB, err := db.New(tmp) + t.Cleanup(func() { + boltDB.Close() + }) Ok(t, err) dbUpdater.Backend = boltDB applyCommandRunner.Backend = boltDB diff --git a/server/events/delete_lock_command_test.go b/server/events/delete_lock_command_test.go index 2e652770b9..3efb43b474 100644 --- a/server/events/delete_lock_command_test.go +++ b/server/events/delete_lock_command_test.go @@ -60,6 +60,9 @@ func TestDeleteLock_Success(t *testing.T) { }, nil) tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) dlc := events.DefaultDeleteLockCommand{ Locker: l, diff --git a/server/events/plan_command_runner_test.go b/server/events/plan_command_runner_test.go index b1da0012e1..f8dc1d202c 100644 --- a/server/events/plan_command_runner_test.go +++ b/server/events/plan_command_runner_test.go @@ -78,6 +78,9 @@ func TestPlanCommandRunner_IsSilenced(t *testing.T) { // create an empty DB tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) vcsClient := setup(t, func(tc *TestConfig) { @@ -463,6 +466,9 @@ func TestPlanCommandRunner_ExecutionOrder(t *testing.T) { tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) vcsClient := setup(t, func(tc *TestConfig) { @@ -700,6 +706,9 @@ func TestPlanCommandRunner_AtlantisApplyStatus(t *testing.T) { // create an empty DB tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) vcsClient := setup(t, func(tc *TestConfig) { diff --git a/server/events/pull_closed_executor_test.go b/server/events/pull_closed_executor_test.go index df904a1c6f..add595ea67 100644 --- a/server/events/pull_closed_executor_test.go +++ b/server/events/pull_closed_executor_test.go @@ -43,6 +43,9 @@ func TestCleanUpPullWorkspaceErr(t *testing.T) { w := mocks.NewMockWorkingDir() tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) pce := events.PullClosedExecutor{ WorkingDir: w, @@ -63,6 +66,9 @@ func TestCleanUpPullUnlockErr(t *testing.T) { l := lockmocks.NewMockLocker() tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) pce := events.PullClosedExecutor{ Locker: l, @@ -85,6 +91,9 @@ func TestCleanUpPullNoLocks(t *testing.T) { cp := vcsmocks.NewMockClient() tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) pce := events.PullClosedExecutor{ Locker: l, @@ -182,6 +191,9 @@ func TestCleanUpPullComments(t *testing.T) { l := lockmocks.NewMockLocker() tmp := t.TempDir() db, err := db.New(tmp) + t.Cleanup(func() { + db.Close() + }) Ok(t, err) pce := events.PullClosedExecutor{ Locker: l, From 38ba3869cd4549f2e031fab1f79cc2c2395453fe Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Wed, 5 Feb 2025 01:04:55 +0000 Subject: [PATCH 7/8] chore(deps): update golangci/golangci-lint-action digest to e60da84 in .github/workflows/lint.yml (main) (#5302) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- .github/workflows/lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 951538e9b6..26f208604c 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -65,7 +65,7 @@ jobs: go-version-file: go.mod - name: golangci-lint - uses: golangci/golangci-lint-action@ec5d18412c0aeab7936cb16880d708ba2a64e1ae # v6 + uses: golangci/golangci-lint-action@e60da84bfae8c7920a47be973d75e15710aa8bd7 # v6 with: # renovate: datasource=github-releases depName=golangci/golangci-lint version: v1.62.2 From 766dac90227ff19513a44015921386c816e95a70 Mon Sep 17 00:00:00 2001 From: Hajime Terasawa Date: Wed, 5 Feb 2025 14:33:41 +0900 Subject: [PATCH 8/8] fix: ensure cloning workingdir before calling plan api (#3584) Signed-off-by: Hajime Terasawa Co-authored-by: PePe Amengual <2208324+jamengual@users.noreply.github.com> --- server/controllers/api_controller.go | 39 +++- server/controllers/api_controller_test.go | 205 +++++++++++++++++++--- server/server.go | 2 + 3 files changed, 215 insertions(+), 31 deletions(-) diff --git a/server/controllers/api_controller.go b/server/controllers/api_controller.go index 29037ec9a0..0694e72b22 100644 --- a/server/controllers/api_controller.go +++ b/server/controllers/api_controller.go @@ -33,6 +33,8 @@ type APIController struct { RepoAllowlistChecker *events.RepoAllowlistChecker Scope tally.Scope VCSClient vcs.Client + WorkingDir events.WorkingDir + WorkingDirLocker events.WorkingDirLocker CommitStatusUpdater events.CommitStatusUpdater } @@ -91,12 +93,18 @@ func (a *APIController) Plan(w http.ResponseWriter, r *http.Request) { return } + err = a.apiSetup(ctx) + if err != nil { + a.apiReportError(w, http.StatusInternalServerError, err) + return + } + result, err := a.apiPlan(request, ctx) if err != nil { a.apiReportError(w, http.StatusInternalServerError, err) return } - defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, 0) // nolint: errcheck + defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, ctx.Pull.Num) // nolint: errcheck if result.HasErrors() { code = http.StatusInternalServerError } @@ -119,13 +127,19 @@ func (a *APIController) Apply(w http.ResponseWriter, r *http.Request) { return } + err = a.apiSetup(ctx) + if err != nil { + a.apiReportError(w, http.StatusInternalServerError, err) + return + } + // We must first make the plan for all projects _, err = a.apiPlan(request, ctx) if err != nil { a.apiReportError(w, http.StatusInternalServerError, err) return } - defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, 0) // nolint: errcheck + defer a.Locker.UnlockByPull(ctx.HeadRepo.FullName, ctx.Pull.Num) // nolint: errcheck // We can now prepare and run the apply step result, err := a.apiApply(request, ctx) @@ -145,6 +159,27 @@ func (a *APIController) Apply(w http.ResponseWriter, r *http.Request) { a.respond(w, logging.Warn, code, "%s", string(response)) } +func (a *APIController) apiSetup(ctx *command.Context) error { + pull := ctx.Pull + baseRepo := ctx.Pull.BaseRepo + headRepo := ctx.HeadRepo + + unlockFn, err := a.WorkingDirLocker.TryLock(baseRepo.FullName, pull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir) + if err != nil { + return err + } + ctx.Log.Debug("got workspace lock") + defer unlockFn() + + // ensure workingDir is present + _, _, err = a.WorkingDir.Clone(ctx.Log, headRepo, pull, events.DefaultWorkspace) + if err != nil { + return err + } + + return nil +} + func (a *APIController) apiPlan(request *APIRequest, ctx *command.Context) (*command.Result, error) { cmds, cc, err := request.getCommands(ctx, a.ProjectCommandBuilder.BuildPlanCommands) if err != nil { diff --git a/server/controllers/api_controller_test.go b/server/controllers/api_controller_test.go index 778c41bee2..02a35dbcbd 100644 --- a/server/controllers/api_controller_test.go +++ b/server/controllers/api_controller_test.go @@ -25,49 +25,194 @@ const atlantisToken = "token" func TestAPIController_Plan(t *testing.T) { ac, projectCommandBuilder, projectCommandRunner := setup(t) - body, _ := json.Marshal(controllers.APIRequest{ - Repository: "Repo", - Ref: "main", - Type: "Gitlab", - Projects: []string{"default"}, - }) - req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) - req.Header.Set(atlantisTokenHeader, atlantisToken) - w := httptest.NewRecorder() - ac.Plan(w, req) - ResponseContains(t, w, http.StatusOK, "") - projectCommandBuilder.VerifyWasCalledOnce().BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]()) - projectCommandRunner.VerifyWasCalledOnce().Plan(Any[command.ProjectContext]()) + + cases := []struct { + repository string + ref string + vcsType string + pr int + projects []string + paths []struct { + Directory string + Workspace string + } + }{ + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + projects: []string{"default"}, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + { + Directory: "./myworkspace2", + Workspace: "myworkspace2", + }, + }, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + projects: []string{"test"}, + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + }, + }, + } + + expectedCalls := 0 + for _, c := range cases { + body, _ := json.Marshal(controllers.APIRequest{ + Repository: c.repository, + Ref: c.ref, + Type: c.vcsType, + PR: c.pr, + Projects: c.projects, + Paths: c.paths, + }) + + req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) + req.Header.Set(atlantisTokenHeader, atlantisToken) + w := httptest.NewRecorder() + ac.Plan(w, req) + ResponseContains(t, w, http.StatusOK, "") + + expectedCalls += len(c.projects) + expectedCalls += len(c.paths) + } + + projectCommandBuilder.VerifyWasCalled(Times(expectedCalls)).BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]()) + projectCommandRunner.VerifyWasCalled(Times(expectedCalls)).Plan(Any[command.ProjectContext]()) } func TestAPIController_Apply(t *testing.T) { ac, projectCommandBuilder, projectCommandRunner := setup(t) - body, _ := json.Marshal(controllers.APIRequest{ - Repository: "Repo", - Ref: "main", - Type: "Gitlab", - Projects: []string{"default"}, - }) - req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) - req.Header.Set(atlantisTokenHeader, atlantisToken) - w := httptest.NewRecorder() - ac.Apply(w, req) - ResponseContains(t, w, http.StatusOK, "") - projectCommandBuilder.VerifyWasCalledOnce().BuildApplyCommands(Any[*command.Context](), Any[*events.CommentCommand]()) - projectCommandRunner.VerifyWasCalledOnce().Plan(Any[command.ProjectContext]()) - projectCommandRunner.VerifyWasCalledOnce().Apply(Any[command.ProjectContext]()) + + cases := []struct { + repository string + ref string + vcsType string + pr int + projects []string + paths []struct { + Directory string + Workspace string + } + }{ + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + projects: []string{"default"}, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + { + Directory: "./myworkspace2", + Workspace: "myworkspace2", + }, + }, + }, + { + repository: "Repo", + ref: "main", + vcsType: "Gitlab", + pr: 1, + projects: []string{"test"}, + paths: []struct { + Directory string + Workspace string + }{ + { + Directory: ".", + Workspace: "myworkspace", + }, + }, + }, + } + + expectedCalls := 0 + for _, c := range cases { + body, _ := json.Marshal(controllers.APIRequest{ + Repository: c.repository, + Ref: c.ref, + Type: c.vcsType, + PR: c.pr, + Projects: c.projects, + Paths: c.paths, + }) + + req, _ := http.NewRequest("POST", "", bytes.NewBuffer(body)) + req.Header.Set(atlantisTokenHeader, atlantisToken) + w := httptest.NewRecorder() + ac.Apply(w, req) + ResponseContains(t, w, http.StatusOK, "") + + expectedCalls += len(c.projects) + expectedCalls += len(c.paths) + } + + projectCommandBuilder.VerifyWasCalled(Times(expectedCalls)).BuildApplyCommands(Any[*command.Context](), Any[*events.CommentCommand]()) + projectCommandRunner.VerifyWasCalled(Times(expectedCalls)).Plan(Any[command.ProjectContext]()) + projectCommandRunner.VerifyWasCalled(Times(expectedCalls)).Apply(Any[command.ProjectContext]()) } func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, *MockProjectCommandRunner) { RegisterMockTestingT(t) locker := NewMockLocker() logger := logging.NewNoopLogger(t) - scope, _, _ := metrics.NewLoggingScope(logger, "null") parser := NewMockEventParsing() - vcsClient := NewMockClient() repoAllowlistChecker, err := events.NewRepoAllowlistChecker("*") + scope, _, _ := metrics.NewLoggingScope(logger, "null") + vcsClient := NewMockClient() + workingDir := NewMockWorkingDir() Ok(t, err) + workingDirLocker := NewMockWorkingDirLocker() + When(workingDirLocker.TryLock(Any[string](), Any[int](), Eq(events.DefaultWorkspace), Eq(events.DefaultRepoRelDir))). + ThenReturn(func() {}, nil) + projectCommandBuilder := NewMockProjectCommandBuilder() When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())). ThenReturn([]command.ProjectContext{{ @@ -111,6 +256,8 @@ func setup(t *testing.T) (controllers.APIController, *MockProjectCommandBuilder, PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner, VCSClient: vcsClient, RepoAllowlistChecker: repoAllowlistChecker, + WorkingDir: workingDir, + WorkingDirLocker: workingDirLocker, CommitStatusUpdater: commitStatusUpdater, } return ac, projectCommandBuilder, projectCommandRunner diff --git a/server/server.go b/server/server.go index 97363fcdf3..966e1cfa39 100644 --- a/server/server.go +++ b/server/server.go @@ -956,6 +956,8 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { RepoAllowlistChecker: repoAllowlist, Scope: statsScope.SubScope("api"), VCSClient: vcsClient, + WorkingDir: workingDir, + WorkingDirLocker: workingDirLocker, } eventsController := &events_controllers.VCSEventsController{