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

Detects if master branch has diverged and warn to pull new commits #815

Closed
wants to merge 7 commits into from

Conversation

MRinalducci
Copy link
Contributor

@MRinalducci MRinalducci commented Oct 26, 2019

Hi Luke,

I've worked on implementing a warning in the comment if master has changed like as I suggested in issue #804.

There is an example at MRinalducci/atlantis-example#1 (comment).
It adds message: ":warning: Master branch is ahead, it is recommended to pull new commits first."

Can you please give me your inputs about this PR?
I think about to implement a "pull/merge" command. What do you think?

@codecov
Copy link

codecov bot commented Oct 26, 2019

Codecov Report

Merging #815 into master will decrease coverage by 0.01%.
The diff coverage is 76.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
- Coverage   71.83%   71.82%   -0.02%     
==========================================
  Files          65       65              
  Lines        5213     5203      -10     
==========================================
- Hits         3745     3737       -8     
+ Misses       1183     1181       -2     
  Partials      285      285
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 83.01% <100%> (+0.89%) ⬆️
server/events/working_dir.go 75% <73.07%> (ø) ⬆️
server/events/yaml/parser_validator.go 96.66% <0%> (-0.15%) ⬇️

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 f691563...1981a67. Read the comment docs.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Nice work, I think this is a good approach. You need to add tests for both working_dir and markdown_renderer that exercise the new functionality.

@MRinalducci
Copy link
Contributor Author

Hi,
Thank you for your review and comment.
Ok no problem, I'll add tests for both files.

@MRinalducci
Copy link
Contributor Author

Hi @lkysow,
As you asked I added the tests for working_dir and markdown_renderer.
I'm coming just too late for getting in the current release.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Does this work? I'm seeing that the status is just looking at the remote tracking branch which isn't master but the same branch:

$ git status
On branch autoplan
Your branch is up to date with 'origin/autoplan'.

nothing to commit, working tree clean

Even when master is ahead.

server/events/working_dir.go Outdated Show resolved Hide resolved
server/events/working_dir.go Outdated Show resolved Hide resolved
server/events/working_dir.go Outdated Show resolved Hide resolved
server/events/working_dir.go Outdated Show resolved Hide resolved
server/events/working_dir.go Outdated Show resolved Hide resolved
}
status := strings.Trim(string(outputStatusUno), "\n")
hasDiverged := strings.Contains(status, "have diverged")
if hasDiverged {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we skip this section if any of the above commands fail? Maybe pull it into its own function so you can return early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it like this because I don't want to impact the clone function if for any reason it cannot detected that master branch has diverged as it displays only a warning in the comment of the plan.

@MRinalducci
Copy link
Contributor Author

Yes this is working, did you tried with argument --checkout-strategy=merge ?

I got this:

$ git status --untracked-files=no
On branch master
Your branch and 'origin/master' have diverged,
and have 2 and 1 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

@lkysow lkysow mentioned this pull request Nov 27, 2019
@lkysow
Copy link
Member

lkysow commented Nov 27, 2019

Closed by #867 (I took all your commits and made some small changes, awesome work! 🎉 )

@lkysow lkysow closed this Nov 27, 2019
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.

3 participants