feat: pending apply status #2188

AndreZiviani · 2022-04-06T16:27:38Z

This PR adds the following improvements:

add an option to set a pending apply status when a plan command is issued (including autoplan), this allows to configure the repository to only be mergeable if atlantis applied all changes
fix apply logic that changed the PR status before checking if it is mergeable
if only atlantis/apply status is pending/failing consider the PR mergeable

With these changes we can configure the git repository to only be mergeable if: all status succeed, at least 1 approval from CODEOWNERS and only allow merging after atlantis is finished

I have tried to implement it before on #2053 but ended up breaking some use-cases (#2176), this adds an option to enable/disable the behavior

…heck if the PR is mergeable like the doc next to it says

…ing/failing

AndreZiviani · 2022-04-06T16:40:09Z

@jamengual @msarvar @nishkrishnan @zxpower can you help review/test this PR? or maybe someone from #2138

jacekn · 2022-04-06T19:57:01Z

if only atlantis/apply status is pending/failing consider the PR mergeable

I have a question about this. Will this method ensure all other merge requirements are considered? For example it's possible to require commits to be signed or for all conversations to be resolved before PRs can be merged in GitHub. Will those requirements be taken into account before atlantis apply is allowed?

AndreZiviani · 2022-04-06T21:06:18Z

Will this method ensure all other merge requirements are considered? [...] Will those requirements be taken into account before atlantis apply is allowed?

