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.

Sign up for GitHub

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

Use repository API for merges when possible #585

Closed
wants to merge 3 commits into from
Closed

Use repository API for merges when possible #585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3922aaf
Use repository API for merges when possible
sstok Jul 30, 2016
3516212
Fix if statement missing return
sstok Jul 30, 2016
570485c
Allow disable merge-api
sstok Jul 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Adapter/Adapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,18 @@ public function getPullRequest($id);
*/
public function getPullRequestCommits($id);

/**
* Merges a pull-request by id.
*
* @param int $id
* @param string $message
*
* @throws AdapterException when merging failed
*
* @return string sha1 of the merge commit
*/
public function mergePullRequest($id, $message);

/**
* Updates the state of a pull-request by id.
*
Expand Down
64 changes: 54 additions & 10 deletions src/Command/PullRequest/PullRequestMergeCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
namespace Gush\Command\PullRequest;

use Gush\Command\BaseCommand;
use Gush\Config;
use Gush\Exception\CannotSquashMultipleAuthors;
use Gush\Exception\UserException;
use Gush\Feature\GitDirectoryFeature;
use Gush\Feature\GitRepoFeature;
use Gush\Helper\GitConfigHelper;
use Gush\Helper\GitHelper;
use Gush\Template\Pats\Pats;
use Gush\ThirdParty\Gitlab\Adapter\GitLabRepoAdapter;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
Expand Down Expand Up @@ -169,16 +171,20 @@ protected function execute(InputInterface $input, OutputInterface $output)
);
};

$mergeOperation = $gitHelper->createRemoteMergeOperation();
$mergeOperation->setTarget($targetRemote, $targetBranch);
$mergeOperation->setSource($sourceRemote, $sourceBranch);
$mergeOperation->squashCommits($squash, $input->getOption('force-squash'));
$mergeOperation->switchBase($input->getOption('switch'));
$mergeOperation->setMergeMessage($messageCallback);
$mergeOperation->useFastForward($input->getOption('fast-forward'));

$mergeCommit = $mergeOperation->performMerge();
$mergeOperation->pushToRemote();
if ($this->canUseApi($input)) {
$mergeCommit = $adapter->mergePullRequest($prNumber, $messageCallback($targetBranch, $sourceBranch));
} else {
$mergeOperation = $gitHelper->createRemoteMergeOperation();
$mergeOperation->setTarget($targetRemote, $targetBranch);
$mergeOperation->setSource($sourceRemote, $sourceBranch);
$mergeOperation->squashCommits($squash, $input->getOption('force-squash'));
$mergeOperation->switchBase($input->getOption('switch'));
$mergeOperation->setMergeMessage($messageCallback);
$mergeOperation->useFastForward($input->getOption('fast-forward'));

$mergeCommit = $mergeOperation->performMerge();
$mergeOperation->pushToRemote();
}

if (!$input->getOption('no-comments') && !$input->getOption('fast-forward')) {
$gitConfigHelper->ensureNotesFetching($targetRemote);
Expand Down Expand Up @@ -219,6 +225,44 @@ protected function execute(InputInterface $input, OutputInterface $output)
}
}

private function canUseApi(InputInterface $input)
{
if (true === $this->getConfig()->get('disable_merge_api', Config::CONFIG_ALL, false)) {
return false;
}

if ($input->getOption('squash') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

You can still squash commits using the preview github API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but that will complicate the Adapter API 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Not too much though. Probably worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions? You need to detect whether the Adapter support this operation.
But I don't like adding yet another interface like CanSquashPullRequestOnMerge 💀

Adding an extra arguments on the method is no problem btw 👍

$input->getOption('force-squash') ||
$input->getOption('switch') ||
$input->getOption('fast-forward')
) {
$this->getHelper('gush_style')->note(
[
'Extra merge options were provided, falling back to a local merge.',
'A local merge is slower then using the API, please wait till the command is finished.',
]
);

return false;
}

// Ugly hacks, GitLab doesn't provide a merge-hash at the moment.
// https://gitlab.com/gitlab-org/gitlab-ce/issues/20456
if (!$input->getOption('no-comments') && $this->getAdapter() instanceof GitLabRepoAdapter) {
$this->getHelper('gush_style')->note(
[
'GitLab (currently) doesn\'t return a merge hash making it impossible to add comments.',
'Use the `--no-comments` option to force usage of the API.',
'And please vote on https://gitlab.com/gitlab-org/gitlab-ce/issues/20456'
]
);

return false;
}

return true;
}

private function guardPullRequestMerge(array $pr)
{
if ('open' !== $pr['state']) {
Expand Down
20 changes: 20 additions & 0 deletions src/ThirdParty/Bitbucket/BitbucketRepoAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,26 @@ public function getPullRequestCommits($id)
return $commits;
}

/**
* {@inheritdoc}
*/
public function mergePullRequest($id, $message)
{
$response = $this->client->apiPullRequests()->accept(
$this->getUsername(),
$this->getRepository(),
$id,
['message' => $message]
);

$resultArray = json_decode($response->getContent(), true);
if ('MERGED' !== $resultArray['state']) {
throw new AdapterException($response->getContent());
}

return $resultArray['merge_commit']['hash'];
}

