Allow skipping 'basetest' or 'newtest' phases (#37)

adamwill commented

2026-04-29 23:04:17 +00:00

Owner

This allows you to skip either the 'basetest' or 'newtest'
'phase'. If you skip basetest, we will only run checks on
the new package repository against the modified base repos;
if you skip newtest, we will only run checks on the modified
base repos.

This is intended to be used for ELN. ELN's buildroot contains
more packages than are in its published repositories. The ELN
maintainers don't consider repoclosure of the buildroot as a
goal. So for the base phase we should check the published
repositories. However, for the newtest phase, we need to check
against the buildroot, because the packages under test may be
destined for the buildroot and thus have buildroot-only
dependencies. We don't want to report a failure if their
dependencies cannot be satisfied from the published repositories.

We can cover both cases by allowing the phases to be run
separately, so for ELN we will run the basetest phase against
the published repositories, and the newtest phase against the
buildroot repository.

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

This allows you to skip either the 'basetest' or 'newtest' 'phase'. If you skip basetest, we will only run checks on the new package repository against the modified base repos; if you skip newtest, we will only run checks on the modified base repos. This is intended to be used for ELN. ELN's buildroot contains more packages than are in its published repositories. The ELN maintainers don't consider repoclosure of the buildroot as a goal. So for the base phase we should check the published repositories. However, for the newtest phase, we need to check against the buildroot, because the packages under test may be destined for the buildroot and thus have buildroot-only dependencies. We don't want to report a failure if their dependencies cannot be satisfied from the published repositories. We can cover both cases by allowing the phases to be run separately, so for ELN we will run the basetest phase against the published repositories, and the newtest phase against the buildroot repository. Signed-off-by: Adam Williamson <awilliam@redhat.com>

adamwill added 1 commit

2026-04-29 23:04:17 +00:00

Allow skipping 'installability' or 'repoclosure' (#37 )

CI via Tox / tox (pull_request) Successful in 1m30s

Details

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

Details

15047e9b56

This allows you to skip either the installability or
repoclosure 'phase'. If you skip installability, we will only
check repoclosure of the modified base repositories, we will
not check repoclosure of the new package repository. If you
skip repoclosure, it's vice versa.

This is intended to be used for ELN. ELN's buildroot contains
more packages than are in its published repositories. The ELN
maintainers don't consider repoclosure of the buildroot as a
goal. So for the 'repoclosure' phase we should check the
published repositories. However, for the 'installability' phase,
we need to check against the buildroot, because the packages
under test may be destined for the buildroot and thus have
buildroot-only dependencies. We don't want to report a failure
if their dependencies cannot be satisfied from the published
repositories.

We can cover both cases by allowing the phases to be run
separately, so for ELN we will run the repoclosure phase against
the published repositories, and the installability phase against
the buildroot repository.

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

forgejo-actions commented

2026-04-29 23:07:21 +00:00

AI Code Review

📋 MR Summary

Adds a --skip argument to bypass either the 'basetest' or 'newtest' phase, primarily to support the ELN buildroot use case.

Key Changes:
- Added --skip argument to argparse with choices 'basetest' and 'newtest'.
- Updated get_modified_and_new_repoclosure to conditionally skip executing repoclosure for the specified phase.
- Added end-to-end tests for the ELN scenario using new test repository data.
Impact: rmdepcheck.py, tests
Risk Level: 🟢 Low - The changes are straightforward, additive, and well-covered by new end-to-end tests without altering the default execution path.

Detailed Code Review

The implementation neatly solves the ELN buildroot problem by allowing selective phase execution. The logic for skipping the subprocess calls is correct and preserves existing behavior when the flag is not used. The included end-to-end tests robustly verify the new functionality using mock repositories. There is only a minor type-hinting issue regarding the default value of the new argument from argparse.

📂 File Reviews

📄 `rmdepcheck.py` - Introduces the `--skip` argument and modifies the core function to respect it.

Minor [Bug]: The skip parameter in get_modified_and_new_repoclosure is typed as str. However, if the --skip argument is not provided via the CLI, argparse assigns it a value of None. This will cause a type-hinting violation (e.g., for mypy) when args.skip is passed to the function.
- Suggestion: Change the type hint to allow None.

def get_modified_and_new_repoclosure(
    brepos: list[str],
    nmrepos: list[str],
    nrepos: list[str],
    removes: Iterable[str],
    skip: str | None,
) -> tuple[str, str]:

✅ Summary

Overall Assessment: The changes successfully implement the requested feature with thorough test coverage. A minor type hint adjustment is recommended.
Minor Suggestions:
- Update the type hint for the skip parameter in get_modified_and_new_repoclosure to accommodate None.

🤖 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 ### 📋 MR Summary Adds a `--skip` argument to bypass either the 'basetest' or 'newtest' phase, primarily to support the ELN buildroot use case. - **Key Changes:** - Added `--skip` argument to argparse with choices 'basetest' and 'newtest'. - Updated `get_modified_and_new_repoclosure` to conditionally skip executing `repoclosure` for the specified phase. - Added end-to-end tests for the ELN scenario using new test repository data. - **Impact:** rmdepcheck.py, tests - **Risk Level:** 🟢 Low - The changes are straightforward, additive, and well-covered by new end-to-end tests without altering the default execution path. ### Detailed Code Review The implementation neatly solves the ELN buildroot problem by allowing selective phase execution. The logic for skipping the subprocess calls is correct and preserves existing behavior when the flag is not used. The included end-to-end tests robustly verify the new functionality using mock repositories. There is only a minor type-hinting issue regarding the default value of the new argument from `argparse`. #### 📂 File Reviews <details> <summary><strong>📄 `rmdepcheck.py`</strong> - Introduces the `--skip` argument and modifies the core function to respect it.</summary> - **Minor** [Bug]: The `skip` parameter in `get_modified_and_new_repoclosure` is typed as `str`. However, if the `--skip` argument is not provided via the CLI, `argparse` assigns it a value of `None`. This will cause a type-hinting violation (e.g., for mypy) when `args.skip` is passed to the function. - *Suggestion:* Change the type hint to allow `None`. ```` def get_modified_and_new_repoclosure( brepos: list[str], nmrepos: list[str], nrepos: list[str], removes: Iterable[str], skip: str | None, ) -> tuple[str, str]: ```` </details> ### ✅ Summary - **Overall Assessment:** The changes successfully implement the requested feature with thorough test coverage. A minor type hint adjustment is recommended. - **Minor Suggestions:** - Update the type hint for the `skip` parameter in `get_modified_and_new_repoclosure` to accommodate `None`. --- 🤖 **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 alternate-base from 15047e9b56

CI via Tox / tox (pull_request) Successful in 1m30s

Details

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

Details

to b269b5e617

CI via Tox / tox (pull_request) Successful in 1m32s

Details

2026-04-29 23:09:44 +00:00

Compare

adamwill changed title from ~~Allow skipping 'installability' or 'repoclosure' (#37)~~ to Allow skipping 'basetest' or 'newtest' phases (#37)

2026-04-29 23:10:14 +00:00

adamwill commented

2026-04-29 23:13:14 +00:00

Author

Owner

This is an alternative to #38 . This one is simpler and, I think, allows us to be more correct, because we can ensure the buildroot is not in scope for the solver during the basetest phase - with the #38 approach it will be, which could cause false passes in corner cases, I guess.

The main drawback of this is it does mean we have to run rmdepcheck twice on ELN, and cope with that in the pipeline somehow. I guess we either report two separate results, or we have to combine the exit codes and outputs of the two executions into one.

@yselkowitz @lecris thoughts?

This is an alternative to #38 . This one is simpler and, I think, allows us to be more correct, because we can ensure the buildroot is not in scope for the solver during the basetest phase - with the #38 approach it will be, which could cause false passes in corner cases, I guess. The main drawback of this is it does mean we have to run rmdepcheck twice on ELN, and cope with that in the pipeline somehow. I guess we either report two separate results, or we have to combine the exit codes and outputs of the two executions into one. @yselkowitz @lecris thoughts?

adamwill referenced this pull request

2026-04-29 23:14:00 +00:00

Add non-checked base repo concept (#37) #38

adamwill commented

2026-04-29 23:15:51 +00:00

Author

Owner

It would be possible to write this such that we always run both phases, but you can configure different sets of base repos for each. It just felt awkward, as it would mean adding I guess two new args - --newbaserepos and --newnmbaserepos, I guess - and the appropriate logic to run through get_modified_and_new_repoclosure twice internally, or even have it internally run twice...I dunno, this seemed simpler. But maybe that would still be simpler than this plus the required pipeline tweaks?

It would be *possible* to write this such that we always run both phases, but you can configure different sets of base repos for each. It just felt awkward, as it would mean adding I guess two new args - `--newbaserepos` and `--newnmbaserepos`, I guess - and the appropriate logic to run through `get_modified_and_new_repoclosure` twice internally, or even have it internally run twice...I dunno, this seemed simpler. But maybe that would still be simpler than this plus the required pipeline tweaks?

adamwill referenced this pull request from ci/tickets

2026-04-29 23:20:14 +00:00

ELN want rmdepcheck to run against composed repos, not buildroot #555

adamwill referenced this pull request from quality/paperwork

2026-04-30 07:00:30 +00:00

openQA failure monitoring 2026 sprint 8 #37

adamwill commented

2026-04-30 07:05:14 +00:00

Author

Owner

Having stewed on it, I think I'll try and come up with the least ugly internal-two-phase version of this that I can tomorrow. It just makes it easier on the pipelines if we can do it that way.

adamwill commented

2026-05-04 20:48:44 +00:00

Author

Owner

Actually it turns out this was not sophisticated enough. I'm pretty sure I'll be going with some version of #40 here, so closing this.

adamwill closed this pull request

2026-05-04 20:48:44 +00:00

CI via Tox / tox (pull_request) Successful in 1m32s

Details

Pull request closed

Please reopen this pull request to perform a merge.

Rows
Columns

Allow skipping 'basetest' or 'newtest' phases (#37) #39

AI Code Review

📋 MR Summary

Detailed Code Review

📂 File Reviews

✅ Summary

Pull request closed