-
Notifications
You must be signed in to change notification settings - Fork 34
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
Merge blacklist PRs manually using merge=union #284
Conversation
This closes Charcoal-SE/SmokeDetector#776. |
app/controllers/github_controller.rb
Outdated
`git checkout master; git pull origin master` | ||
`git fetch origin #{ref}` | ||
`git merge origin/#{ref} --no-ff -m 'Merge pull request ##{pr_num} from Charcoal-SE/#{ref} --autopull'` | ||
`git push origin master` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an Octokit equivalent for each of these commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sadly. The Github API does not respect the merge=union
Git attribute when performing merges (otherwise I could just drop a .gitattributes
in the SmokeDetector repo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArtOfCode- Is there an |
That'll be the email, since it's used to log in |
app/controllers/github_controller.rb
Outdated
|
||
`git checkout master; git pull origin master` | ||
`git fetch origin #{ref}` | ||
`git merge origin/#{ref} --no-ff -m 'Merge pull request ##{pr_num} from Charcoal-SE/#{ref} --autopull'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String interpolation here makes me nervous. Is it at all possible to parameterize these in some way? If the GH secret gets out, this could be a direct path to full shell access. Otherwise, looks great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change it to a system
call (which also won't capture stdout as a bonus)
app/controllers/github_controller.rb
Outdated
@@ -213,12 +213,32 @@ def pullapprove_merge_hook | |||
return | |||
end | |||
|
|||
unless Dir.exist?('SmokeDetector') | |||
`git clone https://github.com/Charcoal-SE/SmokeDetector` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to switch this over to SSH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
This prevents the usual merge conflicts created when another blacklist PR is merged first.