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

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

Merged
merged 8 commits into from
Aug 18, 2022
Merged

Conversation

rayterrill
Copy link
Contributor

@rayterrill rayterrill commented Aug 9, 2022

Building on the work of @AndreZiviani, @nishkrishnan, and probably many others - another attempt to try to get mergeable working with required "atlantis/apply" status checks.

First contribution attempt for Atlantis 🙏 - we use this tool heavily and would love to contribute back and get this working if possible - it's a critical pain point in our workflow (and seems similar for others) - would love to partner up with a more experience Atlantis contrib to work through any issues, concerns, etc.

Tests passing locally (via make test).

@rayterrill rayterrill requested a review from a team as a code owner August 9, 2022 21:09
@@ -313,6 +347,10 @@ func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest
// hooks. Merging is allowed (green box).
// See: https://github.com/octokit/octokit.net/issues/1763
if state != "clean" && state != "unstable" && state != "has_hooks" {
if state == "blocked" && status && approved.IsApproved {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue here is that state will always be blocked (assuming the pending apply status) but you can't know for certain why it is blocked (GitHub api does not specify the reason, only that it is blocked)

I'm not sure if other status and approval are the only things that would block a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndreZiviani for sure - I anticipated this discussion - that's definitely the majority of the issue here - trying to disentangle what's making up that state.

@AndreZiviani
Copy link
Contributor

AndreZiviani commented Aug 9, 2022

Also you need to actually add the pending apply status here like I did here but keep in mind that this change will affect all VCS not only GitHub

Make sure to read the comments in my initial PR, this issue, this other issue, this dicussion and my last attempt

This behavior would also need to be enabled/disabled via a feature flag in order to not break existing use-cases like the ones I linked before

@rayterrill
Copy link
Contributor Author

Also you need to actually add the pending apply status here like I did here but keep in mind that this change will affect all VCS not only GitHub

@AndreZiviani can you explain a little more about the need for the pending apply status? I saw that, but didn't totally understand the need there. I'll dig into that as well, but hoping you can clarify for me.

@AndreZiviani
Copy link
Contributor

AndreZiviani commented Aug 10, 2022

[...] another attempt to try to get mergeable working with required "atlantis/apply" status checks.

I assumed that you want to add a pending "atlantis/apply" status check after you run the plan, its been a while since I've looked into this but I don't think it does this by default, if thats the case then you would need to do what I said earlier (this PR as is only does half of the work)

@rayterrill
Copy link
Contributor Author

rayterrill commented Aug 10, 2022

I assumed that you want to add a pending "atlantis/apply" status check after you run the plan, its been a while since I've looked into this but I don't think it does this by default, if thats the case then you would need to do what I said earlier (this PR as is only does half of the work)

I think I'm still missing something here - I've built this code, and when I run atlantis plan or atlantis apply, I indeed see the pending updates come in. I guess I'm not understanding why something would need to be added here - just not familiar enough with the codebase. I'm not changing anything about how the "normal" plan/apply routines would run (including the posting of any pending status check updates) - just adjusting the ruleset used to determine if an apply should be allowed - and only on the github side of things (so ideally no side-effects on any other vcs).

@AndreZiviani
Copy link
Contributor

AndreZiviani commented Aug 10, 2022

when you run atlantis plan it automatically adds a pending apply status? it doesn't for me
image

I'm asking because since it does not add a pending/apply status it already work as intended (the status is not set so you don't need to ignore it), I assumed you wanted to add it like I did

@rayterrill
Copy link
Contributor Author

I'm sorry @AndreZiviani I'm still not understanding. When I grab atlantis@master, and then compare it to my PR, the results appear identical to me, which is what I was going for.

When I run atlantis plan, I see this:
image

This seems correct to me - the plan is done and successful, and the apply hasn't run yet, but is expected. I wouldn't expect to see a pending apply at this point - because I haven't done anything with the apply stage.

When I run atlantis apply, I see the following checks:
image

This also seems correct to me, and it's the same behavior as with atlantis@master.

@AndreZiviani
Copy link
Contributor

Then I apologize for the confusion, I misunderstood what you are trying to do.

LGTM

@chicocvenancio
Copy link
Contributor

I think this will have the same issue as #2311, won't it?

Meaning approvals by people not in the CODEOWNER file will be allowed to apply.

It might be the case I'm misunderstanding the issue in #2112, the code here, or even github's apis, but I think to support the use case of requiring CODEWONER approval we need github's pullrequest graphql object with reviewDecision field.

@AndreZiviani
Copy link
Contributor

You are absolutely right @chicocvenancio , that’s the main reason why I gave up trying to implement something like this

@lilincmu
Copy link
Contributor

Adding more context to the above discussion regarding reviewDecision.

As pointed out in #2112 (comment) and its following discussion, we might need to use the reviewDecision in GraphQL API as the approval status, instead of using REST API. In short, the approval status from REST API does not take CODEOWNERS into consideration, so anyone can approve a PR and then apply it, even when apply_requirements: [mergeable] and CODEOWNERS are configured. (It's another use case where people want to protect their infra from being applied arbitrarily. They want a PR to be applied, only after it satisfies certain brach protection rules, in this case, approved by code owners, and becomes mergeable.)

@rayterrill
Copy link
Contributor Author

rayterrill commented Aug 11, 2022

@lilincmu Let me test that out - that's definitely a scenario we're using, and I can pretty easily test that out in our environment using a build of this PR. Thank you!

Edit: Confirmed this indeed "ignores" CODEOWNERS. I'll dig into this. Great catch @lilincmu .

Copy link
Contributor

@lilincmu lilincmu left a comment

Choose a reason for hiding this comment

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

+1 on the comment from @AndreZiviani A feature flag might buy us some time to polish the feature without having to revert it immediately due to potential regression.

This behavior would also need to be enabled/disabled via a feature flag in order to not break existing use-cases like the ones I linked before

server/events/vcs/github_client.go Outdated Show resolved Hide resolved
server/events/vcs/github_client.go Outdated Show resolved Hide resolved
server/events/vcs/github_client.go Outdated Show resolved Hide resolved
@rayterrill
Copy link
Contributor Author

@lilincmu Added a new GetPullReviewDecision function which calls the graphql api - this indeed takes into account CODEOWENRS and correctly returns "REVIEW_REQUIRED" when a CODEOWNER has not approved the request but someone else has.

Is there an example of featureflag functionality being used already that I could leverage while staying aligned with the codebase?

@jamengual
Copy link
Contributor

#2362

@jamengual jamengual added provider/github waiting-on-review Waiting for a review from a maintainer feature New functionality/enhancement labels Aug 11, 2022
@rayterrill
Copy link
Contributor Author

@jamengual it doesnt look like atlantis passes userConfig down to NewGithubClient. I could definitely add that in, might be worth adding in the entire userConfig to allow additional flags to modify how that works in the future? Is there another way to do this?

@chicocvenancio
Copy link
Contributor

Do we still need the feature flag with the current PR? Seems to me this should just work as is currently documented.

@rayterrill
Copy link
Contributor Author

It's a good question. I'm a 1st time contributor and thus looking for guidance from more seasoned contribs.

No issue with feature flag rollout to minimize disruptions/accidentally introducing something else - in particular because mergeable is a meta/rollup status - I'm a little concerned we're missing something else other than just approval.

As far as the feature flag though - just not totally sure how to introduce that without having to change a major func/method signature.

}

