feat: set `atlantis/apply` check to `successful` if all plans are `No Changes` #3378

chroju · 2023-05-05T05:06:09Z

what

Add --skip-apply-no-changes option. This option will allow skipping the apply command if the plan command results in "No Changes". This option enables skipping of the apply command by setting the commit status in the VCS to "successful" without actually running the apply command.

After discussion, this feature has been made the default and no option will be provided. (link)

why

I added this option to improve the workflow efficiency. When the plan command results in "No Changes", the apply step is not always necessary.
This is particularly useful when using automated tools like renovate that generate pull requests. If the apply status is successful even with "No Changes", automerge can be performed, further streamlining the workflow process.

tests

I have written tests in server/events/command/project_result_test.go.
I have written tests in server/events/command_runner_internal_test.go.
I have created a pull request on my repository and tested using Atlantis that I started locally. (e.g. Test for Atlantis skipping apply chroju/infrastructure#56 )

references

resolves: When No changes, atlantis/apply status should be success #2546

nitrocode · 2023-05-05T16:10:58Z

The issue is that if the plan runs, shows no-changes, and then there is time in between which causes that project to drift, then if this flag is enabled, the apply won't run and won't error out due to a stale plan. It will merge and everyone will think that at this point, the terraform had zero drift upon merge when that would not be true.

chroju · 2023-05-07T03:28:06Z

@nitrocode Thanks for your comment!

That's true, but making atlantis apply a condition for PR merging is an optional feature and is not necessarily used in every workflow. Even if the atlantis apply is not skipped, it is still possible to execute atlantis unlock later and make changes elsewhere before merging the PR. Therefore, there is a possibility of encountering similar issues as the concern raised in this case.

For instance, in the scenario of skipping apply, I would suggest considering displaying a warning comment in the PR (and the document). Alternatively, considering Atlantis' concept, is it appropriate to merge this PR? If not, I would close the PR and the linked issue.

nitrocode · 2023-05-07T04:56:51Z

Reading over the linked issue, instead of adding an option to allow skipping the apply command if no changes, could we instead set the atlantis/apply check as green if all the plans are no changes?

chroju · 2023-05-07T05:11:25Z

@nitrocode

could we instead set the atlantis/apply check as green if all the plans are no changes?

I wanted to let you know that this PR already includes the implementation you mentioned. I apologize if the title of the pull request and the name of this option may have been misleading.

cmd/server.go

nitrocode · 2023-05-07T12:03:37Z

Thank you for pointing that out. The skip-apply-no-changes makes me think that it will skip the apply step for any single job that is set to no changes which i pointed out. Perhaps to remove confusion, we can change the name of the flag?

set-atlantis-apply-check-successful-if-no-changes

Or perhaps can set this as a default and eliminate the need for this flag?

cc @runatlantis/maintainers thoughts?

jamengual · 2023-05-07T15:19:53Z

I think is ok to have it as default but it needs to be documented well.

…

On Sun, May 7, 2023, 5:03 a.m. nitrocode ***@***.***> wrote: Thank you for pointing that out. The skip-apply-no-changes makes me think that it will skip the apply step for any single job that is set to no changes which i pointed out. Perhaps to remove confusion, we can change the name of the flag? set-atlantis-apply-check-successful-if-no-changes Or perhaps can set this as a default and eliminate the need for this flag? cc @runatlantis/maintainers <https://github.com/orgs/runatlantis/teams/maintainers> thoughts? — Reply to this email directly, view it on GitHub <#3378 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERHFOKKLXUNRZQEHCATXE6FSHANCNFSM6AAAAAAXWUOVOU> . You are receiving this because your review was requested.Message ID: ***@***.***>

Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>

chroju · 2023-05-08T10:42:24Z

Thanks for the feedback. I agree that the flag's name should be changed to avoid confusion.

Regarding keeping the flag, I believe it is a good idea to offer users a choice to use this feature or not, as this feature alters the existing behavior of Atlantis and may not be preferred by everyone.

While it is difficult to decide whether the flag should be enabled by default, I personally think it is fine to have it disabled, as there are other useful features added with their corresponding flags disabled by default.

log

server/events/plan_command_runner.go

runatlantis.io/docs/server-configuration.md

server/events/apply_command_runner.go

server/events/plan_command_runner.go

chroju · 2023-08-02T11:20:35Z

@GenPage
I've made the changes as discussed above. Please review again!

GenPage

@chroju Looking good so far, a couple nit pick questions to clarify before I approve

GenPage · 2023-08-02T15:27:07Z

server/events/plan_command_runner_test.go

+		ExpVCSApplyStatusSucc  int
+	}{
+		{
+			Description:            "When planning with no previous plan, set the atlantis/apply VCS status to 0/0",


To clarify this test condition expects a success or pending status with 0/0?

GenPage · 2023-08-02T15:28:55Z

server/events/plan_command_runner_test.go

+			Targeted:               true,
+			ExpVCSApplyStatusTotal: 1,
+			ExpVCSApplyStatusSucc:  1,
+		},


I feel like we are missing some conditions here that we could be testing for. What happens when plans override each other with new "No changes" or new "Pending" based on drift?

Thanks @GenPage, I've added various test cases to cover different scenarios based on the review feedback. Please check!

…anges'

GenPage

Thank you, looks great now

… Changes` (runatlantis#3378) * mod: rename updateCommitStatus func * feat: add PlannedNoChangesPlanStatus * Add skipApplyNoChanges option to PlanCommandRunner * Add skipApplyNoChanges option to ApplyCommandRunner * Add --skip-apply-no-changes flag * Fix typo Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> * Rename --skip-apply-no-changes flag * Refactor updateCommitStatus functions * chore(docs): add detailed use case for the flag * test: add plan_command_runner set apply status * feat: set apply status to successful by default when result is 'No Changes' --------- Co-authored-by: chroju <chroju@users.noreply.github.com> Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>

Unlike Gitlab, Github's commit statuses do not have a "running" state, so the atlantis flow doesn't have a concept for "running". Instead of adding complexity for the majority use case (Github), I think a clear user experience is to map "pending" to "pending" for Gitlab, and let commit statuses go from pending to either success or failure. Along with runatlantis#3378, this change should result in a better user experience for Gitlab users.

brandon-fryslie · 2024-01-11T10:27:08Z

@GenPage @chroju

This change fundamentally broke Atlantis for many users (it's still broken 5 months later). I appreciate the effort to make things smoother but next time please use a flag.

The problem is that "no changes reported" is very clearly not identical to "terraform apply ran successfully", and the status check has changed meaning as a result. The meaning used to be "terraform apply ran successfully". That was unambiguous and easy to understand . I assumed it would continue meaning that.

But as of this PR the status check can be set without running terraform apply at all. That should have been a hard stop. Running the apply function automatically would be consistent with existing behavior, solve the problem, and seems pretty logical.

This PR seems to completely remove the ability to require terraform apply to have been run before marking the atlantis/apply status check as "successful". Correct me if I'm wrong but that was functionality we relied on and I'm not seeing a replacement. I'm not looking for a status check that says "i detected 0 changes, or ran terraform apply, or maybe no projects got generated, or xyz". I actually do want to require terraform apply to run on every single PR and I'm not seeing how to configure that.

Thanks for your help,

stasostrovskyi · 2024-01-11T10:40:56Z

I really have no opinion on what the flag is called as long as we don't have to sit there typing atlantis apply on PRs with 0 projects and 0 changes. We have a nightly job that updates docs and anything changed is a markdown file that is is excluded from any Atlantis project anyway. I can't fathom why we still need to run atlantis apply just to get the status check to go green. There are 0 projects.

I actually do want to require terraform apply to run on every single PR and I'm not seeing how to configure that.

I'm extremely confused...

GenPage · 2024-01-26T18:32:33Z

@brandon-fryslie, I appreciate you taking the time to explain your use case. I caution against using such strong language about "breaking" Atlantis. We considered this change very heavily before making a decision. You even advocated for this change as others had pointed out. We advertised this change as a breaking change in the v0.25.0 release.

But as of this PR the status check can be set without running terraform apply at all. That should have been a hard stop. Running the apply function automatically would be consistent with existing behavior, solve the problem, and seems pretty logical.

This PR seems to completely remove the ability to require terraform apply to have been run before marking the atlantis/apply status check as "successful". Correct me if I'm wrong but that was functionality we relied on and I'm not seeing a replacement. I'm not looking for a status check that says "i detected 0 changes, or ran terraform apply, or maybe no projects got generated, or xyz". I actually do want to require terraform apply to run on every single PR and I'm not seeing how to configure that.

The status check is set because there are no projects to apply. This is shown in the check as 0/0. Are you saying you have a workflow where you still run a terraform apply when a terraform plan returns No changes? I have never done this. This is a complete 180 from what you stated above in #3378 (comment). You advocated for this change, and now you want it changed back? You are still required to terraform apply when a project has a plan that has changes to apply. If there is a use-case/workflow we haven't consider, I greatly encourage you to file a new issue referencing this PR.

… Changes` (runatlantis#3378) * mod: rename updateCommitStatus func * feat: add PlannedNoChangesPlanStatus * Add skipApplyNoChanges option to PlanCommandRunner * Add skipApplyNoChanges option to ApplyCommandRunner * Add --skip-apply-no-changes flag * Fix typo Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com> * Rename --skip-apply-no-changes flag * Refactor updateCommitStatus functions * chore(docs): add detailed use case for the flag * test: add plan_command_runner set apply status * feat: set apply status to successful by default when result is 'No Changes' --------- Co-authored-by: chroju <chroju@users.noreply.github.com> Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>

brandon-fryslie · 2024-04-19T19:13:51Z

@brandon-fryslie, I appreciate you taking the time to explain your use case. I caution against using such strong language about "breaking" Atlantis. We considered this change very heavily before making a decision. You even advocated for this change as others had pointed out. We advertised this change as a breaking change in the v0.25.0 release.

But as of this PR the status check can be set without running terraform apply at all. That should have been a hard stop. Running the apply function automatically would be consistent with existing behavior, solve the problem, and seems pretty logical.

This PR seems to completely remove the ability to require terraform apply to have been run before marking the atlantis/apply status check as "successful". Correct me if I'm wrong but that was functionality we relied on and I'm not seeing a replacement. I'm not looking for a status check that says "i detected 0 changes, or ran terraform apply, or maybe no projects got generated, or xyz". I actually do want to require terraform apply to run on every single PR and I'm not seeing how to configure that.

The status check is set because there are no projects to apply. This is shown in the check as 0/0. Are you saying you have a workflow where you still run a terraform apply when a terraform plan returns No changes? I have never done this. This is a complete 180 from what you stated above in #3378 (comment). You advocated for this change, and now you want it changed back? You are still required to terraform apply when a project has a plan that has changes to apply. If there is a use-case/workflow we haven't consider, I greatly encourage you to file a new issue referencing this PR.

You are correct, I did advocate for this change. However at that time I hadn't experienced the problematic side effects caused by this change. I was wrong before and hadn't considered the potential issues.

We have a lot of PRs. We have come to rely on Atlantis as the "arbiter" of sorts as to whether a PR request is suitable for merging. The problem is that while this may speed up simple use cases, the risk is that any false positives allow bad code to be merged.

We generate the Atlantis config in a pre-workflow hook. Atlantis now says "0/0 projects" automatically when the PR is opened and automatically sets the status to green. If someone goes and runs "Atlantis plan" then it will generate the config and actually figure out what projects needs to be planned. I have been away for a bit and don't know what series of issues is causing this, but since Atlantis is a critical gatekeeper at this point, it would be great if there was a way to have a failsafe. A way to absolutely ensure Atlantis has planned and applied all changes. Because right now it reports "0/0 projects" when autoplan is used but when run manually it finds projects. It might be due to GitHub team membership. I don't have access to the code to look at it or I could provide more info (I left the company).

nitrocode · 2024-04-20T18:12:42Z

@brandon-fryslie if you set the pre workflow hook or atlantis plan as required checks on the repo, that will prevent the PRs from getting merged prior to running atlantis plan.

chroju added 5 commits May 3, 2023 13:55

mod: rename updateCommitStatus func

59f65d7

feat: add PlannedNoChangesPlanStatus

f0ca470

Add skipApplyNoChanges option to PlanCommandRunner

d549a36

Add skipApplyNoChanges option to ApplyCommandRunner

e219be6

Add --skip-apply-no-changes flag

b035b4a

chroju requested a review from a team as a code owner May 5, 2023 05:06

github-actions bot added docs Documentation go Pull requests that update Go code labels May 5, 2023

Merge branch 'main' into no_changes_bypass_apply

88a687e

chroju changed the title ~~Add option to allow skipping the apply command with "No Changes"~~ feat: Add option to allow skipping the apply command with "No Changes" May 5, 2023

nitrocode reviewed May 7, 2023

View reviewed changes

cmd/server.go Outdated Show resolved Hide resolved

nitrocode changed the title ~~feat: Add option to allow skipping the apply command with "No Changes"~~ feat: option to set atlantis/apply check to successful if all plans are No Changes May 7, 2023

Fix typo

220a895

Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>

Merge branch 'master' into no_changes_bypass_apply

8967726