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

require-approval option is deceptive #155

Closed
sagikazarmark opened this issue Jun 21, 2018 · 2 comments
Closed

require-approval option is deceptive #155

sagikazarmark opened this issue Jun 21, 2018 · 2 comments

Comments

@sagikazarmark
Copy link
Contributor

On github anyone who can see the repo (even with read permission) can submit reviews to a pull request, even approve them, but that won't count into required reviews, PR cannot be merged until someone with write permission reviews the PR.

As far as I can see the current implementation will simply go through all the submitted reviews and checks if at least one of them is APPROVED.

This is a bit deceptive, because one would assume that Atlantis checks if according to Github the PR is reviewed or not.

Also, this seems to be the only access control possibility based on the documentation for now, and it doesn't work as one would assume.

For the short term clarifying this in the README would probably be nice (happy to provide a PR), for the long term it would be nice to provide some access control features (maybe check if a github PR is properly approved).

Unfortunately, after a quick look at Github's API, the only way to do that for now is checking the mergability of the PR, but that includes every other configured status checks (which might not be a bad thing, but that's not about approval anymore, so won't fit into this option).

@lkysow
Copy link
Member

lkysow commented Jun 21, 2018

@sagikazarmark I think an update to the readme is a good idea for now. Then we can add another approval option called "mergeable" later.

@sagikazarmark
Copy link
Contributor Author

Please see the linked PR.

jamengual pushed a commit that referenced this issue Nov 23, 2022
* Remove force apply feature flag

* Fix tests for force apply flag removal

* Fix tests
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

No branches or pull requests

2 participants