Allow "Mergeable" apply requirement to work #454
Conversation
Codecov Report
@@ Coverage Diff @@
## master #454 +/- ##
==========================================
- Coverage 70.78% 70.75% -0.03%
==========================================
Files 63 63
Lines 3987 3991 +4
==========================================
+ Hits 2822 2824 +2
- Misses 970 971 +1
- Partials 195 196 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #454 +/- ##
==========================================
- Coverage 70.53% 70.53% -0.01%
==========================================
Files 63 63
Lines 4243 4245 +2
==========================================
+ Hits 2993 2994 +1
- Misses 1028 1029 +1
Partials 222 222
Continue to review full report at Codecov.
|
pratikmallya
force-pushed
the
master
branch
4 times, most recently
from
bcc0a2d
to
cf7cf56
Compare
|
@lkysow the codecov decrease seems to be due to an additional code that logs a warning. Not sure if you want me to fix that |
pratikmallya
force-pushed
the
master
branch
2 times, most recently
from
a81a600
to
b432ace
Compare
|
If an Atlantis apply fails, then even this change won't help since when we look up the merge status prior to the pull request, it will say non-mergeable, which will cause the apply to fail again... basically we're stuck in a loop. |
|
I don't think it's possible to use the mergeable requirement and require the Atlantis status check. |
|
Sure, but isn't the workflow to plan again? |
|
ie apply -> apply fails -> plan -> apply again etc |
|
So the tradeoff here seems to be that you lose the ability to reapply a failed plan. Which... is perhaps enforcing a best practice? : ) |
|
Oh, that's a good workaround. |
|
If you apply and it fails because you didn't get reviewed, then you get stuck in the same situation and have to re-plan. So I still think it's not recommended to require the atlantis status to pass and use the mergeability requirement but this PR will make it less likely you get into a weird situation. |
|
Please take a look @lkysow ; incorporated your suggestions |
| @@ -451,8 +458,6 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) { | |||
| switch step { | |||
| case "approved": | |||
| mockApproved.VerifyWasCalledOnce().PullIsApproved(ctx.BaseRepo, ctx.Pull) | |||
| case "mergeable": | |||
| mockMergeable.VerifyWasCalledOnce().PullIsMergeable(ctx.BaseRepo, ctx.Pull) | |||
| case "init": | |||
| mockInit.VerifyWasCalledOnce().Run(ctx, nil, repoDir) | |||
| case "plan": | |||
There was a problem hiding this comment.
Can you add a test case that tests your new code?
There was a problem hiding this comment.
How would that work? Sorry, I'm not sure how this mocking framework works, but it seems like you're testing if a function is being called; whereas in the case of testing Mergeability, its just checking the value of ctx.PullMergeable
There was a problem hiding this comment.
@lkysow Added a test case to test the failure path. not sure if that's what you were asking for; lemme know
pratikmallya
force-pushed
the
master
branch
2 times, most recently
from
13b4f91
to
f1bc44a
Compare
Get the Mergeable status of the PR before updating the Status. This allows for the Status to be a required Status for merging the PR. This allows us to use the atlantis status as a Required status, which is the configuration often used, since most deployments want to ensure tf plans have all been successfully applied before merging Issue: runatlantis#453
Get the Mergeable status of the PR before updating the
Status. This allows for the Status to be a required
Status for merging the PR. This allows us to use the
atlantis status as a Required status, which is the
configuration often used, since most deployments
want to ensure tf plans have all been successfully
applied before merging
Issue: #453