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

Add the ability to automatically merge downstream PRs created by modulesync #149

Closed
wants to merge 6 commits into from

Conversation

joestump
Copy link
Contributor

@joestump joestump commented May 2, 2018

What does this PR do?

  • Introduces a manage_pr function. Rubocop was (rightfully) complaining about the complexity of this block and brings the PR code a bit more in line with the existing manage_${thing} pattern.
  • Introduces --pr-auto-merge to automatically merge downstream PRs created by Modulesync.
  • Excludes spec/ from Rubocop checks.
  • Switches to $stdout and $stderr, which worked best with RSpec's output() checks.

Why is this change being made?

My team and I are so lazy we can't be bothered to manually merge all of the PRs our robots make. This lets our robots do it for us. 🤖😂

How was this tested? How can the reviewer verify your testing?

We're still testing this. Please consider this a work in progress until I've confirmed later.

What gif best describes this PR or how it makes you feel?

…lexity. Introduce the --pr-auto-merge option and add relevant docs.
@ekohl
Copy link
Member

ekohl commented May 16, 2018

I'm not sure I follow exactly what this has for a check. Do I understand it correctly that there's no check if the CI passed before merging? In that case, what is the point of a PR rather than a straight commit? You can even auto close it by fast forward merging it in master and pushing master.

@ekohl
Copy link
Member

ekohl commented Jul 9, 2018

@joestump could you have a look at my comment?

@joestump
Copy link
Contributor Author

@ekohl I can add checks for CI via the statuses API. We have branch protections on master for most/all of our repos. I guess we could let our robots push to master though.

@bittner
Copy link
Contributor

bittner commented Mar 26, 2019

I feel this is a valuable change that makes sense. It's optional to activate, so we don't break existing functionality. 👍

@joestump, would it be hard to add a few tests for your implementation? Tests would be great!

I would merge the changes afterwards unless someone wants to veto against it.

@joestump
Copy link
Contributor Author

@bittner I'm not super great with Ruby testing. If you could point me at a PR I can crib from, I'll take a look at mocking something up.

@bastelfreak
Copy link
Member

Hi. Thanks for this awesome PR!
The Vox Pupuli release workflow currently looks like this:
A user runs a rake task locally which will create a git tag and a new commit on master. This will be pushed. Based on that we cannot set our branches on github.com to protected. The option to honour the status API would be very important. Is that something you could implement as well?

@joestump
Copy link
Contributor Author

@bittner tests added. @bastelfreak would it be possible to follow up with that functionality in a later PR? How should that function? Should it simply skip? Poll while waiting on status? Other?

@ekohl
Copy link
Member

ekohl commented Oct 23, 2019

I feel this feature doesn't fit within the tool and that makes me slightly against it. I'd prefer a tool that you can feed PRs, monitor them and merge when green. To me that's more the unix philosophy.

Note Gitlab has this built in but GH needs some special attention.

@joestump
Copy link
Contributor Author

Going to close this out. Relevant tests have been copied over to #171.

@joestump joestump closed this Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants