Introduce new `mergeable` requirement #385

brndnmtthws · 2018-12-11T00:28:06Z

Introduce new mergeable requirement, in similar vein to the approved
requirement.

Addresses #43.

codecov · 2018-12-11T00:39:48Z

Codecov Report

Merging #385 into master will decrease coverage by 0.79%.
The diff coverage is 24.19%.

@@            Coverage Diff            @@
##           master     #385     +/-   ##
=========================================
- Coverage   70.37%   69.58%   -0.8%     
=========================================
  Files          62       62             
  Lines        3743     3797     +54     
=========================================
+ Hits         2634     2642      +8     
- Misses        922      967     +45     
- Partials      187      188      +1

Impacted Files	Coverage Δ
cmd/server.go	`79.16% <ø> (ø)`	⬆️
testdrive/github.go	`0% <ø> (ø)`	⬆️
server/events/vcs/bitbucketcloud/client.go	`41.02% <0%> (-5.13%)`	⬇️
server/events/vcs/proxy.go	`30.76% <0%> (-2.57%)`	⬇️
server/events/vcs/not_configured_vcs_client.go	`0% <0%> (ø)`	⬆️
server/events/vcs/github_client.go	`58.66% <0%> (-4.2%)`	⬇️
server/events/vcs/gitlab_client.go	`19.81% <0%> (-1.41%)`	⬇️
server/events/vcs/bitbucketserver/client.go	`32% <0%> (-3.83%)`	⬇️
server/events/yaml/raw/project.go	`100% <100%> (ø)`	⬆️
server/server.go	`66.27% <100%> (+0.26%)`	⬆️
... and 1 more

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 10bbe0a...12e4f76. Read the comment docs.

codecov · 2018-12-11T00:39:48Z

Codecov Report

Merging #385 into master will decrease coverage by 0.8%.
The diff coverage is 28.78%.

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
- Coverage   70.37%   69.57%   -0.81%     
==========================================
  Files          62       62              
  Lines        3743     3799      +56     
==========================================
+ Hits         2634     2643       +9     
- Misses        922      968      +46     
- Partials      187      188       +1

Impacted Files	Coverage Δ
cmd/server.go	`79.16% <ø> (ø)`	⬆️
server/events/vcs/github_client.go	`58.66% <0%> (-4.2%)`	⬇️
server/events/vcs/gitlab_client.go	`19.81% <0%> (-1.41%)`	⬇️
server/events/vcs/bitbucketserver/client.go	`32% <0%> (-3.83%)`	⬇️
server/events/vcs/not_configured_vcs_client.go	`0% <0%> (ø)`	⬆️
server/events/vcs/bitbucketcloud/client.go	`41.02% <0%> (-5.13%)`	⬇️
server/events/vcs/proxy.go	`30.76% <0%> (-2.57%)`	⬇️
server/server.go	`66.27% <100%> (+0.26%)`	⬆️
server/events/yaml/raw/project.go	`100% <100%> (ø)`	⬆️
server/events/project_command_runner.go	`81.48% <83.33%> (-0.27%)`	⬇️

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 27eb577...e001353. Read the comment docs.

sstarcher · 2018-12-11T01:29:05Z

I'll try and check this out tomorrow.

lkysow · 2018-12-11T14:33:10Z

@brndnmtthws I'm going to merge #386 to regen all the mocks and update pegomock. Can you then rebase off master please? Thanks!

brndnmtthws · 2018-12-11T14:34:19Z

👍

lkysow · 2018-12-11T14:39:13Z

Which VCS providers did you manually test this with? I can help you with testing, I just want to know.

brndnmtthws · 2018-12-11T14:40:45Z

Just GitHub. However with all the code shuffling it would be a good idea for me to test it again before you merge this PR.

brndnmtthws · 2018-12-11T16:12:49Z

I've rebased on master and updated the PR. Also squashed all the commits into one.

It looks like there are some style changes from my linter. I'm guessing that's because we're using a slightly different version. I can go through and drop those changes if necessary.

Let me test this with GitHub and I'll report back.

brndnmtthws · 2018-12-11T18:34:41Z

Everything works, tested with GitHub:

However there's an issue I've noticed, and I'd like to ask for guidance on: should the CLI override options remove any per-project options? Or should it be additive?

Specifically, I'm referring to the logic here: https://github.com/brndnmtthws/atlantis/blob/2a6b2f50edb29545efda24a047ac71214e4a7680/server/events/project_command_runner.go#L218-L223

Currently the logic is additive, so it appends these requirements to the existing list. The downside of this is that it may check the same requirements multiple times if you specify it both per project, and globally. Should the global options override the project options?

Thoughts?

lkysow

In the middle of a review

server/events/project_command_builder_test.go

server/events/project_command_runner.go

server/events/vcs/bitbucketcloud/client.go

server/events/vcs/bitbucketcloud/models.go

server/events/vcs/bitbucketserver/client.go

server/events/vcs/github_client.go

server/events/yaml/parser_validator_test.go

server/server.go

testdrive/github.go

lkysow · 2018-12-11T18:54:43Z

To answer your question, the flags should override the yaml settings. When server-side config is released, the flags will be deprecated.

brndnmtthws · 2018-12-11T19:14:23Z

I think everything has been addressed, but you should probably take another look.

lkysow · 2018-12-11T19:23:43Z

I managed to test Bitbucket Server. We need to hit another endpoint and the response is different:

curl 'http://localhost:7990/rest/api/1.0/projects/AT/repos/atlantis-example/pull-requests/1/merge'
{"canMerge":true,"conflicted":false,"outcome":"CLEAN","vetoes":[]}%

brndnmtthws · 2018-12-11T19:24:41Z

Hmm, should I guess we should be checking canMerge && !conflicted?

brndnmtthws · 2018-12-11T19:31:11Z

Something like this?: https://gist.github.com/brndnmtthws/d828c30fea582504c686845fb1b51cfa

I'm not quite sure how to handle vetoes correctly in the JSON struct.

cmd/server.go

server/events/project_command_runner_test.go

server/events/yaml/raw/project_test.go

lkysow · 2018-12-11T19:34:56Z

Something like this?: https://gist.github.com/brndnmtthws/d828c30fea582504c686845fb1b51cfa

I'm not quite sure how to handle vetoes correctly in the JSON struct.

Patch looks great. Let's ignore vetoes, dunno either 😆

brndnmtthws · 2018-12-11T19:53:16Z

I think that's everything addressed, again.

server/events/project_command_runner_test.go

server/events/vcs/bitbucketserver/client.go

server/events/vcs/bitbucketserver/models.go

Introduce new `mergeable` requirement, in similar vein to the `approved` requirement. Ran `make go-generate` to update mocks accordingly. This addresses issue runatlantis#43.

osterman · 2018-12-12T19:29:54Z

I'm excited about this much-needed PR! It's a step in the right direction.

Not to beat a dead horse, just want to emphasize that without something like plan_requirements, we're still very exposed since any command can be executed as part of plan. I don't want companies having a false sense of security. Full context of previous discussion available here.

Also, just to be clear, this PR addresses a big part of the issues raised. I'm just harping on the lack of security around plan phase.

lkysow · 2018-12-12T20:34:28Z

Thanks for the awesome PR @brndnmtthws. I made some additional changes in #388 specifically around checking another field for github.

brndnmtthws mentioned this pull request Dec 11, 2018

Add new "mergeable" apply requirement #43

Closed

lkysow mentioned this pull request Dec 11, 2018

Restrict Plan or Apply to Github Teams or Github Users #308

Closed

brndnmtthws force-pushed the master branch 2 times, most recently from 723e71d to dc45f69 Compare December 11, 2018 16:11

brndnmtthws force-pushed the master branch 2 times, most recently from 134bd94 to 2a6b2f5 Compare December 11, 2018 18:29

lkysow reviewed Dec 11, 2018

View reviewed changes

server/events/project_command_builder_test.go Outdated Show resolved Hide resolved

server/events/project_command_runner.go Outdated Show resolved Hide resolved

lkysow reviewed Dec 11, 2018

View reviewed changes

brndnmtthws force-pushed the master branch 3 times, most recently from 3d37a6f to 1945a5a Compare December 11, 2018 19:13

lkysow reviewed Dec 11, 2018

View reviewed changes

cmd/server.go Outdated Show resolved Hide resolved

cmd/server.go Show resolved Hide resolved

server/events/project_command_runner_test.go Outdated Show resolved Hide resolved

server/events/yaml/raw/project_test.go Outdated Show resolved Hide resolved

brndnmtthws force-pushed the master branch from 1945a5a to 05a06d2 Compare December 11, 2018 19:53

lkysow reviewed Dec 11, 2018

View reviewed changes

server/events/project_command_runner_test.go Show resolved Hide resolved

server/events/vcs/bitbucketserver/client.go Outdated Show resolved Hide resolved

server/events/vcs/bitbucketserver/models.go Outdated Show resolved Hide resolved

Add mergeable requirement.

e001353

Introduce new `mergeable` requirement, in similar vein to the `approved` requirement. Ran `make go-generate` to update mocks accordingly. This addresses issue runatlantis#43.

brndnmtthws force-pushed the master branch from 05a06d2 to e001353 Compare December 11, 2018 21:00

lkysow mentioned this pull request Dec 12, 2018

Introduce new mergeable requirement #388

Merged

lkysow merged commit e001353 into runatlantis:master Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new `mergeable` requirement #385

Introduce new `mergeable` requirement #385

brndnmtthws commented Dec 11, 2018 •

edited

Loading

codecov bot commented Dec 11, 2018

codecov bot commented Dec 11, 2018 •

edited

Loading

sstarcher commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow left a comment

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

osterman commented Dec 12, 2018 •

edited

Loading

lkysow commented Dec 12, 2018

Introduce new mergeable requirement #385

Introduce new mergeable requirement #385

Conversation

brndnmtthws commented Dec 11, 2018 • edited Loading

codecov bot commented Dec 11, 2018

Codecov Report

codecov bot commented Dec 11, 2018 • edited Loading

Codecov Report

sstarcher commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow left a comment

Choose a reason for hiding this comment

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

lkysow commented Dec 11, 2018

brndnmtthws commented Dec 11, 2018

osterman commented Dec 12, 2018 • edited Loading

lkysow commented Dec 12, 2018

Introduce new `mergeable` requirement #385

Introduce new `mergeable` requirement #385

brndnmtthws commented Dec 11, 2018 •

edited

Loading

codecov bot commented Dec 11, 2018 •

edited

Loading

osterman commented Dec 12, 2018 •

edited

Loading