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 --remove-source-branch/--no-remove-source-branch options to pull-request:merge command #619

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
9 changes: 9 additions & 0 deletions src/Adapter/Adapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -369,4 +369,13 @@ public function getReleaseAssets($id);
* @throws AdapterException when deleting of release failed
*/
public function removeRelease($id);

/**
* Deletes the remote source branch used in a pull request.
*
* @param int $id
*
* @return mixed
*/
public function removePullRequestSourceBranch($id);
}
25 changes: 21 additions & 4 deletions src/Command/PullRequest/PullRequestCloseCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,13 @@ protected function configure()
->setDescription('Closes a pull request')
->addArgument('pr_number', InputArgument::REQUIRED, 'Pull Request number to be closed')
->addOption('message', 'm', InputOption::VALUE_REQUIRED, 'Closing comment')
->addOption('remove-source-branch', null, InputOption::VALUE_REQUIRED, 'Remove remote source branch after closing own pull request', 'no')
->setHelp(
<<<EOF
The <info>%command.name%</info> command closes a Pull Request for either the current or the given organization
and repository:

<info>$ gush %command.name% 12 -m"let's try to keep it low profile guys."</info>
<info>$ gush %command.name% 12 -m"let's try to keep it low profile guys." --remove-source-branch=yes</info>

EOF
)
Expand All @@ -47,11 +48,16 @@ protected function configure()
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
$prNumber = $input->getArgument('pr_number');
$closingComment = $input->getOption('message');

$adapter = $this->getAdapter();
$prNumber = $input->getArgument('pr_number');
$pr = $adapter->getPullRequest($prNumber);
$authenticatedUser = $this->getParameter($input, 'authentication')['username'];
$removeSourceBranch = $input->getOption('remove-source-branch');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe do a strtolower here, just for safety sake for the checks below. Or maybe the option should be changed to not take a value, so it is set to true when passed through

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, value should not be required. That's the common pattern for bool options.

Copy link
Member Author

@phansys phansys Aug 29, 2016

Choose a reason for hiding this comment

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

Should we add a --no-remove-source-branch option then in order to let the user express its choice to not delete the source branch without being prompted?

if ('yes' === $removeSourceBranch && $pr['user'] !== $authenticatedUser) {
throw new UserException(sprintf('`--remove-source-branch` option cannot be used with pull requests that aren\'t owned by the authenticated user (%s)', $authenticatedUser));
}

$closingComment = $input->getOption('message');
$adapter->closePullRequest($prNumber);

if ($input->getOption('message')) {
Expand All @@ -61,6 +67,17 @@ protected function execute(InputInterface $input, OutputInterface $output)
$url = $adapter->getPullRequest($prNumber)['url'];
$this->getHelper('gush_style')->success("Closed {$url}");

// Post close options
if ($pr['user'] === $authenticatedUser) {
if ('yes' !== $removeSourceBranch) {
$removeSourceBranch = $this->getHelper('gush_style')->choice('Delete source branch?', ['yes', 'no'], 'no');
}
if ('yes' === $removeSourceBranch) {
$adapter->removePullRequestSourceBranch($pr['number']);
$this->getHelper('gush_style')->note(sprintf('Remote source branch %s:%s has been removed.', $pr['head']['user'], $pr['head']['ref']));
}
}

return self::COMMAND_SUCCESS;
}
}
40 changes: 34 additions & 6 deletions src/Command/PullRequest/PullRequestMergeCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ protected function configure()
->addOption('force-squash', null, InputOption::VALUE_NONE, 'Force squashing the PR, even if there are multiple authors (this will implicitly use --squash)')
->addOption('switch', null, InputOption::VALUE_REQUIRED, 'Switch the base of the pull request before merging')
->addOption('pat', null, InputOption::VALUE_REQUIRED, 'Give the PR\'s author a pat on the back after the merge')
->addOption('remove-source-branch', null, InputOption::VALUE_NONE, 'Remove remote source branch after merging own pull request')
->addOption('no-remove-source-branch', null, InputOption::VALUE_NONE, 'Don\'t remove remote source branch after merging own pull request')
->setHelp(
<<<'EOF'
The <info>%command.name%</info> command merges the given pull request:
Expand Down Expand Up @@ -99,6 +101,14 @@ protected function configure()
which has precedence to the predefined configuration.

<comment>The whole pat configuration will be ignored and no pat will be placed if the pull request is authored by yourself!</comment>

If you are the author of the pull request, you'll be prompted if the remote source branch will be removed after a successful merge.
Also, you can pass your choice for this action with the <comment>--remove-source-branch</comment> and <comment>--no-remove-source-branch</comment>
options:

<info>$ gush %command.name% --remove-source-branch</info>

<info>$ gush %command.name% --no-remove-source-branch</info>
EOF
)
;
Expand Down Expand Up @@ -141,6 +151,12 @@ protected function execute(InputInterface $input, OutputInterface $output)
$targetLabel = sprintf('Target: %s/%s', $targetRemote, $targetBranch);
}

