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

Automatically merge when all apply's complete #186

Closed
lkysow opened this issue Jul 17, 2018 · 30 comments
Closed

Automatically merge when all apply's complete #186

lkysow opened this issue Jul 17, 2018 · 30 comments
Labels
feature New functionality/enhancement

Comments

@lkysow
Copy link
Member

lkysow commented Jul 17, 2018

Provide an option where Atlantis would merge the pull request automatically if all the apply's succeeded.

@rngtng
Copy link
Contributor

rngtng commented Aug 1, 2018

isn't there even a (web) hook before merging? this way atlantis would apply the plan on merge. if apply fails, the merge gets aborted?

@rngtng
Copy link
Contributor

rngtng commented Aug 1, 2018

ah just read the docs, aborting merge wouldn't work of course, bummer.. be maybe revert and re-open pr?

@lkysow
Copy link
Member Author

lkysow commented Aug 1, 2018

Reverting wouldn't work because Terraform will partially apply all the changes it can. If you reverted then the code wouldn't represent what was partially applied.

I think running apply prior to merge is ok because we lock the repo against modifications from other PRs (for that state).

@mootpt
Copy link
Contributor

mootpt commented Aug 17, 2018

Could you do this using custom workflows? Something like:

  myworkflow:
    apply:
      steps:
      - apply
      - run: ./auto_merge.sh

I know ATLANTIS_GH_USER and ATLANTIS_GH_TOKEN are available for making the API calls to GitHub, but need a way of determining the head of the PR, etc.

@mootpt
Copy link
Contributor

mootpt commented Aug 17, 2018

I guess what I am getting at is, If BaseRepo, HeadRepo, and Pull were exposed as environment variables in the RunStepRunner run function a lot of possibilities open up. Maybe lower hanging fruit?

https://github.com/runatlantis/atlantis/blob/master/server/events/runtime/run_step_runner.go#L32-L37

@lkysow
Copy link
Member Author

lkysow commented Aug 17, 2018

Yeah that's definitely doable. But if there were multiple projects in one repo you'd want to check if there were unapplied plans.

I think this is better implemented in the core.

@mootpt
Copy link
Contributor

mootpt commented Aug 18, 2018

agreed. any issue with #229? Here is how I am using it:

atlantis.yml section:

workflows:
  custom:
    apply:
      steps:
        - apply
        - run: ./merge_script.sh

merge_script.sh:

curl -sS "https://api.github.com/repos/$BASE_REPO_OWNER/$BASE_REPO_NAME/merges" \
-XPOST \
  -H "Authorization: token $ATLANTIS_GH_TOKEN" \
  -d "{\"base\": \"master\", \"head\": \"$HEAD_BRANCH_NAME\"}"

@lkysow
Copy link
Member Author

lkysow commented Aug 18, 2018

Looks good to me! Only thing I'd do is not pipe stderr to /dev/null in case there's an error in the API call. I think curl -sS gives you error output without the other stuff?

@mootpt
Copy link
Contributor

mootpt commented Aug 18, 2018

Updated PR and my example above.

@mootpt
Copy link
Contributor

mootpt commented Aug 28, 2018

User at your own risk: The above should only be used for single project applies. Otherwise the script will likely result in undesired behavior. If your pr is touching multiple projects the first apply will result in merging of the pr.

@brndnmtthws
Copy link

I will take a stab at implementing this, if nobody else is working on it.

@sstarcher
Copy link

@brndnmtthws please do

@majormoses
Copy link
Contributor

Should we support the "merge" options? For example on our repos we only allow squash/merge and rebase options as the straight merge option does not fast forward. Would we want to provide some basic rules around it? For example:

if commits > 1
  squash
else
  rebase

@brndnmtthws
Copy link

@majormoses good question, that was something I was going to ask about. Initially I think it should just default to whatever the repo uses.

Another question: how should we handle the locks with merging? Should the locks be released before or after the merge succeeds? In my case, the main reason for wanting to implement this feature is to avoid conflicts between PRs, particularly in cases where changes are applied but not merged into master. So if atlantis releases the locks, but a merge fails for some reason, we wind up with the same problem: someone could get the repo into an unclean/conflicted state.

@sstarcher
Copy link

I would not release the lock until after the merge.

@brndnmtthws
Copy link

Another question: it's possible to apply specific projects individually on a PR. What should be the behaviour if only one project is applied but multiple projects have been modified in a given PR?

The logic is around here: https://github.com/runatlantis/atlantis/blob/master/server/events/project_command_builder.go#L252-L264

