quality/os-autoinst-distri-fedora

Fork 2

rmdepcheck: handle rdc-repos / rdc-compose split #552

Merged

adamwill merged 1 commit from eln-wrapper into main

2026-05-16 06:37:46 +00:00

adamwill commented

2026-05-15 22:09:53 +00:00

Owner

This adapts to the planned upstream split of rmdepcheck into
rdc-repos and rdc-compose:
quality/rmdepcheck#40
rdc-repos is the 'old' mode, still used for most cases.
rdc-compose is the new mode intended for EL cases, especially ELN;
we use it for ELN now.

Signed-off-by: Adam Williamson awilliam@redhat.com

This adapts to the planned upstream split of rmdepcheck into rdc-repos and rdc-compose: https://forge.fedoraproject.org/quality/rmdepcheck/pulls/40 rdc-repos is the 'old' mode, still used for most cases. rdc-compose is the new mode intended for EL cases, especially ELN; we use it for ELN now. Signed-off-by: Adam Williamson <awilliam@redhat.com>

adamwill added 1 commit

2026-05-15 22:09:55 +00:00

rmdepcheck: handle rdc-repos / rdc-compose split

CI via Tox and perl / tox (pull_request) Successful in 1m22s

Details

CI via Tox and perl / perl (pull_request) Successful in 3m10s

Details

CI via Tox and perl / checkwiki (pull_request) Successful in 33s

Details

AI Code Review / ai-review (pull_request_target) Successful in 22s

Details

982fc029c8

This adapts to the planned upstream split of rmdepcheck into
rdc-repos and rdc-compose:
quality/rmdepcheck#40
rdc-repos is the 'old' mode, still used for most cases.
rdc-compose is the new mode intended for EL cases, especially ELN;
we use it for ELN now.

Signed-off-by: Adam Williamson <awilliam@redhat.com>

adamwill referenced this pull request from quality/rmdepcheck

2026-05-15 22:15:10 +00:00

