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

Support regexp in command builder on the project name #1419

Merged
merged 4 commits into from
Mar 6, 2021

Conversation

bewie
Copy link
Contributor

@bewie bewie commented Feb 19, 2021

Related to #1288, this PR adds support of regexp on atlantis plan/apply with -p flag.
The main objective is to be able to trigger apply only on a specific range of projects in one GitHub comment.

For Example, all projects that matching my naming convention for staging :

atlantis apply -p .*_staging

Or a specific range of project :

atlantis plan -p project[1-4]

This feature can be enabled with --enable-regexp-cmd flag (disable by default)

Closes #254

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #1419 (90d2529) into master (471646b) will decrease coverage by 0.00%.
The diff coverage is 82.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1419      +/-   ##
==========================================
- Coverage   69.53%   69.52%   -0.01%     
==========================================
  Files          93       93              
  Lines        6299     6327      +28     
==========================================
+ Hits         4380     4399      +19     
- Misses       1542     1550       +8     
- Partials      377      378       +1     
Impacted Files Coverage Δ
cmd/server.go 79.09% <ø> (ø)
server/events/yaml/valid/repo_cfg.go 9.09% <0.00%> (-2.03%) ⬇️
server/user_config.go 100.00% <ø> (ø)
server/events/project_command_builder.go 82.30% <93.33%> (+0.29%) ⬆️
server/server.go 64.98% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 471646b...90d2529. Read the comment docs.

nishkrishnan
nishkrishnan previously approved these changes Feb 25, 2021
Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

LGTM

@nishkrishnan
Copy link
Contributor

Just realized one thing that becomes a bit confusing. --disable-apply-all doesn't work anymore because theoretically you can just do atlantis apply -p * to circumvent it.

Can you think of a good way to make this more restrictive? Maybe gating this feature with a server flag would be enough?

@nishkrishnan nishkrishnan dismissed their stale review February 25, 2021 05:40

Thought of a problem

@nishkrishnan nishkrishnan added the waiting-on-response Waiting for a response from the user label Feb 25, 2021
@bewie
Copy link
Contributor Author

bewie commented Feb 25, 2021

Good point, I guess put this feature behind a server flag is a good idea.

@bewie
Copy link
Contributor Author

bewie commented Feb 27, 2021

Just realized one thing that becomes a bit confusing. --disable-apply-all doesn't work anymore because theoretically you can just do atlantis apply -p * to circumvent it.

Can you think of a good way to make this more restrictive? Maybe gating this feature with a server flag would be enough?

This feature is now disabled by default and managed by the --enable-regexp-cmd option.

Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

LGTM

@nishkrishnan nishkrishnan added waiting-on-review Waiting for a review from a maintainer and removed waiting-on-response Waiting for a response from the user labels Feb 28, 2021
@nishkrishnan
Copy link
Contributor

@bewie before I merge this, can you either update this one with docs on the new flag (and specifically call out that it's not meant to be used with disable-apply-all) or create a new PR for this?

@bewie
Copy link
Contributor Author

bewie commented Mar 5, 2021

@nishkrishnan I updated the docs, let me know if it looks good to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants