Support multiple PR arguments in /packit test#3118
Support multiple PR arguments in /packit test#3118centosinfra-prod-github-app[bot] merged 1 commit intopackit:mainfrom
/packit test#3118Conversation
|
✔️ pre-commit SUCCESS in 1m 51s |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables support for multiple PR arguments in testing farm commands by transitioning from single string/model references to lists across the TestingFarmJobHelper and related methods. This includes updates to argument parsing, payload preparation, and Copr build retrieval. Feedback focuses on improving performance by moving regex compilation outside of methods to align with the style guide and optimizing the construction of the Copr builds dictionary to reduce algorithmic complexity from O(M*N) to O(M+N).
There was a problem hiding this comment.
Code Review
This pull request enables support for multiple PR arguments in the /packit test command by transitioning internal build handling from single objects to lists. Changes include updates to argument parsing, payload generation, and artifact retrieval. Feedback suggests generalizing the hardcoded GitHub URL for other forges and adding missing type hints to improve maintainability.
nforro
force-pushed
the
test-multiple-prs
branch
from
f2873b3 to
aa747f6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Testing Farm helper to support multiple PR arguments in test comment commands. The implementation transitions from handling a single PR argument to a list of arguments, updating the parsing logic, payload generation, and artifact retrieval to accommodate multiple additional Copr builds. The feedback provided identifies several instances where docstrings for new or modified methods do not comply with the repository's mandatory Google-style format.
| """ | ||
| Get the artifacts list from the build (if the skip_build option is not defined) | ||
| and additional build (from other PR) if present. | ||
| and additional builds (from other PRs) if present. | ||
| """ |
There was a problem hiding this comment.
The docstring for _get_artifacts does not follow the Google-style format required by the repository's style guide (line 412). It should include Args and Returns sections to describe its parameters and what it returns.
| """ | |
| Get the artifacts list from the build (if the skip_build option is not defined) | |
| and additional build (from other PR) if present. | |
| and additional builds (from other PRs) if present. | |
| """ | |
| """Get the artifacts for the Testing Farm payload. | |
| Args: | |
| chroot: The chroot for which to get artifacts. | |
| build: The main Copr build for the current job. | |
| additional_builds: A list of additional Copr builds from other PRs. | |
| Returns: | |
| A list of artifact dictionaries for the Testing Farm payload. | |
| """ |
References
- With exception of trivial cases, all code must contain accurate and sufficiently detailed docstrings, formatted with accordance with the PEP 257 standard and in Google-style. (link)
| """ | ||
| Get additional Copr builds if there was a PR argument in the | ||
| Get additional Copr builds if there were PR arguments in the | ||
| test comment command: | ||
|
|
||
| For each PR argument: | ||
| 1. parse the PR argument to get the repo, namespace and PR ID | ||
| 2. get the PR from the DB | ||
| 3. get the copr builds from DB for the given PR model | ||
| 4. filter the most recent successful copr build target models | ||
| 5. construct a dictionary to map the target names to actual models | ||
|
|
||
| Then construct a dictionary to map the target names to actual models. | ||
|
|
||
| Returns: | ||
| dict | ||
| dict mapping chroot to list of builds, or None if no builds found | ||
| """ |
There was a problem hiding this comment.
The docstring for this method has been updated but it still doesn't follow the Google-style format required by the repository's style guide (line 412). It should have a summary line, a more detailed description, and a distinct Returns section.
"""Get Copr builds from PRs specified in a comment.
This method parses PR arguments from a test command comment, fetches the
corresponding Copr builds from the database, and filters for the most
recent successful ones.
Returns:
A dictionary mapping each chroot to a list of successful Copr build
models. Returns None if no successful builds are found for any of
the specified PRs.
"""References
- With exception of trivial cases, all code must contain accurate and sufficiently detailed docstrings, formatted with accordance with the PEP 257 standard and in Google-style. (link)
| """ | ||
| Parse the PR argument from test comment command if there is any. | ||
| Parse a PR argument in format namespace/repo#pr_id. | ||
|
|
||
| Returns: | ||
| tuple of strings for namespace, repo and pr_id | ||
| """ |
There was a problem hiding this comment.
The docstring for this new static method does not follow the Google-style format required by the repository's style guide (line 412). It's missing the Args section, and the Returns section could be more descriptive about the failure case.
"""Parse a PR argument in format namespace/repo#pr_id.
Args:
pr_argument: The string to parse, e.g. 'namespace/repo#123'.
Returns:
A tuple of (namespace, repo, pr_id) on success, otherwise None.
"""References
- With exception of trivial cases, all code must contain accurate and sufficiently detailed docstrings, formatted with accordance with the PEP 257 standard and in Google-style. (link)
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 51s |
Signed-off-by: Nikola Forró <nforro@redhat.com> Assisted-by: Claude Opus 4.6 via Claude Code
nforro
force-pushed
the
test-multiple-prs
branch
from
aa747f6 to
fb1c4b4
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 51s |
4 participants
No description provided.