//compile for performance reasons
matcher, _ := regexp.Compile("atlantis/apply")
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to pass vcsstatusname down here since it's actually configurable. See the counterpart of GitLab as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about that - will fix.


for _, r := range status.Statuses {
if !matcher.MatchString(*r.Context) {
if *r.State == "failure" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to return false when checks are pending?

@lilincmu
Copy link
Contributor

it doesnt look like atlantis passes userConfig down to NewGithubClient. I could definitely add that in, might be worth adding in the entire userConfig to allow additional flags to modify how that works in the future? Is there another way to do this?

@rayterrill Alternatively, you might add the new flag in pullReqStatusFetcher and pass it down from https://github.com/runatlantis/atlantis/blob/master/server/server.go#L622. Later, the flag can be passed in as a parameter when PullIsMergeable calls are made.

Do we still need the feature flag with the current PR? Seems to me this should just work as is currently documented.

@chicocvenancio Ideally we don't, however, given the history of the multiple attempts for this feature, I think we can be a bit pessimistic on it.

@rayterrill
Copy link
Contributor Author

Tracking another issue - looks like a previous PR attempt ran into issues with Github Actions-based checks.

I added one to a test PR, and indeed, the CombinedStatus API doesn't appear to take this into account - those get reported under CheckSuite/CheckRun apis. I'll look at getting that added as well, and update the GetCombinedStatus API to handle both.

@rayterrill
Copy link
Contributor Author

Added additional logic to suss out only the required check runs and ignore non-required, added server config doc.

@rayterrill
Copy link
Contributor Author

@lilincmu do you know if there's a next step to get a maintainer to unlock the additional workflows?

@rayterrill
Copy link
Contributor Author

Anything else needed to move this along?

@jamengual
Copy link
Contributor

we will merge it now and release it on the prerelease @rayterrill

@jamengual jamengual merged commit 89d33a0 into runatlantis:master Aug 18, 2022
@rayterrill
Copy link
Contributor Author

Wow thank you @jamengual ❤️ !

@nitrocode
Copy link
Member

nitrocode commented Oct 7, 2022

Nice job @rayterrill !

For anyone else who finds this PR from the 0.19.9 release page, remember to set one of these to allow setting atlantis/apply as a requirement. The following is documented by @chroju in PR #2568.

ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY=true

or flag

--gh-allow-mergeable-bypass-apply

@Roberdvs
Copy link

Roberdvs commented Oct 17, 2022

Nice job @rayterrill !

For anyone else who finds this PR from the 0.19.9 release page, remember to set one of these to allow setting atlantis/apply as a requirement. The following is documented by @chroju in PR #2568.

ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY=true

or flag

--gh-allow-mergeable-bypass-apply

Just wanted to mention that the name for the environment variable is ATLANTIS_GH_ALLOW_MERGEABLE_BYPASS_APPLY since I lost a bit of time until I realized the proper name while checking the documentation here 😄

@jamengual
Copy link
Contributor

GHAllowMergeableBypassApply = "gh-allow-mergeable-bypass-apply" //

@jamengual
Copy link
Contributor

#2583

@nitrocode
Copy link
Member

@rayterrill @briancurt @minamijoyo apologies for the incorrect env var. I found out the hard way as well. Please make sure to use the correct input. The docs have the correct input flag and name now.

@Yasmine92
Copy link

Yasmine92 commented Oct 19, 2022

@jamengual, @rayterrill :

{"level":"warn","ts":"2022-10-19T13:01:14.604Z","caller":"events/apply_command_runner.go:114","msg":"unable to get pull request status: fetching mergeability status for repo: ***/***, and pull number: 241: getting pull request status: getting required status checks: GET https://api.github.com/repos/***/****/branches/main/protection: 404 Not Found []. Continuing with mergeable and approved assumed false","json":{"repo":"***/***","p
ull":"241"},"stacktrace":"github.com/runatlantis/atlantis/server/events.(*ApplyCommandRunner).Run\n\tgithub.com/runatlantis/atlantis/server/events/apply_command_runner.go:114\ngithub.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunComme
ntCommand\n\tgithub.com/runatlantis/atlantis/server/events/command_runner.go:296"} 

does the user used to run atlantis needs extra permissions in the repository to be able to check if atlantis/apply is part of the required checks?
this feature unfortunately only worked, when I gave the atlantis user admin permissions :(

@alexlo03
Copy link

@Yasmine92 Yes to use the feature the Atlantis GH user needs more permissions

// GetCombinedStatusMinusApply checks Statuses for PR, excluding atlantis apply. Returns true if all other statuses are not in failure.
func (g *GithubClient) GetCombinedStatusMinusApply(repo models.Repo, pull *github.PullRequest, vcstatusname string) (bool, error) {
//check combined status api
status, _, err := g.client.Repositories.GetCombinedStatus(g.ctx, repo.Owner, repo.Name, *pull.Head.Ref, nil)

Choose a reason for hiding this comment

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

I think this might be #2601 - here the repo.Owner and repo.Name come from the PR Destination repository, and the pull.Head.Ref may be the branch name in the Fork (Source repository)

From https://docs.github.com/en/rest/commits/statuses#get-the-combined-status-for-a-specific-reference it's not clear how to do this for commits on a fork.

krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
…antis#2436)

* Make required apply work with mergeable

* Fix lint hint

* Migrate mergeable approval check to graphql, update tests

* Adding support for check suites

* Adding feature flag protection for new functionality

* Fix linting falsepos

* Adjusted logic to handle required check runs, added doc
@nitrocode nitrocode added this to the v0.19.9 milestone Jan 13, 2023
@nitrocode nitrocode changed the title Make required atlantis/apply status check work with mergeable Make required atlantis/apply status check work with mergeable using --gh-allow-mergeable-bypass-apply Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement provider/github
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants