Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: --silence-no-projects to comments with -d and -p #2969

Merged
merged 7 commits into from
Jan 24, 2023

Conversation

Dawnflash
Copy link
Contributor

@Dawnflash Dawnflash commented Jan 11, 2023

what

  • Makes the SilenceNoProjects flag ignore targeted projects (-d, -p flags) if no configured projects match the target
  • This only applies if projects are defined at repo-level to cooperate with the no-projects setup.
  • The flag effectively restricts the server to defined projects (unless none are defined), which is a mildly breaking feature
  • Adds interaction with SilenceNoProjects to the import command as it is currently lacking it

why

  • The SilenceNoProjects flag is effective at silencing non-targeted commands like atlantis plan but falls flat with targeted commands like atlantis plan -d mydir.
  • Since the purpose of this flag is mainly serving multi-atlantis setups with fully distinct project sets, it is quite undesirable to have an "unused" server (one with 0 project matches) reply with "404, have you run plan?" or similar since it's not expected to run anything in the first place.
  • When -p and -d are used in this setup, we only ever want one Atlantis server to handle the command

tests

  • I added a unit test to the command builder internal tests that tests that my use-case actually ignores the handed project as opposed to a control case.
  • two more tests are added to cover the targeted-but-not-matched plan use-case and interaction with the import command

references

@Dawnflash Dawnflash requested a review from a team as a code owner January 11, 2023 19:28
@Dawnflash Dawnflash marked this pull request as draft January 11, 2023 19:45
@jamengual jamengual added the refactoring Code refactoring that doesn't add additional functionality label Jan 11, 2023
@nitrocode nitrocode changed the title Expand --silence-no-projects to targeted commands feat: --silence-no-projects to comments with -d and -p Jan 11, 2023
@Dawnflash Dawnflash marked this pull request as ready for review January 11, 2023 23:55
@Dawnflash
Copy link
Contributor Author

I updated the import cmd to react to the flag and tried covering more use-cases, especially those in the linked issue.
I tried covering the basics with tests but it doesn't test every possibility.

Now the flag locks the server down on defined projects (it will silently ignore anything else) which is backwards-incompatible (the flag hasn't behaved this way until now). Maybe a new flag could be introduced instead but I haven't figured out how to call it.

@GenPage
Copy link
Member

GenPage commented Jan 18, 2023

@Dawnflash I approved the test runs, can you rebase?

@Dawnflash Dawnflash force-pushed the dawn/feat/silence-targeted-cmds branch 2 times, most recently from aa80d1d to 73dd2bf Compare January 23, 2023 22:35
@Dawnflash Dawnflash force-pushed the dawn/feat/silence-targeted-cmds branch from 73dd2bf to 558839d Compare January 23, 2023 22:35
@github-actions github-actions bot added docs Documentation go Pull requests that update Go code labels Jan 23, 2023
@Dawnflash Dawnflash force-pushed the dawn/feat/silence-targeted-cmds branch 2 times, most recently from a5269a6 to fab8aaa Compare January 23, 2023 22:46
@Dawnflash Dawnflash force-pushed the dawn/feat/silence-targeted-cmds branch from fab8aaa to 47c56b6 Compare January 23, 2023 22:48
@nitrocode
Copy link
Member

Thank you for the PR @Dawnflash !

cc @runatlantis/maintainers for another review

@nitrocode nitrocode requested a review from GenPage January 23, 2023 23:44
Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jamengual
Copy link
Contributor

Thanks again @Dawnflash for the contribution

@nitrocode
Copy link
Member

nitrocode commented Jan 24, 2023

@Dawnflash there seems to be a linting issue. Let's fix that before merging.

reviewdog: found at least one result in diff
  reviewdog: Reporting results for "golangci-lint"
  Error: [golangci-lint] reported by reviewdog 🐶
  Error return value of `db.UpdatePullWithResults` is not checked (errcheck)
  
  Raw Output:
  server/events/apply_command_runner_test.go:166:29: Error return value of `db.UpdatePullWithResults` is not checked (errcheck)
  				db.UpdatePullWithResults(modelPull, []command.ProjectResult{
  				                        ^
  Error: [golangci-lint] reported by reviewdog 🐶
  Error return value of `db.UpdatePullWithResults` is not checked (errcheck)
  
  Raw Output:
  server/events/plan_command_runner_test.go:104:29: Error return value of `db.UpdatePullWithResults` is not checked (errcheck)
  				db.UpdatePullWithResults(modelPull, []command.ProjectResult{
  				                        ^

@Dawnflash
Copy link
Contributor Author

Oops, I fixed that.

@nitrocode nitrocode merged commit 953de00 into runatlantis:main Jan 24, 2023
@nitrocode
Copy link
Member

Thank you again!! This will be available in the dev tag (once the build finishes). Please use that tag if possible to keep testing this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation github-actions go Pull requests that update Go code refactoring Code refactoring that doesn't add additional functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: flag to disable plan/apply on projects not included in atlantis.yaml
4 participants