$authenticatedUser = $this->getParameter($input, 'authentication')['username'];
$removeSourceBranch = $input->getOption('remove-source-branch');
if ($removeSourceBranch && $pr['user'] !== $authenticatedUser) {
Copy link
Contributor

@sstok sstok Aug 10, 2016

Choose a reason for hiding this comment

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

This can be a problem for GitLab CE where there are no forks for pull-requests.
And you are always allowed, is it possible to add an API method to check for access?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sstok, I don't know if I got your point. I think this restriction isn't driven by permissions, but for respect to PR author. By instance, in my private organization I have access to delete branches from another author than me, but I think this isn't a good practice or something we should allow. IMO, the responsibility over each branch belongs only to their author, regardless your access level as merger.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, no problem 👍

throw new UserException(sprintf('`--remove-source-branch` option cannot be used with pull requests that aren\'t owned by the authenticated user (%s)', $authenticatedUser));
}

$styleHelper->title(sprintf('Merging pull-request #%d - %s', $prNumber, $pr['title']));
$styleHelper->text([sprintf('Source: %s/%s', $sourceRemote, $sourceBranch), $targetLabel]);

Expand Down Expand Up @@ -197,15 +213,18 @@ protected function execute(InputInterface $input, OutputInterface $output)
$this->addClosedPullRequestNote($pr, $mergeCommit, $squash, $input->getOption('switch'));
}

if ($pr['user'] !== $this->getParameter($input, 'authentication')['username']) {
$patComment = $this->givePatToPullRequestAuthor($pr, $input->getOption('pat'));
if ($patComment) {
$styleHelper->note(sprintf('Pat given to @%s at %s.', $pr['user'], $patComment));
$styleHelper->success([$mergeNote, $pr['url']]);

// Post merge options
if ($pr['user'] === $authenticatedUser) {
if ($removeSourceBranch || (!$input->getOption('no-remove-source-branch') && 'yes' === $this->getHelper('gush_style')->choice('Delete source branch?', ['yes', 'no'], 'no'))) {
$adapter->removePullRequestSourceBranch($pr['number']);
$styleHelper->note(sprintf('Remote source branch %s:%s has been removed.', $sourceRemote, $sourceBranch));
}
} elseif ($patComment = $this->givePatToPullRequestAuthor($pr, $input->getOption('pat'))) {
$styleHelper->note(sprintf('Pat given to @%s at %s.', $pr['user'], $patComment));
}

$styleHelper->success([$mergeNote, $pr['url']]);

return self::COMMAND_SUCCESS;
} catch (CannotSquashMultipleAuthors $e) {
$styleHelper->error([
Expand Down Expand Up @@ -415,4 +434,13 @@ private function givePatToPullRequestAuthor(array $pr, $pat)
return $this->getAdapter()->createComment($pr['number'], $patMessage);
}
}

protected function initialize(InputInterface $input, OutputInterface $output)
{
parent::initialize($input, $output);

if ($input->getOption('remove-source-branch') && $input->getOption('no-remove-source-branch')) {
throw new UserException('Options `--remove-source-branch` and `--no-remove-source-branch` cannot be used toghether');
}
}
}
7 changes: 7 additions & 0 deletions src/ThirdParty/Bitbucket/BitbucketRepoAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Gush\Adapter\BaseAdapter;
use Gush\Exception\AdapterException;
use Gush\Exception\UnsupportedOperationException;
use Herrera\Version\Parser;
use Herrera\Version\Validator as VersionValidator;

Expand Down Expand Up @@ -519,4 +520,10 @@ protected function adaptPullRequestStructure(array $pr)
],
];
}

public function removePullRequestSourceBranch($id)
{
/** @link https://developer.atlassian.com/static/rest/bitbucket-server/4.7.1/bitbucket-branch-rest.html#idp45632 REST Resources Provided By: Bitbucket Server - Branch */
throw new UnsupportedOperationException('Bitbucket client "gentle/bitbucket-api" doesn\'t support references.');
}
}
8 changes: 8 additions & 0 deletions src/ThirdParty/Github/GitHubAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,14 @@ public function createReleaseAssets($id, $name, $contentType, $content)
return $asset['id'];
}

public function removePullRequestSourceBranch($id)
{
$api = $this->client->api('git_data')->references();
$pr = $this->getPullRequest($id);

return $api->remove($pr['user'], $pr['head']['repo'], 'heads/'.$pr['head']['ref']);
}

protected function adaptIssueStructure(array $issue)
{
return [
Expand Down
8 changes: 8 additions & 0 deletions src/ThirdParty/Gitlab/Adapter/GitLabRepoAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -278,4 +278,12 @@ public function createReleaseAssets($id, $name, $contentType, $content)
{
throw new UnsupportedOperationException('Releases are not supported by Gitlab.');
}

public function removePullRequestSourceBranch($id)
{
$api = $this->client->api('repo');
$pr = $this->getPullRequest($id);

return $api->deleteBranch($this->getCurrentProject()->id, $pr['head']['ref']);
}
}
5 changes: 5 additions & 0 deletions tests/Fixtures/Adapter/TestAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,11 @@ public function createReleaseAssets($id, $name, $contentType, $content)
return self::RELEASE_ASSET_NUMBER;
}

public function removePullRequestSourceBranch($id)
{
return [];
}

/**
* {@inheritdoc}
*/
Expand Down