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

Conversation

EmilMassey
Copy link
Contributor

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.

Copy link
Owner

@staabm staabm left a 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

extension.neon Outdated Show resolved Hide resolved
src/utils/TicketStatusFetcher.php Show resolved Hide resolved
src/utils/github/GitHubTicketStatusFetcher.php Outdated Show resolved Hide resolved
extension.neon Outdated Show resolved Hide resolved
@staabm
Copy link
Owner

staabm commented Jan 4, 2024

I think we need at least one real end-2-end test for github and jira

@EmilMassey
Copy link
Contributor Author

EmilMassey commented Jan 5, 2024

I think we need at least one real end-2-end test for github and jira

Unfortunately I have no idea how to approach it. Do I understand correctly that we need to rely on real issues on GitHub / Jira?

  1. Currently I see that current E2E tests workflow asserts only if no issues are reported. I think we should also be able to test if expected issues are present. How can we do it? I think about comparing it to a text file with expected errors (see 09f21fe), but I'm not sure if it's OK.
  2. How to test if it works with open issues? Should we have an always-open dummy issue on GitHub?

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.

@staabm
Copy link
Owner

staabm commented Jan 5, 2024

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)

@EmilMassey EmilMassey marked this pull request as ready for review January 5, 2024 22:48
Copy link
Owner

@staabm staabm left a 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.

README.md Outdated Show resolved Hide resolved
src/utils/ticket/GitHubTicketStatusFetcher.php Outdated Show resolved Hide resolved
src/utils/ticket/GitHubTicketStatusFetcher.php Outdated Show resolved Hide resolved
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

@staabm staabm merged commit 93770c1 into staabm:main Jan 6, 2024
11 of 13 checks passed
@staabm
Copy link
Owner

staabm commented Jan 6, 2024

I couldn't wait. love it

@EmilMassey EmilMassey deleted the github-issues branch January 6, 2024 11:47
# 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants