add flag for rebasing branch off master

[sstarcher]

This adds a flag to the CLI to have the PR rebased onto the master branch when the flag --rebase-repo is set.

I did not implement the configuration for the atlantis.yml as I was not sure if we would want that interaction where once rebased we would have to reread the atlantis.yml.

[sstarcher]

Fixes #35

[codecov]

Codecov Report

Merging #374 into master will decrease coverage by 0.08%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage    70.3%   70.22%   -0.09%     
==========================================
  Files          62       62              
  Lines        3742     3758      +16     
==========================================
+ Hits         2631     2639       +8     
- Misses        923      930       +7     
- Partials      188      189       +1

Impacted Files	Coverage Δ
server/events/models/models.go	89.61% <ø> (ø)	⬆️
cmd/server.go	79.16% <ø> (ø)	⬆️
server/events/command_runner.go	72.79% <100%> (+0.4%)	⬆️
server/server.go	66.14% <100%> (+0.13%)	⬆️
server/events/project_command_runner.go	81.88% <100%> (+0.14%)	⬆️
server/events/project_command_builder.go	83.16% <100%> (+0.26%)	⬆️
server/events/working_dir.go	58.73% <37.5%> (-7.94%)	⬇️

---

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 284fa2e...6f95fb6. Read the comment docs.

[sstarcher]

For this flag to work users must set a gitconfig that contains a email address. I was not sure the best way to document this since it's supplied optionally by however the user runs it.

[lkysow]

Would it make more sense to use the merge_commit_sha that GitHub provides? https://developer.github.com/v3/pulls/#response-1

This is more in-line with how Travis CI works: https://docs.travis-ci.com/user/pull-requests#how-pull-requests-are-built

Note that if the master branch changes then it will be out of date, however with locking, the master branch shouldn't be changing so that might be okay.

[sstarcher]

@lkysow using merge_commit_sha seems interesting, but to do this we would have to have a polling loop that waits for this to finish instead of doing the rebase ourselves and moving on. This would also be a github specific solution and would not necessarily work for all other VCS systems.

As for the second thing I would certainly prefer to always rebase and not assume that the master branch has not changed.

Thoughts?

[sstarcher]

It's now being tested and used in a few environments by me to validate it with real use cases.

I have a outstanding question if you are relying on the sideffect here or if this is being used to clone
https://github.com/runatlantis/atlantis/blob/master/server/events/project_command_runner.go#L139

[sstarcher]

Looks like those tests are failing because I removed the extra clone.

[sstarcher]

So looking at the tests that are failing now they are comment tests and with my recent changes it looks like it expects the repo to exist. So I need to revert my change to make the comment always clone.

[lkysow]

I'm curious if you tried to implement this using a custom workflow?

I'm not sure if it makes sense for the atlantis core to have this setting.

[sstarcher]

@lkysow What do you mean by custom workflows I'm not familiar.

[lkysow]

Using the custom run steps: https://www.runatlantis.io/guide/atlantis-yaml-use-cases.html#running-custom-commands

[sstarcher]

ahh I see using a custom workflow in atlantis. That seems like a viable way to accomplish it. I think my current complaint about the customization is I have to do it for each main.tf which in our case is about 30 as we still don't have a default workflow in atlantis correct?

[sstarcher]

I'll go ahead and close this if you prefer it implemented via the custom commands.

[lkysow]

Does it work with custom commands? I haven't tried it. Just wanted to know if it was an option. If it does work, then I think I would prefer that it be implemented that way to keep Atlantis more in-line with how circle-ci works. If it doesn't, I'm not against adding it as an option. This is how travis ci works. However it still has edge cases because master could change between plan and apply phases.

With the latest server-side config RFC, you can specify a default workflow on the server: https://docs.google.com/document/d/1w2wePfXGA3L151Af6D0kB1aKVB7pXys_l9Vk9SD7kyA so that will fix the issue of having to write it multiple times.

[sstarcher]

@lkysow sounds good I look forward to that RFC being implemented. I'll test it out and get back to you, but I suspect it will work.

[majormoses]

I personally think this should not come from a custom workflow and should be a first class feature in atlantis as its something like realistically most people want and having to have 99% of people setup custom workflows for a generally desired behavior does not seem like the best experience to me.

[lkysow]

I'm not certain that it's something everyone wants. For instance in CircleCI, it runs against the branch that's in the pull request.

The issues I see with rebasing:

1. Due to how a rebase might work, you could see changes in your plan that don't show up in the diff on GitHub. This is unintuitive.
2. With Atlantis, projects are locked against concurrent changes, so rebasing your branch off of master should have no effect on the plan/apply output since that project should not have been able to be modified thanks to the in-progess pull request having the lock. You could still run into a problem if the lock is discarded, another PR is merged, and then you come back to your first PR. But I don't think automatically rebasing and hiding that fact from the user is the most intuitive way to handle this.
3. Between planning and applying you can get out of sync: I generate a plan, then someone pushes something to master, now what do I do if someone runs apply? If I apply the generated plan, then I'm running apply on a branch that's not been rebased with master. If I run an apply after doing another rebase then I'm applying a plan that I haven't reviewed.

I can see a benefit of the Atlantis core making this behaviour configurable and letting users opt-in to the behaviour they desire. But isn't that already available with the custom workflows? I'm open to making it easier to customize this behaviour through a new config schema vs. using the workflows.

[sstarcher]

