Allow retriggering builds/tests on ELN rawhide PRs independently#3032
Allow retriggering builds/tests on ELN rawhide PRs independently#3032betulependule wants to merge 9 commits intopackit:mainfrom
Conversation
betulependule
requested review from
a team,
lbarcziova,
mfocko and
nforro
as code owners
![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 introduces a mechanism to independently retrigger builds and tests for eln and rawhide on Fedora CI pull requests. The implementation adds a new argument to the comment parser and a decorator to map handlers to specific branches. The changes are well-implemented and include thorough test coverage.
My feedback includes a suggestion to improve the user experience by changing the new command-line argument from positional to a keyword argument, which would make it more explicit and less prone to user error. I also noted a minor type inconsistency in a new helper function and provided a fix.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
betulependule
force-pushed
the
trigger-job-for-branch
branch
2 times, most recently
from
9bf9816 to
689a6b2
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
betulependule
force-pushed
the
trigger-job-for-branch
branch
from
689a6b2 to
5f888f5
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
What's invalid about this? Isn't it the same pattern as |
That command is definitely valid. The line regarding a command not being valid was referring to the following command below:
I've edited |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 53s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 48s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 51s |
|
✔️ pre-commit SUCCESS in 1m 48s |
betulependule
force-pushed
the
trigger-job-for-branch
branch
from
7863486 to
bf7cdb6
Compare
|
✔️ pre-commit SUCCESS in 1m 47s |
betulependule
force-pushed
the
trigger-job-for-branch
branch
from
bf7cdb6 to
f4a319b
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
| MAP_TARGET_BRANCH_TO_HANDLER: dict[type["FedoraCIJobHandler"], str] = defaultdict(str) | ||
|
|
||
|
|
||
| def corresponds_to_check_target_branch(check_target_branch: str): | ||
| """ | ||
| [class decorator] | ||
| Specify which target branch the handler corresponds to. | ||
|
|
||
| Normally, when retriggering jobs for eln packages on a PR against the | ||
| rawhide branch, jobs would be run for both the rawhide and eln targets. | ||
| When the check target branch is specified like this, | ||
|
|
||
| /packit-ci test installability rawhide | ||
|
|
||
| then only the handler corresponding to the specified target branch | ||
| (rawhide in this example) will be run, resulting in jobs being run | ||
| only for the desired target. | ||
|
|
||
| Example: | ||
| ``` | ||
| @corresponds_to_check_target_branch(check_target_branch="rawhide") | ||
| class DownstreamKojiScratchBuildHandler( | ||
| ``` | ||
| """ | ||
|
|
||
| def _add_to_mapping(kls: type["FedoraCIJobHandler"]): | ||
| MAP_TARGET_BRANCH_TO_HANDLER[kls] = check_target_branch | ||
| return kls | ||
|
|
||
| return _add_to_mapping | ||
|
|
There was a problem hiding this comment.
While this isn't user facing, it's still technically incorrect to call it target branch. Could you rename the variables/functions?
There was a problem hiding this comment.
Yes definitely. Thank you for pointing this out.
| def report_task_accepted_for_fedora_ci( | ||
| self, | ||
| handler_kls: type[FedoraCIJobHandler], | ||
| user_specified_target_branch: Optional[str] = None, |
There was a problem hiding this comment.
This one is a bit tricky, FedoraCIHelper has target_branch attribute that comes from a target_branch argument of StatusReporter.set_status() and was originally used for target branch, but now it's actually the build target (in eln and main cases). But I would probably keep this as it is.
packit_service/utils.py
Outdated
| test_parser = subparsers.add_parser("test", help="Run tests in Testing Farm") | ||
| test_parser.add_argument( | ||
| "target", | ||
| "test_type", |
There was a problem hiding this comment.
In the docs we call these test identifiers, I often refer to them as checks, technically they are fmf/tmt test plans, it's a bit of a mess 😅 We should figure out consistent naming, but for this particular case test_identifier sounds better to me than test_type.
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
The check should pass when using the `--target-branch` argument. There is no need to skip it.
Co-authored-by: Nikola Forró <nforro@redhat.com>
Fedora CI arguments have been changed for clarity and factual correctness.
The functionality defined in `filter_handlers_based_on_branch_fedora_ci` has been moved to `get_handlers_for_command_fedora_ci`.
betulependule
force-pushed
the
trigger-job-for-branch
branch
from
7a3e997 to
82ca6e8
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
nforro
left a comment
nforro
left a comment
There was a problem hiding this comment.
Thanks! Could you just squash the commits?
3 participants
TODO:
packit/packit.dev.NOTE 1
When figuring out how to implement this PR, I noticed that separate handlers are run when running jobs for
elnandrawhide. This implementation is based on discarding the one handler associated with the jobs that are not needed (removing it from the handlers_triggered_by_job variable). For example:/packit-ci test rpminspect elnWould lead to the handler corresponding with the
rawhidetarget being removed from the list of handlers to be run. It's possible that there could be a better way of implementing this.NOTE 2
I implemented the retriggering comment syntax as was described in the issue. However, having it implemented as a positional argument means that comments like this are valid:
/packit-ci test custom eln/packit-ci build rawhideBut the following would be invalid and lead to a parsing error because of the missing positional argument before
eln(the first positional argument cannot be missing if the second one is present):/packit-ci test elnIf needed, I could instead turn the new argument into a keyword argument like this:
/packit-ci test --target-branch elnEDIT: I've made it into a keyword argument in the second commit. I should probably write a new test to test the functionality of
/packit-ci test --target-branch eln.Fixes #2958
RELEASE NOTES BEGIN
In Fedora CI, it is now possible to retrigger builds and tests on ELN rawhide PRs independently.
RELEASE NOTES END