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 GitHub support for by ticket rule #62

Merged
merged 13 commits into from
Jan 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .github/workflows/e2e-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ jobs:
script: |
composer install
vendor/bin/phpstan analyse
- os: ubuntu-latest
php-version: '8.2'
test-dir: tests-e2e/github/
script: |
composer install
diff <(vendor/bin/phpstan analyse --error-format=raw --no-progress | sed "s|$(pwd)/||") expected-errors.txt
Copy link
Owner

Choose a reason for hiding this comment

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

looks great. another one for jira please - after we merged the PR and verified this works in the pipeline as expected

Copy link
Owner

@staabm staabm Jan 6, 2024

Choose a reason for hiding this comment

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

I have fixed the github action, so the e2e tests should also work on forks now.
please rebase and we will see.

Copy link
Owner

Choose a reason for hiding this comment

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

@EmilMassey would be great you could do the jira e2e test


steps:
- name: Checkout
Expand Down
41 changes: 39 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ see examples of different comment variants which are supported:
// TODO: phpunit/phpunit:<5 This has to be fixed before updating to phpunit 5.x
// TODO@markus: phpunit/phpunit:5.3 This has to be fixed when updating phpunit to 5.3.x or higher

// TODO: APP-123 fix it
// TODO: APP-123 fix it when this Jira ticket is closed
// TODO: #123 fix it when this GitHub issue is closed
// TODO: some-organization/some-repo#123 change me if this GitHub pull request is closed
```

## Configuration
Expand Down Expand Up @@ -180,7 +182,7 @@ Reference these virtual packages like any other package in your todo-comments:
Optionally you can configure this extension to analyze your comments with issue tracker ticket keys.
The extension fetches issue tracker API for issue status. If the remote issue is resolved, the comment will be reported.

Currently only Jira is supported.
Currently, Jira and GitHub are supported.

This feature is disabled by default. To enable it, you must set `ticket.enabled` parameter to `true`.
You also need to set these parameters:
Expand All @@ -191,8 +193,12 @@ parameters:
ticket:
enabled: true

# one of: jira, github (case-sensitive)
tracker: jira

# a case-sensitive list of status names.
# only tickets having any of these statuses are considered resolved.
# supported trackers: jira. Other trackers ignore this parameter.
Copy link
Owner

Choose a reason for hiding this comment

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

@EmilMassey why does the github tracker not need the resolved statuses?

don't we need the github ticket status in this list here to overcome the IF in

if (!in_array($ticketStatus, $this->configuration->getResolvedStatuses(), true)) {
continue;
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staabm
In GitHub there are only two issue states: open and closed. This is different than Jira, where you can define your statuses according to your needs. So setting this parameter doesn't make sense for GitHub, because you always want to check if it is closed.

This condition you mentioned is satisfied because in TicketRuleConfigurationFactory we hardcode ['closed'] list as resolved statuses.

resolvedStatuses:
- Done
- Resolved
Expand All @@ -201,6 +207,7 @@ parameters:
# if your ticket key is FOO-12345, then this value should be ["FOO"].
# multiple key prefixes are allowed, e.g. ["FOO", "APP"].
# only comments with keys containing this prefix will be analyzed.
# supported trackers: jira. Other trackers ignore this parameter.
keyPrefixes:
- FOO

Expand All @@ -217,6 +224,22 @@ parameters:
# if credentials parameter is not empty, it will be used instead of this file.
# this file must not be commited into the repository!
credentialsFilePath: .secrets/jira-credentials.txt

github:
# The account owner of referenced repositories.
defaultOwner: your-name

# The name of the repository without the .git extension.
defaultRepo: your-repository

# GitHub Access Token
# if this value is empty, credentials file will be used instead.
credentials: null

# path to a file containing GitHub Access Token.
# if credentials parameter is not empty, it will be used instead of this file.
# this file must not be committed into the repository!
credentialsFilePath: null
```

#### Jira Credentials
Expand Down Expand Up @@ -263,6 +286,20 @@ Depending on chosen authentication method its value should be:
* Access Token for token based authentication, e.g. `JATATT3xFfGF0Gv_pLFSsunmigz8wq5Y0wkogoTMgxDFHyR...`
* `<username>:<passwordOrApiKey>` for basic authentication, e.g. `john.doe@example.com:p@ssword`

#### GitHub
Both issues and pull requests are supported. The comment will be reported if the referenced issue/PR is closed.
There are multiple ways to reference GitHub issue/PR:

