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

Validate Git branch naming convention #328 #329

Merged
merged 11 commits into from
Mar 16, 2017
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ parameters:
doctrine_orm: ~
gherkin: ~
git_blacklist: ~
git_branch_name: ~
Copy link
Contributor

Choose a reason for hiding this comment

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

This list should be ordered alfabetical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

git_commit_message: ~
git_conflict: ~
grunt: ~
Expand Down
2 changes: 2 additions & 0 deletions doc/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ parameters:
doctrine_orm: ~
gherkin: ~
git_blacklist: ~
git_branch_name: ~
Copy link
Contributor

Choose a reason for hiding this comment

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

This list should be ordered alfabetical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, also task link below in the same file.

git_commit_message: ~
git_conflict: ~
grunt: ~
Expand Down Expand Up @@ -60,6 +61,7 @@ Every task has it's own default configuration. It is possible to overwrite the p
- [Doctrine ORM](tasks/doctrine_orm.md)
- [Gherkin](tasks/gherkin.md)
- [Git blacklist](tasks/git_blacklist.md)
- [Git branch name](tasks/git_branch_name.md)
- [Git commit message](tasks/git_commit_message.md)
- [Git conflict](tasks/git_conflict.md)
- [Grunt](tasks/grunt.md)
Expand Down
40 changes: 40 additions & 0 deletions doc/tasks/git_branch_name.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Git commit message

The Git branch name task ensures that the current branch name matches the specified patterns.
For example: if you are working with JIRA, it is possible to add a pattern for the JIRA issue number.

```yaml
# grumphp.yml
parameters:
tasks:
git_branch_name:
matchers:
Branch name nust contain JIRA issue number: /JIRA-\d+/
additional_modifiers: ''
```

**matchers**

*Default: []*

Use this parameter to specify one or multiple patterns. The value can be in regex or glob style.
Here are some example matchers:

- /JIRA-([0-9]*)/
- pre-fix*
- *suffix
- ...

**additional_modifiers**

*Default: ''*

Add one or multiple additional modifiers like:

```yaml
additional_modifiers: 'u'

# or

additional_modifiers: 'xu'
```
8 changes: 8 additions & 0 deletions resources/config/tasks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ services:
tags:
- {name: grumphp.task, config: git_commit_message}

task.git.branchname:
class: GrumPHP\Task\Git\BranchName
arguments:
- '@config'
- '@git.repository'
tags:
- {name: grumphp.task, config: git_branch_name}

task.git.conflict:
class: GrumPHP\Task\Git\Conflict
arguments:
Expand Down
83 changes: 83 additions & 0 deletions spec/Task/Git/BranchNameSpec.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
<?php

namespace spec\GrumPHP\Task\Git;

use Gitonomy\Git\Repository;
use GrumPHP\Configuration\GrumPHP;
use GrumPHP\Runner\TaskResultInterface;
use GrumPHP\Task\Context\RunContext;
use GrumPHP\Task\Git\BranchName;
use GrumPHP\Task\TaskInterface;
use PhpSpec\ObjectBehavior;
use Symfony\Component\OptionsResolver\OptionsResolver;

class BranchNameSpec extends ObjectBehavior
{
function let(GrumPHP $grumPHP, Repository $repository)
{
$this->beConstructedWith($grumPHP, $repository);
$grumPHP->getTaskConfiguration('git_branch_name')->willReturn([
'matchers' => ['test', '*es*', 'te[s][t]', '/^te(.*)/', '/(.*)st$/', '/t(e|a)st/', 'TEST'],
'additional_modifiers' => 'i',
]);
$repository->run('symbolic-ref', ['HEAD', '--short'])->willReturn('test');
}

function it_should_have_a_name()
{
$this->getName()->shouldBe('git_branch_name');
}

function it_should_have_configurable_options()
{
$options = $this->getConfigurableOptions();
$options->shouldBeAnInstanceOf(OptionsResolver::class);
$options->getDefinedOptions()->shouldContain('matchers');
$options->getDefinedOptions()->shouldContain('additional_modifiers');
}

function it_is_initializable()
{
$this->shouldHaveType(BranchName::class);
}

function it_is_a_grumphp_task()
{
$this->shouldImplement(TaskInterface::class);
}

function it_should_run_in_run_context(RunContext $context)
{
$this->canRunInContext($context)->shouldReturn(true);
}

function it_runs_the_suite(RunContext $context)
{
$result = $this->run($context);
$result->shouldBeAnInstanceOf(TaskResultInterface::class);
$result->isPassed()->shouldBe(true);
}

function it_throws_exception_if_the_process_fails(RunContext $context, Repository $repository)
{
$repository->run('symbolic-ref', ['HEAD', '--short'])->willReturn('not-good');

$result = $this->run($context);
$result->shouldBeAnInstanceOf(TaskResultInterface::class);
$result->isPassed()->shouldBe(false);
}

function it_runs_with_additional_modifiers(RunContext $context, GrumPHP $grumPHP, Repository $repository)
{
$grumPHP->getTaskConfiguration('git_branch_name')->willReturn([
'matchers' => ['/^ümlaut/'],
'additional_modifiers' => 'u',
]);

$repository->run('symbolic-ref', ['HEAD', '--short'])->willReturn('ümlaut-branch-name');

$result = $this->run($context);
$result->shouldBeAnInstanceOf(TaskResultInterface::class);
$result->isPassed()->shouldBe(true);
}
}
130 changes: 130 additions & 0 deletions src/Task/Git/BranchName.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
<?php

