Skip to content

Conversation

FrancoisPoinsot
Copy link
Contributor

@FrancoisPoinsot FrancoisPoinsot commented Jun 4, 2025

what

This PR has introduced an issue: #5264
When using gh app, with merge checkout policy: atlantis silently fails the merge.

You can easily spot it running this command in the git repo folder, after running atlantis plan:

> git ls-remote --get-url
https://:@github.com/cognitedata/terraform-test.git

why

Clone() logic for gh app has a small fix for the repo URL, to remove the empty credentials in the URL.
recheckDiverged(), called by MergeAgain(), needs the same fix because it resets the repo URL, as a way to ensure the token generated by the gh-app is refreshed.

But since the PR above, MergeAgain() has moved out of Clone(), so the fix on repo URL was not applied anymore.

tests

I am using this commit in the fork of atlantis I am running. This has solved the merge issue.

also ran

docker run --rm -v $(pwd):/go/src/github.com/runatlantis/atlantis -w /go/src/github.com/runatlantis/atlantis ghcr.io/runatlantis/testing-env:latest make test-all
make  fmt lint

notes

  1. The issue is not part of a release yet. So that did not affect too many people. You might want to include this fix before the next release.

  2. recheckDiverged() failing silently is another problem that I have not addressed here. I might open another PR for this.

  3. I went for the most straightforward fix. In hope to get it merged before the next release. But having gh app logic spread over a few files sure makes this difficult to reason about.

@github-actions github-actions bot added go Pull requests that update Go code provider/github labels Jun 4, 2025
@dosubot dosubot bot added the bug Something isn't working label Jun 4, 2025
@FrancoisPoinsot FrancoisPoinsot marked this pull request as draft June 4, 2025 08:02
@FrancoisPoinsot FrancoisPoinsot changed the title Fix gh-app merge fix: gh-app merge Jun 4, 2025
MergeAgain() ends up calling recheckDiverged() which reset remotes URLs using CloneURL.

Github App workingdir needs to fix the CloneURL of both remotes. Until recently this logic was done one Clone(). But recently MergeAgain() logic has been moved out of Clone().
So the same fix needs to be applied there.

Without this fix, recheckDiverged() fails when running `git remote update`. Because it will use empty credentials.
Also this failure is done silently because it does not return an error.

Looking at FileWorkspace methods, we potentially also want to run the same fix for `HasDiverged()` which runs `git fetch`. But for now this function does not reset the remotes URLs the same way recheckDiverged() does.

Signed-off-by: francois poinsot <[email protected]>
Signed-off-by: francois poinsot <[email protected]>
@FrancoisPoinsot FrancoisPoinsot marked this pull request as ready for review June 4, 2025 08:13
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 4, 2025
Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

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

do you think you could add a test with a malformed url?

@jamengual
Copy link
Contributor

@FrancoisPoinsot Thanks for your contribution.

@jamengual jamengual merged commit a7c712b into runatlantis:main Jun 4, 2025
64 checks passed
@FrancoisPoinsot FrancoisPoinsot deleted the fix-gh-app-merge branch June 4, 2025 17:24
joe1981al pushed a commit to joe1981al/atlantis that referenced this pull request Jun 20, 2025
Signed-off-by: francois poinsot <[email protected]>
Signed-off-by: Joseph McDonald <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code lgtm This PR has been approved by a maintainer provider/github
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants