From fb484ad8ebfce655b0cee3733cfdb3bfa4ba60d7 Mon Sep 17 00:00:00 2001 From: Luke Massa Date: Mon, 11 Dec 2023 20:29:21 -0500 Subject: [PATCH] feat: Remove flag for disable apply (#3912) * Add code for disabling apply flag * Better comment --- cmd/server.go | 16 ----------- cmd/server_test.go | 13 +-------- .../events/events_controller_e2e_test.go | 24 ++++++++++------ ...exp-output-allow-command-unknown-apply.txt | 5 ++++ server/core/locking/apply_locking.go | 28 +++++++++---------- server/core/locking/locking_test.go | 10 +++---- server/events/apply_command_runner.go | 2 +- server/events/apply_command_runner_test.go | 4 +-- server/user_config.go | 1 - 9 files changed, 43 insertions(+), 60 deletions(-) create mode 100644 server/controllers/events/testdata/test-repos/simple-yaml/exp-output-allow-command-unknown-apply.txt diff --git a/cmd/server.go b/cmd/server.go index 93c698ffe..f8d24c9e7 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -28,7 +28,6 @@ import ( "github.com/runatlantis/atlantis/server" "github.com/runatlantis/atlantis/server/core/config/valid" - "github.com/runatlantis/atlantis/server/events/command" "github.com/runatlantis/atlantis/server/events/vcs/bitbucketcloud" "github.com/runatlantis/atlantis/server/logging" ) @@ -71,7 +70,6 @@ const ( DataDirFlag = "data-dir" DefaultTFVersionFlag = "default-tf-version" DisableApplyAllFlag = "disable-apply-all" - DisableApplyFlag = "disable-apply" DisableAutoplanFlag = "disable-autoplan" DisableAutoplanLabelFlag = "disable-autoplan-label" DisableMarkdownFoldingFlag = "disable-markdown-folding" @@ -443,10 +441,6 @@ var boolFlags = map[string]boolFlag{ description: "Disable \"atlantis apply\" command without any flags (i.e. apply all). A specific project/workspace/directory has to be specified for applies.", defaultValue: false, }, - DisableApplyFlag: { - description: "Disable all \"atlantis apply\" command regardless of which flags are passed with it.", - defaultValue: false, - }, DisableAutoplanFlag: { description: "Disable atlantis auto planning feature", defaultValue: false, @@ -1084,16 +1078,6 @@ func (s *ServerCmd) deprecationWarnings(userConfig *server.UserConfig) error { deprecatedFlags = append(deprecatedFlags, RequireMergeableFlag) commandReqs = append(commandReqs, valid.MergeableCommandReq) } - if userConfig.DisableApply { - deprecatedFlags = append(deprecatedFlags, DisableApplyFlag) - var filtered []string - for _, allowCommand := range strings.Split(userConfig.AllowCommands, ",") { - if allowCommand != command.Apply.String() { - filtered = append(filtered, allowCommand) - } - } - userConfig.AllowCommands = strings.Join(filtered, ",") - } // Build up strings with what the recommended yaml and json config should // be instead of using the deprecated flags. diff --git a/cmd/server_test.go b/cmd/server_test.go index 3e2c2e5c5..6b7cbbbbb 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -57,7 +57,7 @@ var testFlags = map[string]interface{}{ ADWebhookPasswordFlag: "ad-wh-pass", ADWebhookUserFlag: "ad-wh-user", AtlantisURLFlag: "url", - AllowCommandsFlag: "version,plan,unlock,import,approve_policies", // apply is disabled by DisableApply + AllowCommandsFlag: "version,plan,apply,unlock,import,approve_policies", AllowForkPRsFlag: true, AllowRepoConfigFlag: true, AutoDiscoverModeFlag: "auto", @@ -71,7 +71,6 @@ var testFlags = map[string]interface{}{ DataDirFlag: "/path", DefaultTFVersionFlag: "v0.11.0", DisableApplyAllFlag: true, - DisableApplyFlag: true, DisableMarkdownFoldingFlag: true, DisableRepoLockingFlag: true, DiscardApprovalOnPlanFlag: true, @@ -763,16 +762,6 @@ func TestExecute_AllowAndWhitelist(t *testing.T) { ErrEquals(t, "--repo-allowlist must be set for security purposes", err) } -func TestExecute_DisableApplyDeprecation(t *testing.T) { - c := setupWithDefaults(map[string]interface{}{ - DisableApplyFlag: true, - AllowCommandsFlag: "plan,apply,unlock", - }, t) - err := c.Execute() - Ok(t, err) - Equals(t, "plan,unlock", passedConfig.AllowCommands) -} - func TestExecute_AutoDetectModulesFromProjects_Env(t *testing.T) { t.Setenv("ATLANTIS_AUTOPLAN_MODULES_FROM_PROJECTS", "**/init.tf") c := setupWithDefaults(map[string]interface{}{}, t) diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go index 9edb17608..5dbf6089e 100644 --- a/server/controllers/events/events_controller_e2e_test.go +++ b/server/controllers/events/events_controller_e2e_test.go @@ -87,8 +87,6 @@ func TestGitHubWorkflow(t *testing.T) { ModifiedFiles []string // Comments are what our mock user writes to the pull request. Comments []string - // DisableApply flag used by userConfig object when initializing atlantis server. - DisableApply bool // ApplyLock creates an apply lock that temporarily disables apply command ApplyLock bool // AllowCommands flag what kind of atlantis commands are available. @@ -457,7 +455,6 @@ func TestGitHubWorkflow(t *testing.T) { Description: "global apply lock disables apply commands", RepoDir: "simple-yaml", ModifiedFiles: []string{"main.tf"}, - DisableApply: false, ApplyLock: true, ExpAutoplan: true, Comments: []string{ @@ -470,18 +467,22 @@ func TestGitHubWorkflow(t *testing.T) { }, }, { - Description: "disable apply flag always takes presedence", + Description: "omitting apply from allow commands always takes presedence", RepoDir: "simple-yaml", ModifiedFiles: []string{"main.tf"}, - DisableApply: true, + AllowCommands: []command.Name{command.Plan}, ApplyLock: false, ExpAutoplan: true, Comments: []string{ "atlantis apply", }, + ExpParseFailedCount: 1, + ExpAllowResponseCommentBack: true, ExpReplies: [][]string{ {"exp-output-autoplan.txt"}, - {"exp-output-apply-locked.txt"}, + // Disabling apply is implementing by omitting it from the apply list + // See: https://github.com/runatlantis/atlantis/pull/2877 + {"exp-output-allow-command-unknown-apply.txt"}, {"exp-output-merge.txt"}, }, }, @@ -628,7 +629,6 @@ func TestGitHubWorkflow(t *testing.T) { // reset userConfig userConfig = server.UserConfig{} - userConfig.DisableApply = c.DisableApply opt := setupOption{ repoConfigFile: c.RepoConfigFile, @@ -799,7 +799,6 @@ func TestSimpleWorkflow_terraformLockFile(t *testing.T) { // reset userConfig userConfig = server.UserConfig{} - userConfig.DisableApply = true ctrl, vcsClient, githubGetter, atlantisWorkspace := setupE2E(t, c.RepoDir, setupOption{}) // Set the repo to be cloned through the testing backdoor. @@ -1295,6 +1294,13 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers if opt.allowCommands != nil { allowCommands = opt.allowCommands } + disableApply := true + for _, allowCommand := range allowCommands { + if allowCommand == command.Apply { + disableApply = false + break + } + } commentParser := &events.CommentParser{ GithubUser: "github-user", GitlabUser: "gitlab-user", @@ -1308,7 +1314,7 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers backend := boltdb lockingClient := locking.NewClient(boltdb) noOpLocker := locking.NewNoOpLocker() - applyLocker = locking.NewApplyClient(boltdb, userConfig.DisableApply) + applyLocker = locking.NewApplyClient(boltdb, disableApply) projectLocker := &events.DefaultProjectLocker{ Locker: lockingClient, NoOpLocker: noOpLocker, diff --git a/server/controllers/events/testdata/test-repos/simple-yaml/exp-output-allow-command-unknown-apply.txt b/server/controllers/events/testdata/test-repos/simple-yaml/exp-output-allow-command-unknown-apply.txt new file mode 100644 index 000000000..33d3bbd1b --- /dev/null +++ b/server/controllers/events/testdata/test-repos/simple-yaml/exp-output-allow-command-unknown-apply.txt @@ -0,0 +1,5 @@ +``` +Error: unknown command "apply". +Run 'atlantis --help' for usage. +Available commands(--allow-commands): plan +``` diff --git a/server/core/locking/apply_locking.go b/server/core/locking/apply_locking.go index 8107770ce..921b4ae7e 100644 --- a/server/core/locking/apply_locking.go +++ b/server/core/locking/apply_locking.go @@ -31,7 +31,7 @@ type ApplyLocker interface { // ApplyCommandLock contains information about apply command lock status. type ApplyCommandLock struct { // Locked is true is when apply commands are locked - // Either by using DisableApply flag or creating a global ApplyCommandLock + // Either by using omitting apply from AllowCommands or creating a global ApplyCommandLock // DisableApply lock take precedence when set Locked bool Time time.Time @@ -39,25 +39,25 @@ type ApplyCommandLock struct { } type ApplyClient struct { - backend Backend - disableApplyFlag bool + backend Backend + disableApply bool } -func NewApplyClient(backend Backend, disableApplyFlag bool) ApplyLocker { +func NewApplyClient(backend Backend, disableApply bool) ApplyLocker { return &ApplyClient{ - backend: backend, - disableApplyFlag: disableApplyFlag, + backend: backend, + disableApply: disableApply, } } // LockApply acquires global apply lock. -// DisableApplyFlag takes presedence to any existing locks, if it is set to true +// DisableApply takes presedence to any existing locks, if it is set to true // this function returns an error func (c *ApplyClient) LockApply() (ApplyCommandLock, error) { response := ApplyCommandLock{} - if c.disableApplyFlag { - return response, errors.New("DisableApplyFlag is set; Apply commands are locked globally until flag is unset") + if c.disableApply { + return response, errors.New("apply is omitted from AllowCommands; Apply commands are locked globally until flag is updated") } applyCmdLock, err := c.backend.LockCommand(command.Apply, time.Now()) @@ -73,11 +73,11 @@ func (c *ApplyClient) LockApply() (ApplyCommandLock, error) { } // UnlockApply releases a global apply lock. -// DisableApplyFlag takes presedence to any existing locks, if it is set to true +// DisableApply takes presedence to any existing locks, if it is set to true // this function returns an error func (c *ApplyClient) UnlockApply() error { - if c.disableApplyFlag { - return errors.New("apply commands are disabled until DisableApply flag is unset") + if c.disableApply { + return errors.New("apply commands are disabled until AllowCommands flag is updated") } err := c.backend.UnlockCommand(command.Apply) @@ -89,11 +89,11 @@ func (c *ApplyClient) UnlockApply() error { } // CheckApplyLock retrieves an apply command lock if present. -// If DisableApplyFlag is set it will always return a lock. +// If DisableApply is set it will always return a lock. func (c *ApplyClient) CheckApplyLock() (ApplyCommandLock, error) { response := ApplyCommandLock{} - if c.disableApplyFlag { + if c.disableApply { return ApplyCommandLock{ Locked: true, }, nil diff --git a/server/core/locking/locking_test.go b/server/core/locking/locking_test.go index dd59454d4..edf90d1a2 100644 --- a/server/core/locking/locking_test.go +++ b/server/core/locking/locking_test.go @@ -202,12 +202,12 @@ func TestApplyLocker(t *testing.T) { Assert(t, !lock.Locked, "exp false") }) - t.Run("can't lock if userConfig.DisableApply is set", func(t *testing.T) { + t.Run("can't lock if apply is omitted from userConfig.AllowCommands", func(t *testing.T) { backend := mocks.NewMockBackend() l := locking.NewApplyClient(backend, true) _, err := l.LockApply() - ErrEquals(t, "DisableApplyFlag is set; Apply commands are locked globally until flag is unset", err) + ErrEquals(t, "apply is omitted from AllowCommands; Apply commands are locked globally until flag is updated", err) backend.VerifyWasCalled(Never()).LockCommand(Any[command.Name](), Any[time.Time]()) }) @@ -232,12 +232,12 @@ func TestApplyLocker(t *testing.T) { Equals(t, errExpected, err) }) - t.Run("can't unlock if userConfig.DisableApply is set", func(t *testing.T) { + t.Run("can't lock if apply is omitted from userConfig.AllowCommands", func(t *testing.T) { backend := mocks.NewMockBackend() l := locking.NewApplyClient(backend, true) err := l.UnlockApply() - ErrEquals(t, "apply commands are disabled until DisableApply flag is unset", err) + ErrEquals(t, "apply commands are disabled until AllowCommands flag is updated", err) backend.VerifyWasCalled(Never()).UnlockCommand(Any[command.Name]()) }) @@ -264,7 +264,7 @@ func TestApplyLocker(t *testing.T) { Equals(t, lock.Locked, false) }) - t.Run("when DisableApply flag is set always return a lock", func(t *testing.T) { + t.Run("when apply is not in AllowCommands always return a lock", func(t *testing.T) { backend := mocks.NewMockBackend() l := locking.NewApplyClient(backend, true) diff --git a/server/events/apply_command_runner.go b/server/events/apply_command_runner.go index 06439aff0..fdaf36d33 100644 --- a/server/events/apply_command_runner.go +++ b/server/events/apply_command_runner.go @@ -68,7 +68,7 @@ func (a *ApplyCommandRunner) Run(ctx *command.Context, cmd *CommentCommand) { pull := ctx.Pull locked, err := a.IsLocked() - // CheckApplyLock falls back to DisableApply flag if fetching the lock + // CheckApplyLock falls back to AllowedCommand flag if fetching the lock // raises an error // We will log failure as warning if err != nil { diff --git a/server/events/apply_command_runner_test.go b/server/events/apply_command_runner_test.go index 61f57a223..27b1efa4f 100644 --- a/server/events/apply_command_runner_test.go +++ b/server/events/apply_command_runner_test.go @@ -34,13 +34,13 @@ func TestApplyCommandRunner_IsLocked(t *testing.T) { ExpComment: "**Error:** Running `atlantis apply` is disabled.", }, { - Description: "When no global apply lock is present and DisableApply flag is false IsDisabled returns false", + Description: "When no global apply lock is present IsDisabled returns false", ApplyLocked: false, ApplyLockError: nil, ExpComment: "Ran Apply for 0 projects:", }, { - Description: "If ApplyLockChecker returns an error IsDisabled return value of DisableApply flag", + Description: "If ApplyLockChecker returns an error IsDisabled returns false", ApplyLockError: errors.New("error"), ApplyLocked: false, ExpComment: "Ran Apply for 0 projects:", diff --git a/server/user_config.go b/server/user_config.go index ce57d18a2..219730c57 100644 --- a/server/user_config.go +++ b/server/user_config.go @@ -33,7 +33,6 @@ type UserConfig struct { CheckoutStrategy string `mapstructure:"checkout-strategy"` DataDir string `mapstructure:"data-dir"` DisableApplyAll bool `mapstructure:"disable-apply-all"` - DisableApply bool `mapstructure:"disable-apply"` DisableAutoplan bool `mapstructure:"disable-autoplan"` DisableAutoplanLabel string `mapstructure:"disable-autoplan-label"` DisableMarkdownFolding bool `mapstructure:"disable-markdown-folding"`