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

Diverge warning #867

Merged
merged 2 commits into from
Nov 27, 2019
Merged

Diverge warning #867

merged 2 commits into from
Nov 27, 2019

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Nov 27, 2019

Warn when using checkout-strategy=merge and the branch we're merging into has been updated after we've cloned it.

Takes commits from #815 and slightly refactors
Fixes #804

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #867 into master will increase coverage by <.01%.
The diff coverage is 72.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
+ Coverage   71.86%   71.87%   +<.01%     
==========================================
  Files          65       65              
  Lines        5222     5244      +22     
==========================================
+ Hits         3753     3769      +16     
- Misses       1184     1188       +4     
- Partials      285      287       +2
Impacted Files Coverage Δ
server/events/markdown_renderer.go 92.5% <ø> (ø) ⬆️
server/events/models/models.go 75% <ø> (ø) ⬆️
server/events/project_command_runner.go 74.33% <100%> (+0.22%) ⬆️
server/events/project_command_builder.go 82.12% <100%> (ø) ⬆️
server/events/working_dir.go 74.28% <69.69%> (-0.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54d677a...889d584. Read the comment docs.

MRinalducci and others added 2 commits November 27, 2019 10:52
- only check for divergence if using checkout strategy merge
@MRinalducci
Copy link
Contributor

I have to admit this is a nice refactor 👍

@lkysow
Copy link
Member Author

lkysow commented Nov 27, 2019

I have to admit this is a nice refactor 👍

Haha thanks! I would have preferred to work with you on it via review but I don't have much time to work on Atlantis these days so figured it's better to go ahead and get it merged asap.

@MRinalducci
Copy link
Contributor

Haha thanks! I would have preferred to work with you on it via review but I don't have much time to work on Atlantis these days so figured it's better to go ahead and get it merged asap.

Don't worry, I think it was more efficient like that and the goal was to get it merged asap.

finnag added a commit to finnag/atlantis that referenced this pull request Mar 3, 2023
The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979
finnag added a commit to finnag/atlantis that referenced this pull request Mar 21, 2023
The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979
nitrocode pushed a commit that referenced this pull request Mar 25, 2023
…n base update (#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects #804, #867, #979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <[email protected]>
nitrocode pushed a commit that referenced this pull request May 5, 2023
…n base update (#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects #804, #867, #979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <[email protected]>
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
…n base update (runatlantis#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <[email protected]>
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
…n base update (runatlantis#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <[email protected]>
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

Successfully merging this pull request may close these issues.

checkout-strategy merge : checking if it's at the right commit not working
2 participants