fix: Propagate git errors in divergence checks to abort plan#6632
Open
bhavith wants to merge 1 commit into
Open
fix: Propagate git errors in divergence checks to abort plan#6632bhavith wants to merge 1 commit into
bhavith wants to merge 1 commit into
Conversation
bhavith
changed the title
dosubot
Bot
added
bug
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Atlantis’ working directory divergence checks to return and propagate git command errors so plan/apply can fail loudly instead of silently assuming divergence and potentially planning against stale code.
Changes:
- Change
WorkingDir.HasDiverged/HasDivergedFromPullHeadto return(bool, error)and propagate errors from divergence checks. - Update callers and mocks to handle the new return signature.
- Update unit tests to accommodate the signature change.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/events/working_dir.go | Changes divergence APIs to return errors and updates merge recheck logic. |
| server/events/working_dir_test.go | Updates divergence tests for new (bool, error) signature. |
| server/events/undiverged_project_impact_test.go | Updates mocked divergence expectations for new signature. |
| server/events/project_command_runner_test.go | Updates mocked divergence expectations for new signature. |
| server/events/mocks/mock_working_dir.go | Regenerates/updates mock methods for new signature. |
| server/events/mock_workingdir_test.go | Updates test mock implementation for new signature. |
| server/events/command_requirement_handler.go | Updates undiverged requirement divergence plumbing for new signature. |
| server/events/command_requirement_handler_test.go | Updates mocked divergence expectations for new signature. |
Files not reviewed (2)
- server/events/mock_workingdir_test.go: Generated file
- server/events/mocks/mock_working_dir.go: Generated file
Comment on lines
+222
to
+225
| diverged, err := w.recheckDiverged(logger, p, headRepo, cloneDir) | ||
| if err != nil { | ||
| return true, err | ||
| } |
Author
There was a problem hiding this comment.
Idea is to return divergence if there is a failure, along with the error.
Comment on lines
410
to
414
| outputStatusUno, err := statusUnoCmd.CombinedOutput() | ||
| if err != nil { | ||
| logger.Warn("getting repo status has failed: %s", string(outputStatusUno)) | ||
| return true | ||
| return true, nil | ||
| } |
Author
There was a problem hiding this comment.
If git status failed, we dont have to fail the plan. Just responding divergence true is enough.
Comment on lines
112
to
117
| if err != nil { | ||
| ctx.Log.Warn("evaluating undiverged requirement has failed, falling back to full divergence check: %s", err) | ||
| diverged = a.hasDiverged(repoDir, ctx, cmd, nil) | ||
| // Here it doesn't matter if diverged check has errored. If `true` is returned, | ||
| // diverged path will be followed which will ask the user to rebase | ||
| diverged, _ = a.hasDiverged(repoDir, ctx, cmd, nil) | ||
| } |
Author
There was a problem hiding this comment.
That is fine. This change is relevant for MergeableRequirement. For UnDivergedRequirement, a failure will return divergence and then the user has to rebase the branch.
Comment on lines
336
to
338
| if len(autoplanWhenModified) > 0 { | ||
| return w.hasDivergedForPatterns(logger, cloneDir, projectPath, autoplanWhenModified, pullRequest, w.getDivergedFiles) | ||
| return w.hasDivergedForPatterns(logger, cloneDir, projectPath, autoplanWhenModified, pullRequest, w.getDivergedFiles), nil | ||
| } |
Author
There was a problem hiding this comment.
Idea is to fail plan if git mutation failed. Not for all errors.
Comment on lines
355
to
357
| if len(autoplanWhenModified) > 0 { | ||
| return w.hasDivergedForPatterns(logger, cloneDir, projectPath, autoplanWhenModified, pullRequest, w.getDivergedFilesFromPullHead) | ||
| return w.hasDivergedForPatterns(logger, cloneDir, projectPath, autoplanWhenModified, pullRequest, w.getDivergedFilesFromPullHead), nil | ||
| } |
Author
There was a problem hiding this comment.
Idea is to fail plan if git update or git fetch have failed. Not for all github actions.
bhavith
changed the title
Signed-off-by: bhavith <bhavith.vijayan@cognite.com>
Member
|
please create a bug report with the atlantis config first. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
Propagate errors from hasDiverged and recheckDiverged so that if they fail, the plan is also marked as failed.
why
Currently if there are errors while checking divergence or updating remote branch, divergence is assumed. The error itself is not returned and hence hidden. In such scenarios, fixing divergence updates local repo with outdated remote and that causes stale plan which goes unnoticed.
With this change if the checks fail, the plan is also failed to ensure the user is aware.
tests