-
Notifications
You must be signed in to change notification settings - Fork 56
Support Github auto-format for retriggering with other PR #2923
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
base: main
Are you sure you want to change the base?
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.
Code Review
This pull request adds support for parsing GitHub's auto-formatted pull request URLs when retriggering tests, which is a great enhancement. The implementation is straightforward and includes relevant test cases. I have one suggestion to improve the code's efficiency and readability by avoiding a redundant regular expression match.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
682d0be to
e207ae4
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 52s |
| pr_argument_pattern = re.compile(r"^[^/\s]+/[^#\s]+#\d+$") | ||
| github_url_pattern = re.compile(r"^https://github\.com/([^/\s]+)/([^/\s]+)/pull/(\d+)$") |
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.
Probably wouldn't be a bad idea to have one regex for both, since you can capture what you need from both;
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.
Additionally it wouldn't be that bad of an idea to compile the regex(es) only once, e.g., in constants?
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.
https://regex101.com/r/sX7EJU/1
Note
- I dropped the
^and$for boundaries, since it didn't work well with whole comments I copy-pasted - Also used named capture groups, since the alternatives must also be enclosed in parentheses and therefore make a capture group, haven't checked the Python docs, but I ticked the Python syntax on that web, so it should be allowed.
| self.pr_argument = arg | ||
| logger.debug(f"Parsed pr_argument: {self.pr_argument}") | ||
| break | ||
| if match := github_url_pattern.match(arg): |
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.
We support this only for GitHub?
Or GitHub is the only “UX-friendly” enough to do this crap?
am afraid I know the answers to those two questions sigh
Fixes #2866
RELEASE NOTES BEGIN
For retriggering tests with builds from another pull request, Packit now supports Github's automatically converted URL format (
https://github.com/namespace/repo/pull/<pr_id>, e.g./packit test https://github.com/namespace/repo/pull/<pr_id>)RELEASE NOTES END