/**
* {@inheritdoc}
*/
Expand Down
21 changes: 21 additions & 0 deletions src/ThirdParty/Github/GitHubAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,27 @@ public function getCommitStatuses($org, $repo, $hash)
return $pager->fetchAll($this->client->api('repo')->statuses(), 'combined', [$org, $repo, $hash])['statuses'];
}

/**
* {@inheritdoc}
*/
public function mergePullRequest($id, $message)
{
$api = $this->client->api('pull_request');

$result = $api->merge(
$this->getUsername(),
$this->getRepository(),
$id,
$message
);

if (false === $result['merged']) {
throw new AdapterException('Merge failed: '.$result['message']);
}

return $result['sha'];
}

/**
* {@inheritdoc}
*/
Expand Down
10 changes: 10 additions & 0 deletions src/ThirdParty/Gitlab/Adapter/GitLabRepoAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,16 @@ public function getPullRequestCommits($id)
return [];
}

/**
* {@inheritdoc}
*/
public function mergePullRequest($id, $message)
{
$mr = $this->client->api('merge_requests')->show($this->getCurrentProject()->id, $id);
$mr = MergeRequest::fromArray($this->client, $this->getCurrentProject(), $mr);
$mr->merge($message);
}

/**
* {@inheritdoc}
*/
Expand Down
66 changes: 56 additions & 10 deletions tests/Command/PullRequest/PullRequestMergeCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Gush\Command\PullRequest\PullRequestMergeCommand;
use Gush\Tests\Command\CommandTestCase;
use Gush\Tests\Fixtures\Adapter\TestAdapter;
use Prophecy\Argument;
use Symfony\Component\Console\Helper\HelperSet;

Expand Down Expand Up @@ -145,6 +146,36 @@ public function testMergePullRequest()
$command,
null,
null,
function (HelperSet $helperSet) {
$helperSet->set($this->getLocalGitHelperForApi(sprintf($this->mergeMessage, 'merge', 10))->reveal());
}
);

$tester->execute(
['pr_number' => 10],
['interactive' => false]
);

$display = $tester->getDisplay();

$this->assertCommandOutputMatches([self::COMMAND_DISPLAY, self::COMMAND_DISPLAY_TARGET], $display);
}

public function testMergePullRequestWithApiDisabled()
{
$command = new PullRequestMergeCommand();
$tester = $this->getCommandTester(
$command,
null,
[
'disable_merge_api' => true,
'repo_adapter' => 'github_enterprise',
'issue_tracker' => 'github_enterprise',
'repo_org' => 'gushphp',
'repo_name' => 'gush',
'issue_project_org' => 'gushphp',
'issue_project_name' => 'gush',
],
function (HelperSet $helperSet) {
$helperSet->set($this->getLocalGitHelper(sprintf($this->mergeMessage, 'merge', 10))->reveal());
}
Expand All @@ -170,11 +201,8 @@ public function testMergePullRequestWithNoComments()
function (HelperSet $helperSet) {
$helperSet->set($this->getGitConfigHelper(false)->reveal());
$helperSet->set(
$this->getLocalGitHelper(
$this->getLocalGitHelperForApi(
sprintf($this->mergeMessage, 'merge', 10),
false,
false,
null,
false
)->reveal()
);
Expand All @@ -198,7 +226,7 @@ public function testMergePullRequestWithCustomType()
null,
null,
function (HelperSet $helperSet) {
$helperSet->set($this->getLocalGitHelper(sprintf($this->mergeMessage, 'feat', 10))->reveal());
$helperSet->set($this->getLocalGitHelperForApi(sprintf($this->mergeMessage, 'feat', 10))->reveal());
}
);

Expand All @@ -219,7 +247,7 @@ public function testMergePullRequestTypeIsAsked()
null,
null,
function (HelperSet $helperSet) {
$helperSet->set($this->getLocalGitHelper(sprintf($this->mergeMessage, 'feature', 10))->reveal());
$helperSet->set($this->getLocalGitHelperForApi(sprintf($this->mergeMessage, 'feature', 10))->reveal());
}
);

Expand Down Expand Up @@ -329,7 +357,7 @@ function (HelperSet $helperSet) {
$helperSet->set($this->getGitConfigHelper(false)->reveal());
$helperSet->set(
$this->getLocalGitHelper(
sprintf($this->mergeMessage, 'merge', 10, 'develop'),
sprintf($this->mergeMessage, 'merge', 10),
false,
false,
null,
Expand All @@ -346,7 +374,7 @@ function (HelperSet $helperSet) {
);

$display = $tester->getDisplay();
$this->assertCommandOutputMatches(sprintf(self::COMMAND_DISPLAY, 'develop'), $display);
$this->assertCommandOutputMatches(sprintf(self::COMMAND_DISPLAY), $display);
}

public function testMergePullRequestInteractiveTypeAsk()
Expand All @@ -357,7 +385,7 @@ public function testMergePullRequestInteractiveTypeAsk()
null,
array_merge(CommandTestCase::$localConfig, ['pr_type' => ['security', 'feature', 'bug']]),
function (HelperSet $helperSet) {
$helperSet->set($this->getLocalGitHelper(sprintf($this->mergeMessage, 'feature', 10))->reveal());
$helperSet->set($this->getLocalGitHelperForApi(sprintf($this->mergeMessage, 'feature', 10))->reveal());
}
);

Expand All @@ -377,7 +405,7 @@ public function testMergeTypeByArgumentIsValidatedWhenTypeRestrictionIsConfigure
array_merge(CommandTestCase::$localConfig, ['pr_type' => ['security', 'feature', 'bug']]),
function (HelperSet $helperSet) {
$helperSet->set($this->getGitConfigHelper(false)->reveal());
$helperSet->set($this->getLocalGitHelper(null, false, false, null, false)->reveal());
$helperSet->set($this->getLocalGitHelperForApi(null, false)->reveal());
}
);

Expand Down Expand Up @@ -492,4 +520,22 @@ function ($closure) use ($message, $switch) {

return $helper;
}

private function getLocalGitHelperForApi($message = null, $withComments = true)
{
$helper = parent::getGitHelper();

if ($withComments) {
$helper->remoteUpdate('gushphp')->shouldBeCalled();
$helper->addNotes(Argument::any(), TestAdapter::MERGE_HASH, 'github-comments')->shouldBeCalled();
$helper->pushToRemote('gushphp', 'refs/notes/github-comments')->shouldBeCalled();
}

if (null !== $message) {
$helper->createRemoteMergeOperation()->shouldNotBeCalled();
$helper->getLogBetweenCommits('base_ref', 'head_ref')->willReturn($this->commits);
}

return $helper;
}
}
6 changes: 4 additions & 2 deletions tests/Fixtures/Adapter/TestAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class TestAdapter extends BaseAdapter implements IssueTracker
private $pullRequest = [];
private $issue = [];

const MERGE_HASH = '32fe234332fe234332fe234332fe234332fe2343';

public function __construct($name)
{
$this->name = $name;
Expand Down Expand Up @@ -403,7 +405,7 @@ public function getPullRequestCommits($id)
{
return [
[
'sha' => '32fe234332fe234332fe234332fe234332fe2343',
'sha' => self::MERGE_HASH,
'message' => 'added merge pull request feature',
'user' => 'cordoval',
],
Expand All @@ -420,7 +422,7 @@ public function getPullRequestCommits($id)
*/
public function mergePullRequest($id, $message)
{
return '32fe234332fe234332fe234332fe234332fe2343';
return self::MERGE_HASH;
}

/**
Expand Down