1. Yes, but you will also revert anything that has been applied to master. Which I consider even worse.
2. The main use-case I have seen on a day to day basis is someone is not up to date with master. That person puts in a PR and now the plan/apply wants to revert work that has already been merged to master.
3. The best safety against this is to consider storing the plan. Although I don't think that is necessary for this PR. You currently have this same issue. If changes are made on master after a plan you and a apply is done you will apply those changes and not just the changes that showed in the plan. This also happens if someone made a change directly against the backing resources.

[lkysow]

The best safety against this is to consider storing the plan. Although I don't think that is necessary for this PR. You currently have this same issue. If changes are made on master after a plan you and a apply is done you will apply those changes and not just the changes that showed in the plan. This also happens if someone made a change directly against the backing resources.

Atlantis does store the plan right now. Terraform will error out if the state has been update after the plan was created. I think in the case of a rebase, it should do the initial rebase on plan and store the plan. It shouldn't rebase on apply again. That should still solve the issue you're talking about.

The main use-case I have seen on a day to day basis is someone is not up to date with master. That person puts in a PR and now the plan/apply wants to revert work that has already been merged to master.

Yeah I can see how that would be annoying and also happen often.

Sounds like we need a configurable checkout behaviour that would also allow people to use the github merge commit if they'd rather. Something like --checkout={branch,merge,rebase}?

Is there any reason people would want to configure this on a per-repo basis rather than a global flag?

[sstarcher]

I would think they would want the same workflow on all repos and I agree doing it on the plan makes sense and not doing any rebase on a apply.

[kipkoan]

I like the idea of Atlantis merging in master to the branch, and committing it back, before the plan. I would never want it to rebase and commit since we'd lose the commits the developer did. So the code review in the PR wouldn't be clear.

[lkysow]

I like the idea of Atlantis merging in master to the branch, and committing it back, before the plan. I would never want it to rebase and commit since we'd lose the commits the developer did. So the code review in the PR wouldn't be clear.

I don't think the plan was for it to commit the rebase, just do the rebase and use the resulting code for running all the commands.

If that's the behaviour you'd like @kipkoan then I think if it used the --check=merge strategy, it would use the special merge_commit that GitHub creates that is what the code will look like once it's merged.

[kipkoan]

I don't think the plan was for it to commit the rebase, just do the rebase and use the resulting code for running all the commands.

Oh! So Atlantis would run code that is different than what the reviewer of the PR sees in that PR? That sounds like a bad idea to me. I'd rather just be notified that the PR code doesn't have the latest master code merged. I'd rather force the PR to merge master before even being notified that I have a PR to review.

[sstarcher]

@kipkoan Jenkins/Travis and other systems all have support for this.

[kipkoan]

A build is pretty much 100% safe. Applying terraform is not. It seems safer to me for the reviewer to see the exact terraform that is being plannned before applying it.

But, maybe I'm being too cautious, and there aren't any possibly bad repercussions.

[sstarcher]

Terraform runs off the desired state. What is in master should be the desired state so based on that you should always have your changes rebased on master.

[kipkoan]

Rebasing doesn't matter really. Merging is just as good here. What I want is to have master always reflect what is applied.

[sstarcher]

If rebasing vs merging does not matter to you then feel free to ignore that distinction. Without support for this someone could apply a PR and after that PR gets merged to master it may differ.

[majormoses]

Due to how a rebase might work, you could see changes in your plan that don't show up in the diff on GitHub. This is unintuitive.

Yes, but you will also revert anything that has been applied to master. Which I consider even worse.

Agreed with shane this often this means resource destruction

With Atlantis, projects are locked against concurrent changes, so rebasing your branch off of master should have no effect on the plan/apply output since that project should not have been able to be modified thanks to the in-progess pull request having the lock. You could still run into a problem if the lock is discarded, another PR is merged, and then you come back to your first PR. But I don't think automatically rebasing and hiding that fact from the user is the most intuitive way to handle this.

At my org we have 180+ people in R&D (not all are engineers we have groups such as PMs, data scientists, etc) submitting pull requests to manage github access (creating repos, updating required ci contexts, etc) with varying skillsets and are not generally proficient at terraform and/or git. This morning I had to checkout someone's branch and rebase on their behalf because they could not seem to figure it out, I'd like this to automatically happen assuming no conflicts. If there are conflicts then we can comment a message to the user telling them they need to rebase and fix it manually.

Between planning and applying you can get out of sync: I generate a plan, then someone pushes something to master, now what do I do if someone runs apply? If I apply the generated plan, then I'm running apply on a branch that's not been rebased with master. If I run an apply after doing another rebase then I'm applying a plan that I haven't reviewed.

I think we should handle this situation by rebasing, discarding the review, and re-run the plan. We do not need to rebase on apply.

Terraform runs off the desired state. What is in master should be the desired state so based on that you should always have your changes rebased on master.

Exactly, on most of my terraform projects (using atlantis and jenkins) I have to rebase an average of 5+ times a day to stay in sync. Either we should only apply from master or we should auto rebase from master, force push the changes (I can easily be talked out of that but I guess my question is why there is resistance to it as it makes a cleaner history and is safer to merge back), discard any reviews, plan, and then follow what we are already doing.

Rebasing doesn't matter really. Merging is just as good here.

From a terraform/atlantis perspective its not a issue but if you like clean git history, merges are uglier as it puts history out of order.

[majormoses]

Another option which is supported by https://dependabot.com is to make it an atlantis command for example @atlantis rebase see: sensu-plugins/sensu-plugins-graphite#69 (comment) for an example