yes, it will only ignore the pending atlantis/apply status (https://github.com/runatlantis/atlantis/pull/2188/files#diff-8f27c369fd18d57f78cad1008481a28ce841f08630c69d9c9bbec37c0574dd6aR322)

edbighead · 2022-04-07T07:00:06Z

With these changes we can configure the git repository to only be mergeable if: all status succeed, at least 1 approval from CODEOWNERS and only allow merging after atlantis is finished

Will this create a "chicken or the egg" problem when one of apply_requirements will be mergeable? For example you won't be able to apply until PR is mergeable status (based on octokit/octokit.net#1763) and it won't be mergeable until atlantis/apply passes.

We previously had an issue when an iteration of this PR was merged (without config flag), I documented it in #2182: mergeable (in our case approved by CODEOWNER) apply requirement was not honoured. It worked fine after revert #2173.

jamengual · 2022-04-07T07:10:12Z

We will be releasing base with the revert #2173 soon, hopefully this week. Please keep the feedback coming so we can find a middle ground that works for most people. Thanks. Regards

…

On Thu, Apr 7, 2022 at 12:00 AM Ed Laur ***@***.***> wrote: With these changes we can configure the git repository to only be mergeable if: all status succeed, at least 1 approval from CODEOWNERS and only allow merging after atlantis is finished Will this create a "chicken or the egg" problem when one of apply_requirements will be mergeable? Foe example you won't be able to apply until PR is "mergeable" status (based on octokit/octokit.net#1763 <octokit/octokit.net#1763>) and it won't be mergeable until atlantis/apply passes. We previously had an issue when an iteration of this PR was merged (without config flag), I documented it in #2182 <#2182>: mergeable (in our case approved by CODEOWNER) apply requirement was not honoured. It worked fine after revert #2173 <#2173>. — Reply to this email directly, view it on GitHub <#2188 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERBHUF5DNAGJTOUJXPTVD2BYDANCNFSM5SWS5XSA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jacekn · 2022-04-07T08:28:48Z

Will this method ensure all other merge requirements are considered? [...] Will those requirements be taken into account before atlantis apply is allowed?

yes, it will only ignore the pending atlantis/apply status (https://github.com/runatlantis/atlantis/pull/2188/files#diff-8f27c369fd18d57f78cad1008481a28ce841f08630c69d9c9bbec37c0574dd6aR322)

I checked code and I think this change may not work as expected. Please double check my findings as I only have limited golang experience.

The GetCombinedStatus method will return commit statuses (as per github docs). It will not return any information about whether other merge requirements are satisfied or not.

It is my understanding that even if we ignore the atlantis/apply status check we still don't have enough information to confirm whether a PR can be safely considered mergable.
In GitHub commit status checks are only one of the possible merge requirements. On top of requiring status checks to pass repos can be configured to require signed commits, require multiple approvals (might be a good idea for critically important repos) and a few others. For approach in this PR to work we'd need to confirm that every branch protection requirement has been satisfied not just status checks.

AndreZiviani · 2022-04-07T14:18:12Z

Checking octokit source the blocked state is used only for Failing/missing required status check. Merging is blocked., but the official doc doest not specify what blocked means

The GetCombinedStatus method will return commit statuses (as per github docs). It will not return any information about whether other merge requirements are satisfied or not. [...] On top of requiring status checks to pass repos can be configured to require signed commits, require multiple approvals (might be a good idea for critically important repos) and a few others.

Agreed, AFAIK it will look for the commit status, lets use this PR as an example:

$ curl -s https://api.github.com/repos/runatlantis/atlantis/pulls/2188 | jq '.| {mergeable,mergeable_state, state}'   
{
  "mergeable": true,
  "mergeable_state": "clean",
  "state": "open"
}

GitHub considers it mergeable even without an approval from CODEOWNERS

$ curl -s https://api.github.com/repos/runatlantis/atlantis/commits/c56d6ba/status | jq '.| {state}'
{
  "state": "success"
}

mergeable_state does not check if PR is approved, thats why we have a separate check for that on the workflow config code)

$ curl -s https://api.github.com/repos/runatlantis/atlantis/pulls/2188/reviews | jq  
[]

Other approved PR:

$ curl -s https://api.github.com/repos/runatlantis/atlantis/pulls/2185/reviews | jq '.[] | .user.login,.state'                        3 ↵
"acastle"
"APPROVED"
"jamengual"
"APPROVED"

AndreZiviani · 2022-04-07T14:39:51Z

I'm testing GitHub first because that's what I use but I will validate all this in GitLab after everything is working

AndreZiviani · 2022-04-07T14:59:08Z

After further testing I'm not sure there is a way to do this in GitHub

approved requirement only check if someone approved (not necessarily a reviewer from CODEOWNERS doc )

mergeable can't differentiate between checks (status checks, CODEOWNERS, up to date branch... doc)

I think it might be possible to workaround this in Atlantis but would need to change a lot of the logic (e.g. approved should check if the user is in reviewer list) and will probably break a lot of already running use-cases

edbighead · 2022-04-07T15:19:03Z

@AndreZiviani even though CODEOWNERS files is set, you have to configure branch protection rule for mergeable_state to be blocked (see screen).

I guess there is no such config for this repo, that's why mergeable_state reports clean

AndreZiviani · 2022-04-07T15:24:22Z

@edbighead yes you are right, I think its not set on this (atlantis) repo, but I tested it on my private repo and it returns blocked when something is blocking but I could not find a way to check what is actually blocking: commit status? pending review? branch rule?...

snorlaX-sleeps · 2022-04-07T16:27:43Z

add an option to set a pending apply status when a plan command is issued (including autoplan), this allows to configure the repository to only be mergeable if atlantis applied all changes

@AndreZiviani - will we be able to remove the pending apply status after a plan? I have users asking why their PR's are always still "pending" when no applies have been run? (thanks in advance, looks like a cool feature where needed)

AndreZiviani · 2022-04-07T17:39:30Z

@snorlaX-sleeps yes, now there is an option to enable/disable this behavior (if this PR gets merged eventually)

AndreZiviani · 2022-04-19T21:23:59Z

closing this as I could not find a reliable way to check what is blocking a PR in GitHub (status check or approval or ...)

snorlaX-sleeps · 2022-04-20T00:11:47Z

@snorlaX-sleeps yes, now there is an option to enable/disable this behavior (if this PR gets merged eventually)

@AndreZiviani - I am guessing that flag will not be getting merged? (remove pending apply behaviour)

AndreZiviani · 2022-04-20T11:16:24Z

@snorlaX-sleeps the pending apply behavior was already removed (#2173), this PR was an attempt to fix the bug that it introduced and add a flag to enable/disable this behavior

AndreZiviani added 7 commits April 6, 2022 10:06

add a pending status for apply when running plan command

e997f5a

fix status updater logic, we should only update the status after we c…

17ff318

…heck if the PR is mergeable like the doc next to it says

do not dismiss the PR if the only our "atlantis/apply" status is pend…

413ead3

…ing/failing

fix logic and tests

c0d8f37

linting

00111b0

add option to enable/disable the pending apply commit status

99f7b2b

update tests

c56d6ba

AndreZiviani requested a review from a team as a code owner April 6, 2022 16:27

chenrui333 changed the title ~~Pending apply status~~ feat: pending apply status Apr 16, 2022

AndreZiviani closed this Apr 19, 2022

AndreZiviani mentioned this pull request Aug 9, 2022

Make required atlantis/apply status check work with mergeable using --gh-allow-mergeable-bypass-apply #2436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: pending apply status #2188

feat: pending apply status #2188

AndreZiviani commented Apr 6, 2022

AndreZiviani commented Apr 6, 2022

jacekn commented Apr 6, 2022

AndreZiviani commented Apr 6, 2022

edbighead commented Apr 7, 2022 •

edited

Loading

jamengual commented Apr 7, 2022 via email

jacekn commented Apr 7, 2022

AndreZiviani commented Apr 7, 2022

AndreZiviani commented Apr 7, 2022

AndreZiviani commented Apr 7, 2022

edbighead commented Apr 7, 2022 •

edited

Loading

AndreZiviani commented Apr 7, 2022

snorlaX-sleeps commented Apr 7, 2022 •

edited

Loading

AndreZiviani commented Apr 7, 2022

AndreZiviani commented Apr 19, 2022

snorlaX-sleeps commented Apr 20, 2022

AndreZiviani commented Apr 20, 2022 •

edited

Loading

feat: pending apply status #2188

feat: pending apply status #2188

Conversation

AndreZiviani commented Apr 6, 2022

AndreZiviani commented Apr 6, 2022

jacekn commented Apr 6, 2022

AndreZiviani commented Apr 6, 2022

edbighead commented Apr 7, 2022 • edited Loading

jamengual commented Apr 7, 2022 via email

jacekn commented Apr 7, 2022

AndreZiviani commented Apr 7, 2022

AndreZiviani commented Apr 7, 2022

AndreZiviani commented Apr 7, 2022

edbighead commented Apr 7, 2022 • edited Loading

AndreZiviani commented Apr 7, 2022

snorlaX-sleeps commented Apr 7, 2022 • edited Loading

AndreZiviani commented Apr 7, 2022

AndreZiviani commented Apr 19, 2022

snorlaX-sleeps commented Apr 20, 2022

AndreZiviani commented Apr 20, 2022 • edited Loading

edbighead commented Apr 7, 2022 •

edited

Loading

edbighead commented Apr 7, 2022 •

edited

Loading

snorlaX-sleeps commented Apr 7, 2022 •

edited

Loading

AndreZiviani commented Apr 20, 2022 •

edited

Loading