namespace GrumPHP\Task\Git;

use GrumPHP\Runner\TaskResult;
use GrumPHP\Task\Context\ContextInterface;
use GrumPHP\Task\Context\RunContext;
use GrumPHP\Util\Regex;
use GrumPHP\Exception\RuntimeException;
use GrumPHP\Configuration\GrumPHP;
use GrumPHP\Task\TaskInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Gitonomy\Git\Repository;

/**
* Git BranchName Task
*/
class BranchName implements TaskInterface
{

/**
* @var GrumPHP
*/
protected $grumPHP;

/**
* @var Repository
*/
protected $repository;

/**
* @param GrumPHP $grumPHP
*/
public function __construct(GrumPHP $grumPHP, Repository $repository)
{
$this->grumPHP = $grumPHP;
$this->repository = $repository;
}

/**
* @return string
*/
public function getName()
{
return 'git_branch_name';
}

/**
* @return array
*/
public function getConfiguration()
{
$configured = $this->grumPHP->getTaskConfiguration($this->getName());

return $this->getConfigurableOptions()->resolve($configured);
}

/**
* @return OptionsResolver
*/
public function getConfigurableOptions()
{
$resolver = new OptionsResolver();
$resolver->setDefaults([
'matchers' => [],
'additional_modifiers' => ''
]);

$resolver->addAllowedTypes('matchers', ['array']);
$resolver->addAllowedTypes('additional_modifiers', ['string']);

return $resolver;
}

/**
* @param ContextInterface $context
*
* @return bool
*/
public function canRunInContext(ContextInterface $context)
{
return $context instanceof RunContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also run in pre-commit context? Now it will not be triggered during a git commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done, I've added the pre commit context too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, can you add a test for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done!

}

/**
* @param array $config
* @param string $name
* @param string $rule
* @param string $ruleName
*
* @throws RuntimeException
*/
private function runMatcher(array $config, $name, $rule, $ruleName)
{
$regex = new Regex($rule);

$additionalModifiersArray = array_filter(str_split((string) $config['additional_modifiers']));
Copy link
Contributor

Choose a reason for hiding this comment

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

The additional modifiers will always be a string since it is enforce by the OptionsResolver.

array_map([$regex, 'addPatternModifier'], $additionalModifiersArray);

if (!preg_match((string) $regex, $name)) {
throw new RuntimeException("Rule not matched: \"$ruleName\" $rule");
}
}

/**
* @param ContextInterface|RunContext $context
*
* @return TaskResult
*/
public function run(ContextInterface $context)
{
$name = trim($this->repository->run('symbolic-ref', ['HEAD', '--short']));
Copy link
Contributor

@veewee veewee Mar 14, 2017

Choose a reason for hiding this comment

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

When the current revision is a non symbolic reference, the command will throw an exception like:

Error code: 128
Message: fatal: ref HEAD is not a symbolic ref

This can happen in detached head or if you checked out a specific commit.
This will throw a Gitonomy\Git\Exception\ProcessException.

How would you like to handle this issue on the task? Should it be configurable to allow non symbolic references or not? We might want to fetch the error message and display a user-friendly error message instead?

More info: https://git-scm.com/docs/git-symbolic-ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should not check for branch naming convention when you are on a detached head or you checkout a specific commit? We could just catch the exception and skip the step by returning a TaskResult::createSkipped(), does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's best to make this configurable through an extra configuration flag?
Some people might want to allow it, others may have something against it.

Rules:

  • If you are allowed to commit on non symbolic references: task passes
  • If you are not allowed to commit on non symbolic references: task fails with a decent error message.

Copy link
Contributor Author

@ademarco ademarco Mar 14, 2017

Choose a reason for hiding this comment

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

Ok shall we call the boolean option allow_symbolic_references? Should it default to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something like 4f95d27 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. I've added some remarks.
Can you add documentation and tests for this new flag?

$config = $this->getConfiguration();
$exceptions = [];

foreach ($config['matchers'] as $ruleName => $rule) {
try {
$this->runMatcher($config, $name, $rule, $ruleName);
} catch (RuntimeException $e) {
$exceptions[] = $e->getMessage();
}
}

if (count($exceptions)) {
return TaskResult::createFailed($this, $context, implode(PHP_EOL, $exceptions));
}

return TaskResult::createPassed($this, $context);
}
}