##### Only number
```php
// TODO: #123 - fix me
```
If the `defaultOwner` is set to `acme` and `defaultRepo` is set to `hello-world`, the referenced issue is resolved to `acme/hello-world#123`.

##### Owner + repository name + number
```php
// TODO: acme/hello-world#123 - fix me
```

## Installation

Expand Down
48 changes: 44 additions & 4 deletions extension.neon
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,20 @@ parametersSchema:
virtualPackages: arrayOf(string(), string())
ticket: structure([
enabled: bool()
tracker: anyOf('jira', 'github')
keyPrefixes: listOf(string())
resolvedStatuses: listOf(string())
jira: structure([
server: string()
credentials: schema(string(), nullable())
credentialsFilePath: schema(string(), nullable())
])
github: structure([
defaultOwner: string()
defaultRepo: string()
credentials: schema(string(), nullable())
credentialsFilePath: schema(string(), nullable())
])
])
])

Expand All @@ -39,13 +46,18 @@ parameters:
# whether to analyze comments by issue tracker ticket key
enabled: false

# one of: jira, github (case-sensitive)
tracker: jira

# a case-sensitive list of status names.
# only tickets having any of these statuses are considered resolved.
# supported trackers: jira. Other trackers ignore this parameter.
resolvedStatuses: []

# if your ticket key is FOO-12345, then this value should be ["FOO"].
# multiple key prefixes are allowed, e.g. ["FOO", "APP"].
# only comments with keys containing this prefix will be analyzed.
# supported trackers: jira. Other trackers ignore this parameter.
keyPrefixes: []

jira:
Expand All @@ -62,6 +74,22 @@ parameters:
# this file must not be commited into the repository!
credentialsFilePath: null

github:
# The account owner of referenced repositories.
defaultOwner: ~

# The name of the repository without the .git extension.
defaultRepo: ~

# GitHub Access Token
# if this value is empty, credentials file will be used instead.
credentials: null

# path to a file containing GitHub Access Token.
# if credentials parameter is not empty, it will be used instead of this file.
# this file must not be committed into the repository!
credentialsFilePath: null

conditionalTags:
staabm\PHPStanTodoBy\TodoByTicketRule:
phpstan.rules.rule: %todo_by.ticket.enabled%
Expand All @@ -74,9 +102,6 @@ services:
- %todo_by.referenceTime%
-
class: staabm\PHPStanTodoBy\TodoByTicketRule
arguments:
- %todo_by.ticket.resolvedStatuses%
- %todo_by.ticket.keyPrefixes%

-
class: staabm\PHPStanTodoBy\TodoByVersionRule
Expand Down Expand Up @@ -105,7 +130,22 @@ services:
- %todo_by.nonIgnorable%

-
class: staabm\PHPStanTodoBy\utils\jira\JiraTicketStatusFetcher
class: staabm\PHPStanTodoBy\utils\ticket\TicketRuleConfigurationFactory

-
class: staabm\PHPStanTodoBy\utils\ticket\TicketRuleConfiguration
factory: @staabm\PHPStanTodoBy\utils\ticket\TicketRuleConfigurationFactory::create

-
class: staabm\PHPStanTodoBy\utils\ticket\GitHubTicketStatusFetcher
arguments:
- %todo_by.ticket.github.defaultOwner%
- %todo_by.ticket.github.defaultRepo%
- %todo_by.ticket.github.credentials%
- %todo_by.ticket.github.credentialsFilePath%

-
class: staabm\PHPStanTodoBy\utils\ticket\JiraTicketStatusFetcher
arguments:
- %todo_by.ticket.jira.server%
- %todo_by.ticket.jira.credentials%
Expand Down
2 changes: 1 addition & 1 deletion phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ includes:
- extension.neon

parameters:
level: 9
level: 8

paths:
- src
Expand Down
57 changes: 26 additions & 31 deletions src/TodoByTicketRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use PHPStan\Rules\Rule;
use staabm\PHPStanTodoBy\utils\CommentMatcher;
use staabm\PHPStanTodoBy\utils\ExpiredCommentErrorBuilder;
use staabm\PHPStanTodoBy\utils\TicketStatusFetcher;
use staabm\PHPStanTodoBy\utils\ticket\TicketRuleConfiguration;

