-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direction
I think we need at least one real end-2-end test for github and jira |
2f152b5
to
a6a3ce6
Compare
Unfortunately I have no idea how to approach it. Do I understand correctly that we need to rely on real issues on GitHub / Jira?
GitHub API allows accessing public repositories without providing access token, but rate-limits apply (currently 60 requests per hour AFAIK). We'll probably need to use a secret in the CI with a token to avoid hitting the limits. Do we need our own Jira board to be able to test it e2e? I know there are some public boards that we could access, but it's very risky to rely on some external board which may disappear some day. |
I would be fine when the E2E test just triggers for a issue, no matter its state. I just want to see our curl client can connect, find the issue and get its state (for both trackers) |
09f21fe
to
65139ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits, but it looks really good already.
test-dir: tests-e2e/github/ | ||
script: | | ||
composer install | ||
diff <(vendor/bin/phpstan analyse --error-format=raw --no-progress | sed "s|$(pwd)/||") expected-errors.txt |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I couldn't wait. love it |
# 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. |
There was a problem hiding this comment.
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
phpstan-todo-by/src/TodoByTicketRule.php
Lines 63 to 65 in 00690bb
if (!in_array($ticketStatus, $this->configuration->getResolvedStatuses(), true)) { | |
continue; | |
} |
?
There was a problem hiding this comment.
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.
This PR (still a draft) adds GitHub support, but also tries to prepare an extension to support multiple issue trackers. I can see that there's already pending support for YouTrack #51, so I decided to publish this draft so we can discuss the direction together.