Add EL-specific interface, refactor (#37) #40

forgejo-actions commented

2026-05-15 22:15:27 +00:00

AI Code Review

Detailed Code Review

The changes adapt the rmdepcheck testing script to support a split into rdc-repos and rdc-compose. The updates successfully swap the execution commands depending on the environment context (ELN vs. standard releases). However, there are potential logic flaws regarding how variables like $nmbaserepo are appended and potentially overwritten when using ELN, and the script uses unprofessional error messages.

📂 File Reviews

📄 `lib/rmdepcheck.pm` - Adapts the repository setup and command construction to differentiate between `rdc-repos` and `rdc-compose`.

Major [Logic]: If lc($version) eq "eln", the $cmd variable is completely overwritten. Any --nmbaserepos flags appended on line 41 are lost. If rdc-compose does not support --nmbaserepos, this should be explicitly documented or handled. If it does, the flag is inadvertently discarded.
- Suggestion: If --nmbaserepos is needed for rdc-compose, append it to the new $cmd. If it isn't supported, structure the conditional so that --nmbaserepos is only evaluated and appended for rdc-repos.

my $cmd;
if (lc($version) eq "eln") {
    die "aargh this doesn't work!" if ($tag || $copr);
    $cmd = "./rdc-compose --arch ${arch} https://kojipkgs.fedoraproject.org/compose/eln/latest-Fedora-eln/compose ";
} else {
    $cmd = "./rdc-repos --arch ${arch}";
    $cmd .= " --nmbaserepos ${nmbaserepo}" if ($nmbaserepo);
    $cmd .= " ${baserepo} ";
}

Minor [Style]: The die message "aargh this doesn't work!" is unprofessional and provides poor context for debugging.
- Suggestion: Replace the error message with a clear description indicating why the combination is invalid.

die "Tags and COPR are not supported when running ELN mode with rdc-compose" if ($tag || $copr);

Questions:

❓ Does rdc-compose intentionally drop support for --nmbaserepos? If so, does the workaround repository logic need to apply to ELN at all?
❓ Does rdc-repos support local paths without the file:// scheme, considering the removal on line 55?

📄 `tests/rmdepcheck_prepare.pm` - Updates test dependencies and checks out a specific feature branch.

Minor [Suggestion]: The script clones a specific branch (eln-wrapper). Once the upstream pull request is merged, this will need to be reverted or updated to track the main branch.
- Suggestion: Ensure there is a trackable plan or ticket to switch this back to the main branch once the upstream changes land.

✅ Summary

Overall Assessment: The execution split has been fundamentally implemented, but the command construction logic for ELN inadvertently drops previously appended flags.
Priority Issues:
- Address the overwritten $cmd logic for ELN which drops $nmbaserepo.
Minor Suggestions:
- Update the die message to be more descriptive and professional.
- Plan for updating the cloned git branch in rmdepcheck_prepare.pm after upstream merge.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### Detailed Code Review The changes adapt the `rmdepcheck` testing script to support a split into `rdc-repos` and `rdc-compose`. The updates successfully swap the execution commands depending on the environment context (ELN vs. standard releases). However, there are potential logic flaws regarding how variables like `$nmbaserepo` are appended and potentially overwritten when using ELN, and the script uses unprofessional error messages. #### 📂 File Reviews <details> <summary>📄 `lib/rmdepcheck.pm` - Adapts the repository setup and command construction to differentiate between `rdc-repos` and `rdc-compose`.</summary> - **Major** [Logic]: If `lc($version) eq "eln"`, the `$cmd` variable is completely overwritten. Any `--nmbaserepos` flags appended on line 41 are lost. If `rdc-compose` does not support `--nmbaserepos`, this should be explicitly documented or handled. If it does, the flag is inadvertently discarded. - *Suggestion:* If `--nmbaserepos` is needed for `rdc-compose`, append it to the new `$cmd`. If it isn't supported, structure the conditional so that `--nmbaserepos` is only evaluated and appended for `rdc-repos`. ```` my $cmd; if (lc($version) eq "eln") { die "aargh this doesn't work!" if ($tag || $copr); $cmd = "./rdc-compose --arch ${arch} https://kojipkgs.fedoraproject.org/compose/eln/latest-Fedora-eln/compose "; } else { $cmd = "./rdc-repos --arch ${arch}"; $cmd .= " --nmbaserepos ${nmbaserepo}" if ($nmbaserepo); $cmd .= " ${baserepo} "; } ```` - **Minor** [Style]: The `die` message `"aargh this doesn't work!"` is unprofessional and provides poor context for debugging. - *Suggestion:* Replace the error message with a clear description indicating why the combination is invalid. ```` die "Tags and COPR are not supported when running ELN mode with rdc-compose" if ($tag || $copr); ```` **Questions:** - ❓ Does `rdc-compose` intentionally drop support for `--nmbaserepos`? If so, does the workaround repository logic need to apply to ELN at all? - ❓ Does `rdc-repos` support local paths without the `file://` scheme, considering the removal on line 55? </details> <details> <summary>📄 `tests/rmdepcheck_prepare.pm` - Updates test dependencies and checks out a specific feature branch.</summary> - **Minor** [Suggestion]: The script clones a specific branch (`eln-wrapper`). Once the upstream pull request is merged, this will need to be reverted or updated to track the main branch. - *Suggestion:* Ensure there is a trackable plan or ticket to switch this back to the main branch once the upstream changes land. </details> ### ✅ Summary - **Overall Assessment:** The execution split has been fundamentally implemented, but the command construction logic for ELN inadvertently drops previously appended flags. - **Priority Issues:** - Address the overwritten `$cmd` logic for ELN which drops `$nmbaserepo`. - **Minor Suggestions:** - Update the `die` message to be more descriptive and professional. - Plan for updating the cloned git branch in `rmdepcheck_prepare.pm` after upstream merge. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*

adamwill force-pushed eln-wrapper from 982fc029c8

CI via Tox and perl / tox (pull_request) Successful in 1m22s

Details

CI via Tox and perl / perl (pull_request) Successful in 3m10s

Details

CI via Tox and perl / checkwiki (pull_request) Successful in 33s

Details

AI Code Review / ai-review (pull_request_target) Successful in 22s

Details

to 4dc1786278

CI via Tox and perl / tox (pull_request) Successful in 1m18s

Details

CI via Tox and perl / perl (pull_request) Successful in 3m12s

Details

CI via Tox and perl / checkwiki (pull_request) Successful in 33s

Details

AI Code Review / ai-review (pull_request_target) Successful in 23s

Details

2026-05-16 06:30:27 +00:00

Compare

adamwill commented

2026-05-16 06:32:22 +00:00

Author

Owner

Does rdc-compose intentionally drop support for --nmbaserepos?

Yes, or rather, it decides them on its own. Fine, I tweaked it so we don't re-declare $cmd, but it's actually fine.

If so, does the workaround repository logic need to apply to ELN at all?

Eh, we'll deal with that somehow if it ever becomes an issue, I guess.

Does rdc-repos support local paths without the file:// scheme, considering the removal on line 55?

Yes it does.

Minor [Style]: The die message "aargh this doesn't work!" is unprofessional and provides poor context for debugging

I don't care, I'm not being professional. This is a note-to-self, basically.

> Does rdc-compose intentionally drop support for --nmbaserepos? Yes, or rather, it decides them on its own. Fine, I tweaked it so we don't re-declare `$cmd`, but it's actually fine. > If so, does the workaround repository logic need to apply to ELN at all? Eh, we'll deal with that somehow if it ever becomes an issue, I guess. > Does rdc-repos support local paths without the file:// scheme, considering the removal on line 55? Yes it does. > Minor [Style]: The die message "aargh this doesn't work!" is unprofessional and provides poor context for debugging I don't care, I'm not being professional. This is a note-to-self, basically.

forgejo-actions commented

2026-05-16 06:35:59 +00:00

AI Code Review

Detailed Code Review

The changes successfully implement the transition to the upstream rmdepcheck split. The logic correctly routes ELN tests to rdc-compose and standard ones to rdc-repos while adhering to the constraints discussed (like intentionally omitting --nmbaserepos for rdc-compose). The script preparation correctly includes the newly required Python dependencies.

The only notable area for improvement is error handling. A more descriptive exception message when attempting to use ELN with tags/COPR would greatly assist in debugging future failures. Aside from that, the code behaves exactly as expected.

📂 File Reviews

📄 `lib/rmdepcheck.pm` - Switches the execution command between `rdc-compose` for ELN and `rdc-repos` for standard execution, adjusting repository inputs.

Minor [Style]: The die message "aargh this doesn't work!" is not very descriptive for diagnosing failures in automated test logs.
- Suggestion: Use a more explicit error message describing why it's failing (e.g., indicating that rdc-compose lacks tag/copr support).

die "Tags and COPR are not currently supported with ELN / rdc-compose" if ($tag || $copr);

✅ Summary

Overall Assessment: The review is complete and no critical issues were found. The implementation correctly handles the upstream tool split.
Minor Suggestions:
- Improve the error message when ELN is combined with $tag or $copr to provide better debugging context.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### Detailed Code Review The changes successfully implement the transition to the upstream `rmdepcheck` split. The logic correctly routes ELN tests to `rdc-compose` and standard ones to `rdc-repos` while adhering to the constraints discussed (like intentionally omitting `--nmbaserepos` for `rdc-compose`). The script preparation correctly includes the newly required Python dependencies. The only notable area for improvement is error handling. A more descriptive exception message when attempting to use ELN with tags/COPR would greatly assist in debugging future failures. Aside from that, the code behaves exactly as expected. #### 📂 File Reviews <details> <summary>📄 `lib/rmdepcheck.pm` - Switches the execution command between `rdc-compose` for ELN and `rdc-repos` for standard execution, adjusting repository inputs.</summary> - **Minor** [Style]: The `die` message "aargh this doesn't work!" is not very descriptive for diagnosing failures in automated test logs. - *Suggestion:* Use a more explicit error message describing why it's failing (e.g., indicating that `rdc-compose` lacks tag/copr support). ```` die "Tags and COPR are not currently supported with ELN / rdc-compose" if ($tag || $copr); ```` </details> ### ✅ Summary - **Overall Assessment:** The review is complete and no critical issues were found. The implementation correctly handles the upstream tool split. - **Minor Suggestions:** - Improve the error message when ELN is combined with `$tag` or `$copr` to provide better debugging context. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*

adamwill merged commit 4dc1786278 into main

2026-05-16 06:37:46 +00:00

adamwill deleted branch eln-wrapper

2026-05-16 06:37:47 +00:00

adamwill referenced this pull request

2026-05-19 06:56:40 +00:00

Adjust rmdepcheck to use EL wrapper when it's ready #539