use function in_array;
use function strlen;
Expand All @@ -18,34 +18,12 @@
*/
final class TodoByTicketRule implements Rule
{
private const PATTERN = <<<'REGEXP'
{
@?TODO # possible @ prefix
@?[a-zA-Z0-9_-]* # optional username
\s*[:-]?\s* # optional colon or hyphen
\s+ # keyword/ticket separator
(?P<ticketKey>[A-Z0-9]+-\d+) # ticket key consisting of ABC-123 or F01-12345 format
\s*[:-]?\s* # optional colon or hyphen
(?P<comment>(?:(?!\*+/).)*) # rest of line as comment text, excluding block end
}ix
REGEXP;

/** @var list<non-empty-string> */
private array $resolvedStatuses;
/** @var list<non-empty-string> */
private array $keyPrefixes;
private TicketStatusFetcher $fetcher;
private TicketRuleConfiguration $configuration;
private ExpiredCommentErrorBuilder $errorBuilder;

/**
* @param list<non-empty-string> $resolvedStatuses
* @param list<non-empty-string> $keyPrefixes
*/
public function __construct(array $resolvedStatuses, array $keyPrefixes, TicketStatusFetcher $fetcher, ExpiredCommentErrorBuilder $errorBuilder)
public function __construct(TicketRuleConfiguration $configuration, ExpiredCommentErrorBuilder $errorBuilder)
{
$this->resolvedStatuses = $resolvedStatuses;
$this->keyPrefixes = $keyPrefixes;
$this->fetcher = $fetcher;
$this->configuration = $configuration;
$this->errorBuilder = $errorBuilder;
}

Expand All @@ -56,7 +34,7 @@ public function getNodeType(): string

public function processNode(Node $node, Scope $scope): array
{
$it = CommentMatcher::matchComments($node, self::PATTERN);
$it = CommentMatcher::matchComments($node, $this->createPattern());

$errors = [];
foreach ($it as $comment => $matches) {
Expand All @@ -65,11 +43,11 @@ public function processNode(Node $node, Scope $scope): array
$ticketKey = $match['ticketKey'][0];
$todoText = trim($match['comment'][0]);

if (!$this->hasPrefix($ticketKey)) {
if ([] !== $this->configuration->getKeyPrefixes() && !$this->hasPrefix($ticketKey)) {
continue;
}

$ticketStatus = $this->fetcher->fetchTicketStatus($ticketKey);
$ticketStatus = $this->configuration->getFetcher()->fetchTicketStatus($ticketKey);

if (null === $ticketStatus) {
$errors[] = $this->errorBuilder->buildError(
Expand All @@ -82,7 +60,7 @@ public function processNode(Node $node, Scope $scope): array
continue;
}

if (!in_array($ticketStatus, $this->resolvedStatuses, true)) {
if (!in_array($ticketStatus, $this->configuration->getResolvedStatuses(), true)) {
continue;
}

Expand All @@ -104,9 +82,26 @@ public function processNode(Node $node, Scope $scope): array
return $errors;
}

private function createPattern(): string
{
$keyRegex = $this->configuration->getKeyPattern();

return <<<"REGEXP"
{
@?TODO # possible @ prefix
@?[a-zA-Z0-9_-]* # optional username
\s*[:-]?\s* # optional colon or hyphen
\s+ # keyword/ticket separator
(?P<ticketKey>$keyRegex) # ticket key
\s*[:-]?\s* # optional colon or hyphen
(?P<comment>(?:(?!\*+/).)*) # rest of line as comment text, excluding block end
}ix
REGEXP;
}

private function hasPrefix(string $ticketKey): bool
{
foreach ($this->keyPrefixes as $prefix) {
foreach ($this->configuration->getKeyPrefixes() as $prefix) {
if (substr($ticketKey, 0, strlen($prefix)) === $prefix) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<?php

namespace staabm\PHPStanTodoBy\utils\jira;
namespace staabm\PHPStanTodoBy\utils;

use RuntimeException;

final class JiraAuthorization
final class CredentialsHelper
{
public static function getCredentials(?string $credentials, ?string $credentialsFilePath): ?string
{
Expand All @@ -24,13 +24,4 @@ public static function getCredentials(?string $credentials, ?string $credentials

return trim($credentials);
}

public static function createAuthorizationHeader(string $credentials): string
{
if (str_contains($credentials, ':')) {
return 'Basic ' . base64_encode($credentials);
}

return "Bearer $credentials";
}
}
Loading
Loading