search

runatlantis / atlantis

Terraform Pull Request Automation
https://www.runatlantis.io
Other
7.52k stars 1.02k forks source link

fix: Preventing merging without running atlantis apply on Gitlab #4651

Closed shurkus closed 2 weeks ago

shurkus commented 3 weeks ago

what

Preventing merging without running atlantis apply on Gitlab

why

Preventing merging without running atlantis apply on Gitlab

tests

https://github.com/runatlantis/atlantis/issues/4372

chenrui333 commented 3 weeks ago

I have fixed the tests per https://github.com/runatlantis/atlantis/pull/4653 refreshed the pr and rerun

chenrui333 commented 3 weeks ago

@shurkus please rebase your PR to the latest main branch. (I actually cannot refresh your branch due to permission denied issue)

For future pull requests, please allow maintainers to edit your PR to simplify the merge process.

shurkus commented 3 weeks ago

@chenrui333 rebased also allow edit

chenrui333 commented 2 weeks ago

Thanks @shurkus!

Unibozu commented 2 days ago

Hi @shurkus @chenrui333

I wanted to highlight this feels like a breaking change in the way atlantis works with Gitlab Merge Requests, not a fix. This should be behind a feature flag, documented and fully tested (not just a "it works on my machine").

Especially as it's a weird workaround (the apply steps will always look like it's "running") with Gitlab lacking a way to enforce passing a certain CI job to merge an MR. The other difference is that on Github I can control which repositories require this enforcement, while this change will make all MRs on Gitlab requiring an apply before becoming mergeable.

Can we consider to revert the changes until this is better implemented?

lukemassa commented 1 day ago

I agree that this should be reverted as we come up with a more holistic solution here, for a few reasons:

  1. This is a breaking change that changes behavior, not a fix
  2. I'm not sure why this is implemented only for auto plans and not for comment plans
  3. This makes it look like atlantis apply is "stuck", which is best case confusing, and worst case might have some other effects (e.g. interact poorly with mergeable)

I think one issue is the semantics of "pending" are different in gitlab and github, and I think we should either commit to unifying the two notions, or acknowledging they are different and document them differently.

I definitely want to address these issues, and maybe pre-emptively making "apply" be running is part of that solution, but I think it requires more discussion