The easy thing to do is to only merge the PR when all plans are applied. However this wouldn't be great if someone decides to apply multiple plans individually. A more complex, and probably better alternative would be to check that all plans have been applied, and then merge the PR.

Thoughts? I'll try and figure out how to implement it so we wait until every plan has been applied, either individually or all at once.

@brndnmtthws
Copy link

One more Q: autoplan can be specified per project, however automerge should probably be global only. If I want to enable automerge, I would want to enable it for the entire repo. It doesn't make sense (to me, at least) to have automerge configurable per-project. Thoughts?

@sstarcher
Copy link

I would think if you would want autoplan per project you would also want automerge per project, but overall I don't see why people would want autoplan at the project level.

@sstarcher
Copy link

For the targeted apply approach I think I would skip supporting that and just ignore it as a use-case and only support auto merge on the atlantis apply.

@brndnmtthws
Copy link

It seems confusing to me to specify automerge at the per-project level. Autoplan I get, but not automerge. You can have N projects per PR, so specifying whether each one should be merged individually doesn't make sense (it's a 1 to N mapping).

@sstarcher
Copy link

@brndnmtthws ok, that does make sense.

@majormoses
Copy link
Contributor

I think it makes sense to only auto merge if all plans have been properly applied.

At work we have repos for our aws, pagerduty, and github management with terraform. Some repos have multiple projects/directories in them and others do not. As long as we can differentiate behavior at a repo level I think we are good as that should allow us to treat different types of resources differently.

@lkysow
Copy link
Member Author

lkysow commented Dec 12, 2018

To answer the questions raised:

  1. I can think of reasons why users might want to specify automerge on a per-project basis but I
    still think it makes sense to configure this as the repo level.

    Users might want to configure it on a per-project basis if they're using a monorepo since some
    projects might need different workflows.

    This would make implementation difficult though because you'd need to know which projects have already been planned, what their settings are, etc.

  2. I think this should be configured as follows:

     version: 2
     automerge: true
     projects: ...
    

    And via a --automerge flag.

  3. In GitHub, their API for merging is like you clicked the Merge Button so I think it will use whatever merge strategy you've specified, however this needs to be tested: https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button. We should try to honour the merge method that is configured for the repo.

  4. I think all the plans need to be applied before we automerge. There's already a method to get unapplied plans so that's not too hard.

@brndnmtthws
Copy link

Sweet, sounds good. I'm nearly finished with the PR (excluding tests & testing), so hopefully I'll have it ready by the end of the day.

@lkysow
Copy link
Member Author

lkysow commented Dec 12, 2018

I would think if you would want autoplan per project you would also want automerge per project, but overall I don't see why people would want autoplan at the project level.

@sstarcher people want autoplan at the per-project level because of the when_modified clause. For example if you used the following structure:

.
├── modules
│   └── module1
│       ├── main.tf
│       ├── outputs.tf
│       └── submodule
│           ├── main.tf
│           └── outputs.tf
└── project1
    └── main.tf
└── project2
    └── main.tf

And you wanted:

  • project1 to be planned when module1 changes
  • project2 to be planned only when files inside project2/ change

Then you would need to use a different when_modified setting per project:

version: 2
projects:
- dir: project1
  autoplan:
    when_modified: [*.tf, ../modules/module1/*.tf]
- dir: project2
  autoplan:
    when_modified: [*.tf]

@brndnmtthws
Copy link

Just submitted a first pass at the PR. Took a while to get all the tests working correctly, but it should work, so please test it out.

@kipkoan
Copy link
Contributor

kipkoan commented Jan 10, 2019

What is the use case for having Atlantis auto merge once all the plans in a PR have been applied successfully?

@sstarcher
Copy link

To stop the user from needing to merge it themselves. Once the applies are done it should be on master so master reflects reality.

@brndnmtthws
Copy link

To stop the user from needing to merge it themselves. Once the applies are done it should be on master so master reflects reality.

This precisely. It can be problematic when people don't remember to merge their PRs right away.

@kenerwin88
Copy link

Please for the love of all things, I need this feature terribly! 👍 👍

brndnmtthws pushed a commit to brndnmtthws/atlantis that referenced this issue Jan 17, 2019
Similar to autoplan, automerge is a feature whereby Atlantis will merge
a PR/MR once all plans have successfully been applied.

This addresses issue runatlantis#186.
@lkysow lkysow added the feature New functionality/enhancement label Apr 9, 2019
meringu pushed a commit to meringu/atlantis that referenced this issue May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement
Projects
None yet
Development

No branches or pull requests

8 participants