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

Conversation

phansys
Copy link
Member

@phansys phansys commented Aug 10, 2016

Q A
Fixed tickets
License MIT

TODO:

  • Add tests.
  • Provide the same option to pull-request:close command
  • Add removePullRequestSourceBranch() to Adapter interface
  • Use Adapter::removePullRequestSourceBranch() at branch:delete command instead GitHelper <- this is not possible since the command doesn't work with pull requests, it is able to delete any remote branch regardless it is part a PR or not.

@@ -141,6 +147,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 👍

@phansys phansys force-pushed the remove_remote_branch_after_merge branch from a46f086 to cba9fda Compare August 11, 2016 03:40
@phansys phansys force-pushed the remove_remote_branch_after_merge branch from 9ef9c42 to 546945f Compare August 21, 2016 21:28
$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?

@phansys phansys changed the title Add remove-source-branch option to pull-request:merge command Add --remove-source-branch/--no-remove-source-branch options to pull-request:merge command Sep 7, 2016
@phansys phansys force-pushed the remove_remote_branch_after_merge branch from 7739ff8 to 983eaeb Compare September 9, 2016 18:12
@phansys phansys force-pushed the remove_remote_branch_after_merge branch from 983eaeb to 341830d Compare March 4, 2017 22:32
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.

3 participants