Add EL-specific interface, refactor (#37)

adamwill commented

2026-05-01 01:33:35 +00:00

Owner

After much discussion with @yselkowitz, this should be the best
option for ELN testing, I hope.

ELN (like other ELs) has multiple variants with repositories.
There is a mapping in Pungi config of relationships between the
variants. Each variant is expected to be repoclosure-complete,
possibly in itself, possibly with reference to some or all of
the other variants. Each variant contains only a subset of the
packages in the buildroot.

We have been asked to make it so that, given an arbitrary set of
"packages under test" for a given EL, the packages it contains
from each variant should be tested separately against that
variant, so we fail if replacing the packages in the variant with
the matching packages from the set under test would cause new
repoclosure issues in that variant, or the packages under test
for that variant have installability issues when tested against
the variant and any others it "depends" on.

Packages from the set under test that are not in any variant
should be ignored, as repoclosure of and installability within
the buildroot is not currently desired to be tested.

This wrapper script takes as inputs a compose that we should test
against, and a filesystem path containing the packages to be
tested. The compose is usually expected to be the latest compose
of the EL under test. Using the metadata, we discover all variants
in the compose that have repositories (except Buildroot, which is
ignored). We then list out the binary packages by name in each
variant repository.

We then split the set of packages-under-test into several temporary
variant repositories: each package is placed in the repo for all
variants of which the identically-named package in the compose is a
part, if any.

We then get and parse a copy of the variant mapping from the
Pungi config. Then, for each "populated" variant (that is, a
variant for which the packages-under-test set contains at least
one package), we run rmdepcheck, with the compose repos for the
variant plus any variants it "depends" on as the baserepos, the
local temporary variant repository as the newrepo, and the local
temp repos for all "depended-on" variants as addrepos. All
rmdepcheck output is passed through.

We track the return codes of each rmdepcheck run and return the
sum of each unique return code.

In the second commit, we refactor the project into a package containing a large shared module and two independent front-end scripts, rdc-repos and rdc-compose. It can theoretically be installed, but typically you'd just run it out of the checkout with the handy top-level launchers of the same name. This reduces code duplication between the scripts and allows rdc-compose to run faster. We also add tests for rdc-compose, and fix a bug I found while writing the tests.

After much discussion with @yselkowitz, this should be the best option for ELN testing, I hope. ELN (like other ELs) has multiple variants with repositories. There is a mapping in Pungi config of relationships between the variants. Each variant is expected to be repoclosure-complete, possibly in itself, possibly with reference to some or all of the other variants. Each variant contains only a subset of the packages in the buildroot. We have been asked to make it so that, given an arbitrary set of "packages under test" for a given EL, the packages it contains from each variant should be tested separately against that variant, so we fail if replacing the packages in the variant with the matching packages from the set under test would cause new repoclosure issues in that variant, or the packages under test for that variant have installability issues when tested against the variant and any others it "depends" on. Packages from the set under test that are not in any variant should be ignored, as repoclosure of and installability within the buildroot is not currently desired to be tested. This wrapper script takes as inputs a compose that we should test against, and a filesystem path containing the packages to be tested. The compose is usually expected to be the latest compose of the EL under test. Using the metadata, we discover all variants in the compose that have repositories (except Buildroot, which is ignored). We then list out the binary packages by name in each variant repository. We then split the set of packages-under-test into several temporary variant repositories: each package is placed in the repo for all variants of which the identically-named package in the compose is a part, if any. We then get and parse a copy of the variant mapping from the Pungi config. Then, for each "populated" variant (that is, a variant for which the packages-under-test set contains at least one package), we run rmdepcheck, with the compose repos for the variant plus any variants it "depends" on as the baserepos, the local temporary variant repository as the newrepo, and the local temp repos for all "depended-on" variants as addrepos. All rmdepcheck output is passed through. We track the return codes of each rmdepcheck run and return the sum of each unique return code. In the second commit, we refactor the project into a package containing a large `shared` module and two independent front-end scripts, `rdc-repos` and `rdc-compose`. It can theoretically be installed, but typically you'd just run it out of the checkout with the handy top-level launchers of the same name. This reduces code duplication between the scripts and allows `rdc-compose` to run faster. We also add tests for `rdc-compose`, and fix a bug I found while writing the tests.

👀 1

adamwill added 1 commit

2026-05-01 01:33:35 +00:00

Add rdc-el-wrapper, an opinionated wrapper for ELN testing (#37 )

CI via Tox / tox (pull_request) Failing after 1m27s

Details

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

Details

e7aa59ba63

After much discussion with @yselkowitz, this should be the best
option for ELN testing, I hope.

ELN (like other ELs) has multiple variants with repositories.
There is a mapping in Pungi config of relationships between the
variants. Each variant is expected to be repoclosure-complete,
possibly in itself, possibly with reference to some or all of
the other variants. Each variant contains only a subset of the
packages in the buildroot.

We have been asked to make it so that, given an arbitrary set of
"packages under test" for a given EL, the packages it contains
from each variant should be tested separately against that
variant, so we fail if replacing the packages in the variant with
the matching packages from the set under test would cause new
repoclosure issues in that variant, or the packages under test
for that variant have installability issues when tested against
the variant and any others it "depends" on.

Packages from the set under test that are not in any variant
should be ignored, as repoclosure of and installability within
the buildroot is not currently desired to be tested.

This wrapper script takes as inputs a compose that we should test
against, and a filesystem path containing the packages to be
tested. The compose is usually expected to be the latest compose
of the EL under test. Using the metadata, we discover all variants
in the compose that have repositories (except Buildroot, which is
ignored). We then list out the binary packages by name in each
variant repository.

We then split the set of packages-under-test into several temporary
variant repositories: each package is placed in the repo for all
variants of which the identically-named package in the compose is a
part, if any.

We then get and parse a copy of the variant mapping from the
Pungi config. Then, for each "populated" variant (that is, a
variant for which the packages-under-test set contains at least
one package), we run rmdepcheck, with the compose repos for the
variant plus any variants it "depends" on as the baserepos, the
local temporary variant repository as the newrepo, and the local
temp repos for all "depended-on" variants as addrepos. All
rmdepcheck output is passed through.

We track the return codes of each rmdepcheck run and return the
sum of each unique return code.

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

adamwill commented

2026-05-01 01:35:20 +00:00

Author

Owner

This isn't totally ready yet but I wanted to put it up for anyone interested to take a look. CC @lecris

forgejo-actions commented

2026-05-01 01:35:28 +00:00

AI Code Review

📋 MR Summary

Adds rdc-el-wrapper, a script to handle Enterprise Linux specific logic for running rmdepcheck against multiple variants with dependencies.

Key Changes:
- Introduces rdc-el-wrapper.py script.
- Implements logic to fetch compose metadata and variant relationships.
- Splits local packages-under-test into per-variant temporary repositories.
- Executes rmdepcheck.py against populated variant repositories.
Impact: rdc-el-wrapper.py, README.md
Risk Level: 🟡 Medium - The script handles file system operations, executes subroutines, and performs external HTTP requests. It contains missing validations, brittle text parsing, and assumptions about file systems that could lead to runtime exceptions.

Detailed Code Review

The implementation correctly identifies the complex workflow required for EL variant testing but lacks critical safety checks and error handling. Missing checks for external dependencies (createrepo), fragile string slicing for configuration extraction, and missing HTTP response validations will cause the script to fail unexpectedly on errors. Additionally, using hard links between arbitrary paths can fail with cross-device link errors.

📂 File Reviews

📄 `rdc-el-wrapper.py` - Implements the wrapper logic for finding compose variants, splitting packages, and invoking rmdepcheck.

Critical [Bug]: Using os.link can fail with OSError: [Errno 18] Invalid cross-device link if the input directory and /var/tmp are on different file systems.
- Suggestion: Fall back to copying the file if hardlinking fails.

import shutil

try:
    os.link(put / _file, repotemp / variant / _file)
except OSError:
    shutil.copy2(put / _file, repotemp / variant / _file)

Major [Bug]: createrepo is executed via subprocess.run without verifying if the utility is installed or if the command succeeds.
- Suggestion: Check for createrepo alongside dnf and enforce success on the subprocess call.

# In check_dnf (rename to check_dependencies):
try:
    subprocess.run(("dnf", "--version"), stdout=subprocess.DEVNULL, check=True)
    subprocess.run(("createrepo", "--version"), stdout=subprocess.DEVNULL, check=True)
except FileNotFoundError:
    sys.exit("Please install missing required utilities: dnf, createrepo")

# In split_put:
subprocess.run(("createrepo", str(repotemp / variant)), capture_output=True, check=True)

Major [Logic]: HTTP requests lack response validation. If the server returns a 404 or 500, .json() or .text processing will fail cryptically.
- Suggestion: Call .raise_for_status() on the responses before processing.

response = SESSION.get(f"{compose}/metadata/composeinfo.json")
response.raise_for_status()
ci = response.json()

Major [Logic]: String parsing in get_val() relies on exact substrings (variant_as_lookaside = ). If the remote configuration formatting changes slightly (e.g., missing spaces), override.index() throws a ValueError.
- Suggestion: Use regular expressions to extract the python list structure safely.

import re

override = SESSION.get("https://forge.fedoraproject.org/releng/pungi-fedora/raw/branch/eln/fedora/override.conf").text
match = re.search(r"variant_as_lookaside\s*=\s*(\[.*?\])", override, re.DOTALL)
if not match:
    raise ValueError("Could not find variant_as_lookaside mapping in configuration.")
return ast.literal_eval(match.group(1))

Minor [Logic]: The comment in url_check says 'If it's an existing local path, convert to file:// URI', but the code only checks for http or https and raises an error otherwise.
- Suggestion: Either implement the local path conversion or fix the comment to reflect the actual validation logic.

def url_check(arg: str) -> str:
    """Check arg is an http or https URL."""
    parsed = urlparse(arg)
    if parsed.scheme in ("http", "https"):
        return arg
    if parsed.scheme:
        raise ValueError(f"Unsupported URL scheme {parsed.scheme} in {arg}")
    raise ValueError(f"No URL scheme in {arg}")

✅ Summary

Overall Assessment: The script requires fixes for file system operations, dependency validation, and HTTP response handling to prevent unhandled runtime errors.
Priority Issues:
- Fix os.link vulnerability to cross-device file system errors.
- Validate that createrepo is installed and enforce command success.
- Add HTTP response status checks before parsing JSON or text.
- Implement robust extraction of the variant mapping to prevent ValueError.
Minor Suggestions:
- Align the implementation and documentation of url_check.
- Consider capturing and logging the output of createrepo if it fails.
- Avoid creating global tempfile.TemporaryDirectory instances which execute on import.

🤖 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 `rdc-el-wrapper`, a script to handle Enterprise Linux specific logic for running `rmdepcheck` against multiple variants with dependencies. - **Key Changes:** - Introduces `rdc-el-wrapper.py` script. - Implements logic to fetch compose metadata and variant relationships. - Splits local packages-under-test into per-variant temporary repositories. - Executes `rmdepcheck.py` against populated variant repositories. - **Impact:** rdc-el-wrapper.py, README.md - **Risk Level:** 🟡 Medium - The script handles file system operations, executes subroutines, and performs external HTTP requests. It contains missing validations, brittle text parsing, and assumptions about file systems that could lead to runtime exceptions. ### Detailed Code Review The implementation correctly identifies the complex workflow required for EL variant testing but lacks critical safety checks and error handling. Missing checks for external dependencies (`createrepo`), fragile string slicing for configuration extraction, and missing HTTP response validations will cause the script to fail unexpectedly on errors. Additionally, using hard links between arbitrary paths can fail with cross-device link errors. #### 📂 File Reviews <details> <summary>📄 `rdc-el-wrapper.py` - Implements the wrapper logic for finding compose variants, splitting packages, and invoking rmdepcheck.</summary> - **Critical** [Bug]: Using `os.link` can fail with `OSError: [Errno 18] Invalid cross-device link` if the input directory and `/var/tmp` are on different file systems. - *Suggestion:* Fall back to copying the file if hardlinking fails. ```` import shutil try: os.link(put / _file, repotemp / variant / _file) except OSError: shutil.copy2(put / _file, repotemp / variant / _file) ```` - **Major** [Bug]: `createrepo` is executed via `subprocess.run` without verifying if the utility is installed or if the command succeeds. - *Suggestion:* Check for `createrepo` alongside `dnf` and enforce success on the subprocess call. ```` # In check_dnf (rename to check_dependencies): try: subprocess.run(("dnf", "--version"), stdout=subprocess.DEVNULL, check=True) subprocess.run(("createrepo", "--version"), stdout=subprocess.DEVNULL, check=True) except FileNotFoundError: sys.exit("Please install missing required utilities: dnf, createrepo") # In split_put: subprocess.run(("createrepo", str(repotemp / variant)), capture_output=True, check=True) ```` - **Major** [Logic]: HTTP requests lack response validation. If the server returns a 404 or 500, `.json()` or `.text` processing will fail cryptically. - *Suggestion:* Call `.raise_for_status()` on the responses before processing. ```` response = SESSION.get(f"{compose}/metadata/composeinfo.json") response.raise_for_status() ci = response.json() ```` - **Major** [Logic]: String parsing in `get_val()` relies on exact substrings (`variant_as_lookaside = `). If the remote configuration formatting changes slightly (e.g., missing spaces), `override.index()` throws a `ValueError`. - *Suggestion:* Use regular expressions to extract the python list structure safely. ```` import re override = SESSION.get("https://forge.fedoraproject.org/releng/pungi-fedora/raw/branch/eln/fedora/override.conf").text match = re.search(r"variant_as_lookaside\s*=\s*(\[.*?\])", override, re.DOTALL) if not match: raise ValueError("Could not find variant_as_lookaside mapping in configuration.") return ast.literal_eval(match.group(1)) ```` - **Minor** [Logic]: The comment in `url_check` says 'If it's an existing local path, convert to file:// URI', but the code only checks for `http` or `https` and raises an error otherwise. - *Suggestion:* Either implement the local path conversion or fix the comment to reflect the actual validation logic. ```` def url_check(arg: str) -> str: """Check arg is an http or https URL.""" parsed = urlparse(arg) if parsed.scheme in ("http", "https"): return arg if parsed.scheme: raise ValueError(f"Unsupported URL scheme {parsed.scheme} in {arg}") raise ValueError(f"No URL scheme in {arg}") ```` </details> ### ✅ Summary - **Overall Assessment:** The script requires fixes for file system operations, dependency validation, and HTTP response handling to prevent unhandled runtime errors. - **Priority Issues:** - Fix `os.link` vulnerability to cross-device file system errors. - Validate that `createrepo` is installed and enforce command success. - Add HTTP response status checks before parsing JSON or text. - Implement robust extraction of the variant mapping to prevent `ValueError`. - **Minor Suggestions:** - Align the implementation and documentation of `url_check`. - Consider capturing and logging the output of `createrepo` if it fails. - Avoid creating global `tempfile.TemporaryDirectory` instances which execute on import. --- 🤖 **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 commented

2026-05-01 01:38:09 +00:00

Author

Owner

oh, also, I intend to roll this out on openQA staging so we can A/B it against other approaches.

adamwill force-pushed eln-wrapper from e7aa59ba63

CI via Tox / tox (pull_request) Failing after 1m27s

Details

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

Details

to d36b358f0d

CI via Tox / tox (pull_request) Failing after 1m34s

Details

2026-05-01 01:48:26 +00:00

Compare

adamwill force-pushed eln-wrapper from d36b358f0d

CI via Tox / tox (pull_request) Failing after 1m34s

Details

to 3d727a1be7

CI via Tox / tox (pull_request) Failing after 1m33s

Details

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

Details

2026-05-01 01:51:59 +00:00

Compare

forgejo-actions commented

2026-05-01 01:54:02 +00:00

AI Code Review

📋 MR Summary

Adds an opinionated wrapper script (rdc-el-wrapper) for running rmdepcheck on Enterprise Linux by handling multi-variant compositions.

Key Changes:
- Added rdc-el-wrapper.py to orchestrate rmdepcheck across different EL variants.
- Introduced logic to dynamically parse Pungi configuration and resolve variant relationships.
- Added local temporary repository creation based on packages under test.
Impact: rdc-el-wrapper.py, README.md
Risk Level: 🟡 Medium - The script handles external processes and temporary file creation. A couple of logic bugs exist that could cause silent failures or command-line argument parsing errors.

Detailed Code Review

The implementation correctly aligns with the stated design requirements, specifically keeping the separation of variants and ignoring the Buildroot. However, there are two important logic issues to address. First, the script silently ignores errors when invoking dnf repoquery because check=False is used, which can mask network or configuration errors and result in missing packages. Second, if there are no depended-on variants, an empty string is passed to the --addrepos argument of rmdepcheck.py, which may lead to argument parsing errors or invalid repository errors.

📂 File Reviews

📄 `rdc-el-wrapper.py` - Main wrapper script that prepares and invokes rmdepcheck across isolated variant repositories.

Major [Bug]: The subprocess call to dnf repoquery uses check=False and does not check the return code. If the network request fails or DNF errors out, stdout will be empty, and the script will silently assume there are no packages for that variant.
- Suggestion: Check the return code of the subprocess and raise an exception if it fails so that errors are not swallowed.

        res = SUBPCAPTURE(args)
        if res.returncode != 0:
            raise RuntimeError(f"dnf repoquery failed for {variant}: {res.stderr}")
        pkgs[variant] = res.stdout.splitlines()

Major [Bug]: If addarg is empty (when a variant has no dependencies), the script still passes --addrepos followed by an empty string to rmdepcheck.py. Depending on how rmdepcheck.py parses arguments, this could cause the script to treat basearg as the value for --addrepos or fail trying to parse an empty repo.
- Suggestion: Only append --addrepos and its argument to the list if addarg is not empty.

                rargs = [
                    f"{HERE}/rmdepcheck.py",
                    "--arch",
                    args.arch
                ]
                if addarg:
                    rargs.extend(["--addrepos", addarg])
                rargs.extend([basearg, newarg])
                rets.add(subprocess.run(rargs, check=False).returncode)

✅ Summary

Overall Assessment: Several logic bugs were identified related to subprocess error handling and argument construction that must be addressed before merging.
Priority Issues:
- Check the return code of the dnf repoquery subprocess to avoid silent data loss.
- Conditionally pass the --addrepos argument to rmdepcheck.py only when there are actually addrepos to include.

🤖 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 an opinionated wrapper script (`rdc-el-wrapper`) for running `rmdepcheck` on Enterprise Linux by handling multi-variant compositions. - **Key Changes:** - Added `rdc-el-wrapper.py` to orchestrate `rmdepcheck` across different EL variants. - Introduced logic to dynamically parse Pungi configuration and resolve variant relationships. - Added local temporary repository creation based on packages under test. - **Impact:** rdc-el-wrapper.py, README.md - **Risk Level:** 🟡 Medium - The script handles external processes and temporary file creation. A couple of logic bugs exist that could cause silent failures or command-line argument parsing errors. ### Detailed Code Review The implementation correctly aligns with the stated design requirements, specifically keeping the separation of variants and ignoring the Buildroot. However, there are two important logic issues to address. First, the script silently ignores errors when invoking `dnf repoquery` because `check=False` is used, which can mask network or configuration errors and result in missing packages. Second, if there are no depended-on variants, an empty string is passed to the `--addrepos` argument of `rmdepcheck.py`, which may lead to argument parsing errors or invalid repository errors. #### 📂 File Reviews <details> <summary>📄 `rdc-el-wrapper.py` - Main wrapper script that prepares and invokes rmdepcheck across isolated variant repositories.</summary> - **Major** [Bug]: The subprocess call to `dnf repoquery` uses `check=False` and does not check the return code. If the network request fails or DNF errors out, `stdout` will be empty, and the script will silently assume there are no packages for that variant. - *Suggestion:* Check the return code of the subprocess and raise an exception if it fails so that errors are not swallowed. ```` res = SUBPCAPTURE(args) if res.returncode != 0: raise RuntimeError(f"dnf repoquery failed for {variant}: {res.stderr}") pkgs[variant] = res.stdout.splitlines() ```` - **Major** [Bug]: If `addarg` is empty (when a variant has no dependencies), the script still passes `--addrepos` followed by an empty string to `rmdepcheck.py`. Depending on how `rmdepcheck.py` parses arguments, this could cause the script to treat `basearg` as the value for `--addrepos` or fail trying to parse an empty repo. - *Suggestion:* Only append `--addrepos` and its argument to the list if `addarg` is not empty. ```` rargs = [ f"{HERE}/rmdepcheck.py", "--arch", args.arch ] if addarg: rargs.extend(["--addrepos", addarg]) rargs.extend([basearg, newarg]) rets.add(subprocess.run(rargs, check=False).returncode) ```` </details> ### ✅ Summary - **Overall Assessment:** Several logic bugs were identified related to subprocess error handling and argument construction that must be addressed before merging. - **Priority Issues:** - Check the return code of the `dnf repoquery` subprocess to avoid silent data loss. - Conditionally pass the `--addrepos` argument to `rmdepcheck.py` only when there are actually addrepos to include. --- 🤖 **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 3d727a1be7

CI via Tox / tox (pull_request) Failing after 1m33s

Details

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

Details

to 3106853291

CI via Tox / tox (pull_request) Failing after 1m32s

Details

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

Details

2026-05-01 02:54:57 +00:00

Compare

forgejo-actions commented

2026-05-01 02:57:02 +00:00

AI Code Review

📋 MR Summary

Adds an opinionated wrapper script (rdc-el-wrapper) for running rmdepcheck specifically for Enterprise Linux multi-variant repository structures. It parses Pungi configurations to establish variant dependencies and dynamically creates variant-specific temporary test repositories.

Key Changes:
- Added rdc-el-wrapper.py script to automate ELN variant testing.
- Implemented repository splitting logic mapping test packages to specific compose variants.
- Integrated dynamic dependency resolution using Pungi override.conf and compose metadata.
- Updated README with usage instructions for the new wrapper.
Impact: rdc-el-wrapper.py, README.md, install.requires
Risk Level: 🟡 Medium - The script handles file system operations, subprocess calls to package managers, and relies on string-based parsing of remote configurations which can be brittle if upstream formats change.

Detailed Code Review

The script successfully implements the requested logic for identifying ELN variants, splitting packages, and executing rmdepcheck with correct base/addrepo mappings. The algorithmic approach to the Pungi lookaside relationships matches the architectural requirements. However, the parsing of the remote Pungi configuration is fragile and could crash the script if the upstream configuration formatting changes even slightly (e.g., spacing changes). Additionally, modifying a global mutable state (DNFARGS) in the main function is an anti-pattern, though benign in a single-execution script. Some error handling improvements could prevent ungraceful tracebacks on network or subprocess failures.

📂 File Reviews

📄 `rdc-el-wrapper.py` - Implements the core wrapper logic including compose metadata parsing, package splitting, and subprocess orchestration.

Major [Logic]: The parsing of variant_as_lookaside is highly fragile. Searching for a strict string match and the first ] character will break if upstream adds spaces around = or if formatting involves comments.
- Suggestion: Use a regular expression to extract the Python-like list more resiliently, accounting for variable whitespace.

import re

def get_val():
    # ... fetch override ...
    match = re.search(r'variant_as_lookaside\s*=\s*(\[.*?\])', override, re.DOTALL)
    if not match:
        raise ValueError("Could not find variant_as_lookaside in configuration")
    try:
        return ast.literal_eval(match.group(1))
    except (SyntaxError, ValueError) as e:
        raise ValueError(f"Failed to parse variant_as_lookaside: {e}")

Minor [Style]: Modifying the global DNFARGS list inside the main() function is a bad practice. It works here because main() is only called once, but it makes the code harder to test or reuse.
- Suggestion: Create a local copy of the arguments or pass the architecture flag directly where needed.

def main() -> None:
    # ...
    args = parse_args()
    local_dnf_args = list(DNFARGS)
    if f"--forcearch={args.arch}" not in local_dnf_args:
        local_dnf_args.append(f"--forcearch={args.arch}")
    # Pass local_dnf_args down to functions that need it...

Minor [Bug]: When determining the package name from the file name _file.rsplit("-", 2)[0], if a non-RPM or malformed file is in the directory with fewer than two dashes, this can return unexpected results.
- Suggestion: Ensure the split produces exactly 3 parts, or use standard regex/RPM bindings to extract the package name safely.

parts = _file.rsplit("-", 2)
if len(parts) == 3:
    pkgname = parts[0]
    # continue processing...

✅ Summary

Overall Assessment: The script correctly implements the complex ELN variant mapping requirements, but structural robustness in remote config parsing should be addressed to prevent random test failures.
Priority Issues:
- Make the variant_as_lookaside parsing logic more robust using regex to avoid crashes on minor upstream configuration formatting changes.
Minor Suggestions:
- Avoid mutating global lists (DNFARGS) in main().
- Add standard exception catching for network requests and subprocess calls to output clean error messages rather than raw Python tracebacks.
- Add a safeguard for parsing package names from filenames to ensure files without expected dashes don't cause logic bugs.

🤖 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 an opinionated wrapper script (`rdc-el-wrapper`) for running `rmdepcheck` specifically for Enterprise Linux multi-variant repository structures. It parses Pungi configurations to establish variant dependencies and dynamically creates variant-specific temporary test repositories. - **Key Changes:** - Added `rdc-el-wrapper.py` script to automate ELN variant testing. - Implemented repository splitting logic mapping test packages to specific compose variants. - Integrated dynamic dependency resolution using Pungi `override.conf` and compose metadata. - Updated README with usage instructions for the new wrapper. - **Impact:** rdc-el-wrapper.py, README.md, install.requires - **Risk Level:** 🟡 Medium - The script handles file system operations, subprocess calls to package managers, and relies on string-based parsing of remote configurations which can be brittle if upstream formats change. ### Detailed Code Review The script successfully implements the requested logic for identifying ELN variants, splitting packages, and executing `rmdepcheck` with correct base/addrepo mappings. The algorithmic approach to the Pungi lookaside relationships matches the architectural requirements. However, the parsing of the remote Pungi configuration is fragile and could crash the script if the upstream configuration formatting changes even slightly (e.g., spacing changes). Additionally, modifying a global mutable state (`DNFARGS`) in the `main` function is an anti-pattern, though benign in a single-execution script. Some error handling improvements could prevent ungraceful tracebacks on network or subprocess failures. #### 📂 File Reviews <details> <summary>📄 `rdc-el-wrapper.py` - Implements the core wrapper logic including compose metadata parsing, package splitting, and subprocess orchestration.</summary> - **Major** [Logic]: The parsing of `variant_as_lookaside` is highly fragile. Searching for a strict string match and the first `]` character will break if upstream adds spaces around `=` or if formatting involves comments. - *Suggestion:* Use a regular expression to extract the Python-like list more resiliently, accounting for variable whitespace. ```` import re def get_val(): # ... fetch override ... match = re.search(r'variant_as_lookaside\s*=\s*(\[.*?\])', override, re.DOTALL) if not match: raise ValueError("Could not find variant_as_lookaside in configuration") try: return ast.literal_eval(match.group(1)) except (SyntaxError, ValueError) as e: raise ValueError(f"Failed to parse variant_as_lookaside: {e}") ```` - **Minor** [Style]: Modifying the global `DNFARGS` list inside the `main()` function is a bad practice. It works here because `main()` is only called once, but it makes the code harder to test or reuse. - *Suggestion:* Create a local copy of the arguments or pass the architecture flag directly where needed. ```` def main() -> None: # ... args = parse_args() local_dnf_args = list(DNFARGS) if f"--forcearch={args.arch}" not in local_dnf_args: local_dnf_args.append(f"--forcearch={args.arch}") # Pass local_dnf_args down to functions that need it... ```` - **Minor** [Bug]: When determining the package name from the file name `_file.rsplit("-", 2)[0]`, if a non-RPM or malformed file is in the directory with fewer than two dashes, this can return unexpected results. - *Suggestion:* Ensure the split produces exactly 3 parts, or use standard regex/RPM bindings to extract the package name safely. ```` parts = _file.rsplit("-", 2) if len(parts) == 3: pkgname = parts[0] # continue processing... ```` </details> ### ✅ Summary - **Overall Assessment:** The script correctly implements the complex ELN variant mapping requirements, but structural robustness in remote config parsing should be addressed to prevent random test failures. - **Priority Issues:** - Make the `variant_as_lookaside` parsing logic more robust using regex to avoid crashes on minor upstream configuration formatting changes. - **Minor Suggestions:** - Avoid mutating global lists (`DNFARGS`) in `main()`. - Add standard exception catching for network requests and subprocess calls to output clean error messages rather than raw Python tracebacks. - Add a safeguard for parsing package names from filenames to ensure files without expected dashes don't cause logic bugs. --- 🤖 **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 commented

2026-05-01 03:25:07 +00:00

Author

Owner

Fine, robustified the val parser. I am fine with the assumption that anything ending .rpm is an RPM and subject to rsplit logic. I am fine with mutating DNFARGS; we already protect against duplicating the arg, this prevents passing it all over the damn place. I am fine with things crashing with useful tracebacks rather than obfuscated error messages, I like data.

adamwill commented

2026-05-01 03:27:50 +00:00

Author

Owner

One thing I noticed with this is we get duplicated errors when we fail:

[adamw@toolbx fedora-toolbox-44 rmdepcheck (eln-wrapper %)]$ ./rdc-el-wrapper.py https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose /var/tmp/FEDORA-2026-a5c29045c9
Dependencies of other packages that would be BROKEN by the tested packages:
package: varnish-modules-0.28.0-1.eln156.x86_64 from https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose/AppStream/x86_64/os
  varnish = 9.0.0
Dependencies of other packages that would be BROKEN by the tested packages:
package: varnish-modules-0.28.0-1.eln156.x86_64 from https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose/AppStream/x86_64/os
  varnish = 9.0.0
Dependencies of other packages that would be BROKEN by the tested packages:
package: varnish-modules-0.28.0-1.eln156.x86_64 from https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose/AppStream/x86_64/os
  varnish = 9.0.0

We...um. We can actually deal with that by resurrecting the "non-checked repos" idea, I think. When checking CRB, have AppStream as a modified but non-checked base repo...

I'll look at that tomorrow I think.

One thing I noticed with this is we get duplicated errors when we fail: ``` [adamw@toolbx fedora-toolbox-44 rmdepcheck (eln-wrapper %)]$ ./rdc-el-wrapper.py https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose /var/tmp/FEDORA-2026-a5c29045c9 Dependencies of other packages that would be BROKEN by the tested packages: package: varnish-modules-0.28.0-1.eln156.x86_64 from https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose/AppStream/x86_64/os varnish = 9.0.0 Dependencies of other packages that would be BROKEN by the tested packages: package: varnish-modules-0.28.0-1.eln156.x86_64 from https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose/AppStream/x86_64/os varnish = 9.0.0 Dependencies of other packages that would be BROKEN by the tested packages: package: varnish-modules-0.28.0-1.eln156.x86_64 from https://kojipkgs.fedoraproject.org/compose/eln/Fedora-eln-20260429.n.0/compose/AppStream/x86_64/os varnish = 9.0.0 ``` We...um. We can actually deal with that by resurrecting the "non-checked repos" idea, I think. When checking CRB, have AppStream as a modified but non-checked base repo... I'll look at that tomorrow I think.

adamwill force-pushed eln-wrapper from 3106853291

CI via Tox / tox (pull_request) Failing after 1m32s

Details

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

Details

to 6b1d337d97

CI via Tox / tox (pull_request) Failing after 1m35s

Details

2026-05-01 03:29:20 +00:00

Compare

adamwill referenced this pull request from quality/paperwork

2026-05-01 06:47:02 +00:00

openQA failure monitoring 2026 sprint 8 #37

adamwill referenced this pull request from ci/tickets

2026-05-01 06:52:02 +00:00

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

adamwill force-pushed eln-wrapper from 6b1d337d97

CI via Tox / tox (pull_request) Failing after 1m35s

Details

to 44f1bdb0ec

CI via Tox / tox (pull_request) Has been cancelled

Details

2026-05-01 22:46:40 +00:00

Compare

adamwill commented

2026-05-02 07:24:19 +00:00

Author

Owner

Duplicated errors are fixed now, I brought back non-checked repos. But I now want to refactor this, I think...so we maybe have a shared module and two CLI scripts...

adamwill referenced this pull request

2026-05-02 07:24:47 +00:00

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

adamwill referenced this pull request from ci/tickets

2026-05-04 18:17:27 +00:00

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

adamwill referenced this pull request from quality/os-autoinst-distri-fedora

2026-05-04 20:47:46 +00:00

rmdepcheck: run ELN tests on composed repos, not buildroot #532

adamwill referenced this pull request

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

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

adamwill referenced this pull request from quality/os-autoinst-distri-fedora

2026-05-05 17:07:02 +00:00

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

adamwill added 1 commit

2026-05-09 01:23:16 +00:00

Refactor into a module with two CLI scripts and shared bits

CI via Tox / tox (pull_request) Failing after 1m38s

Details

943d87467f

Packaging two Python scripts, one of which is a wrapper for the
other, is pretty awkward. But more importantly, we can make the
EL "wrapper" work better with a refactor, too.

This makes the CLI scripts independent of each other but sharing
a lot of bits via `shared.py`. We put handy run-it-locally
wrappers for each CLI script at the top level; you *can* install
the package properly and you'll get `rmdepcheck` and `rdcelwrap`
executables but we'll likely rarely do that.

The wrapper will now run faster because it will reuse the same
metadata between variants (previously it got re-downloaded with
every execution of `rmdepcheck.py`) and it can now nicely collect
up all the repoclosure strings before using the same parse-and-
exit code as rmdepcheck, which solves a lot of awkward issues
around output.

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

adamwill force-pushed eln-wrapper from 943d87467f

CI via Tox / tox (pull_request) Failing after 1m38s

Details

to 4285d1515a

CI via Tox / tox (pull_request) Failing after 1m36s

Details

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

Details

2026-05-10 23:43:39 +00:00

Compare

forgejo-actions commented

2026-05-11 00:07:13 +00:00

AI Code Review

📋 MR Summary

Refactors rmdepcheck into a reusable module and introduces rdc-el-wrapper to support Enterprise Linux (ELN) testing. It adds the ability to use "non-checked base repos" to simulate dependencies across different compose variants.

Key Changes:
- Refactored the codebase to separate shared logic from CLI entry points (rdcelwrap.py and rmdepcheck.py).
- Introduced rdc-el-wrapper to split "packages under test" into variant-specific temporary repositories based on compose mappings.
- Added support for --ncbaserepos (non-checked base repos) to allow repositories to be modified and available to the dependency solver, but excluded from repoclosure checks.
Impact: src/rmdepcheck/rdcelwrap.py, src/rmdepcheck/rmdepcheck.py, src/rmdepcheck/shared.py
Risk Level: 🟢 Low - This merge request strictly affects testing and CI orchestration utilities. The primary risks involve script crashes on malformed or edge-case environment configurations, but does not impact application runtime security.

Detailed Code Review

The refactoring successfully modularizes the tool and accurately accommodates the Pungi/ELN variant mapping logic without introducing cyclic redundancies. The changes follow the previous discussions, explicitly respecting the accepted return code aggregation and the non-checked base repository paradigm.

While the code avoids exhaustive error trapping by design (as requested), there is one minor logic gap in the variant dependency lookups where Buildroot (or a dropped variant) might cause an unhandled KeyError. Filtering this lookup ensures standard Pungi configurations don't crash the script unexpectedly.

📂 File Reviews

📄 `src/rmdepcheck/rdcelwrap.py` - Implements the ELN testing wrapper logic for splitting and mapping variant repositories.

Major [Bug]: If a dependent variant (like Buildroot) is in depvars but was excluded from vrs (which actively ignores Buildroot), this list comprehension will raise a KeyError and crash the script.
- Suggestion: Filter the comprehension to only include variants present in vrs.

                # put the compose repos for all depended-on variants in
                # ncbaserepos, even ones we didn't 'populate' locally
                ncbaserepos = [vrs[var] for var in depvars if var in vrs]

✅ Summary

Overall Assessment: The implementation aligns well with the intended design. One major logic error regarding variant mapping requires fixing to avoid consistent crashes on normal ELN test runs.
Priority Issues:
- Fix KeyError in rdcelwrap.py caused by depending on variants (like Buildroot) that are deliberately excluded from the vrs dictionary.

🤖 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 Refactors `rmdepcheck` into a reusable module and introduces `rdc-el-wrapper` to support Enterprise Linux (ELN) testing. It adds the ability to use "non-checked base repos" to simulate dependencies across different compose variants. - **Key Changes:** - Refactored the codebase to separate shared logic from CLI entry points (`rdcelwrap.py` and `rmdepcheck.py`). - Introduced `rdc-el-wrapper` to split "packages under test" into variant-specific temporary repositories based on compose mappings. - Added support for `--ncbaserepos` (non-checked base repos) to allow repositories to be modified and available to the dependency solver, but excluded from repoclosure checks. - **Impact:** src/rmdepcheck/rdcelwrap.py, src/rmdepcheck/rmdepcheck.py, src/rmdepcheck/shared.py - **Risk Level:** 🟢 Low - This merge request strictly affects testing and CI orchestration utilities. The primary risks involve script crashes on malformed or edge-case environment configurations, but does not impact application runtime security. ### Detailed Code Review The refactoring successfully modularizes the tool and accurately accommodates the Pungi/ELN variant mapping logic without introducing cyclic redundancies. The changes follow the previous discussions, explicitly respecting the accepted return code aggregation and the non-checked base repository paradigm. While the code avoids exhaustive error trapping by design (as requested), there is one minor logic gap in the variant dependency lookups where `Buildroot` (or a dropped variant) might cause an unhandled `KeyError`. Filtering this lookup ensures standard Pungi configurations don't crash the script unexpectedly. #### 📂 File Reviews <details> <summary>📄 `src/rmdepcheck/rdcelwrap.py` - Implements the ELN testing wrapper logic for splitting and mapping variant repositories.</summary> - **Major** [Bug]: If a dependent variant (like `Buildroot`) is in `depvars` but was excluded from `vrs` (which actively ignores `Buildroot`), this list comprehension will raise a `KeyError` and crash the script. - *Suggestion:* Filter the comprehension to only include variants present in `vrs`. ```` # put the compose repos for all depended-on variants in # ncbaserepos, even ones we didn't 'populate' locally ncbaserepos = [vrs[var] for var in depvars if var in vrs] ```` </details> ### ✅ Summary - **Overall Assessment:** The implementation aligns well with the intended design. One major logic error regarding variant mapping requires fixing to avoid consistent crashes on normal ELN test runs. - **Priority Issues:** - Fix `KeyError` in `rdcelwrap.py` caused by depending on variants (like `Buildroot`) that are deliberately excluded from the `vrs` dictionary. --- 🤖 **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 4285d1515a

CI via Tox / tox (pull_request) Failing after 1m36s

Details

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

Details

to 5aade1d2fd

CI via Tox / tox (pull_request) Failing after 1m33s

Details

2026-05-13 01:43:48 +00:00

Compare

adamwill force-pushed eln-wrapper from 5aade1d2fd

CI via Tox / tox (pull_request) Failing after 1m33s

Details

to f5a53f7f09

CI via Tox / tox (pull_request) Failing after 1m47s

Details

2026-05-14 06:52:09 +00:00

Compare

adamwill commented

2026-05-14 06:54:59 +00:00

Author

Owner

Progress: added tests for rdcelwrapper, now at 90% coverage, just need to cover the last bits tomorrow. Writing the tests also caused me to discover a bug - we were only checking variants we had new packages for, but actually, we need to check variants we have new packages for and any other variants that may depend on those.

I'll add the remaining test coverage and maybe split the commit a bit tomorrow, then merge, hopefully.

Progress: added tests for rdcelwrapper, now at 90% coverage, just need to cover the last bits tomorrow. Writing the tests also caused me to discover a bug - we were only checking variants we had new packages for, but actually, we need to check variants we have new packages for *and any other variants that may depend on those*. I'll add the remaining test coverage and maybe split the commit a bit tomorrow, then merge, hopefully.

adamwill force-pushed eln-wrapper from f5a53f7f09

CI via Tox / tox (pull_request) Failing after 1m47s

Details

to deb8273277

CI via Tox / tox (pull_request) Failing after 1m51s

Details

2026-05-14 06:55:45 +00:00

Compare

adamwill force-pushed eln-wrapper from deb8273277

CI via Tox / tox (pull_request) Failing after 1m51s

Details

to 57f086fe8e

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

Details

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

Details

2026-05-15 01:43:42 +00:00

Compare

forgejo-actions commented

2026-05-15 01:46:00 +00:00

AI Code Review

📋 MR Summary

Refactors the project into a shared module with two CLI scripts, adds a new wrapper rdc-el-wrapper for ELN testing, and improves test coverage.

Key Changes:
- Extracted shared logic into src/rmdepcheck/shared.py.
- Added src/rmdepcheck/rdcelwrap.py for Enterprise Linux variant testing.
- Introduced the concept of 'non-checked base repos'.
- Added unit and end-to-end tests for the new EL wrapper.
Impact: src/rmdepcheck/rdcelwrap.py, src/rmdepcheck/rmdepcheck.py, src/rmdepcheck/shared.py, tests/
Risk Level: 🟡 Medium - The refactoring introduces a new architectural layout and modifies core CLI behaviors, which carries a risk of regressions, though it is mitigated by high test coverage.

Detailed Code Review

The refactoring successfully separates the concerns of the standard rmdepcheck and the opinionated rdc-el-wrapper by utilizing a shared module. The test coverage is a great addition, and the logic aligns with the described requirements for variant mapping.

Overall, the implementation appears robust. The usage of ast.literal_eval for configuration parsing is a secure choice, avoiding arbitrary code execution risks. The argument parser correctly delegates to shared utility functions to validate URLs.

📂 File Reviews

📄 `src/rmdepcheck/rdcelwrap.py` - Implements the opinionated wrapper for ELN.

Minor [Suggestion]: When appending paths to args.compose, double slashes might be produced if the user provides a URL with a trailing slash.
- Suggestion: Strip trailing slashes from args.compose before building URLs.

```python
# Clean URL before using
compose_url = args.compose.rstrip('/')
resp = SESSION.get(f"{compose_url}/metadata/composeinfo.json")
```

✅ Summary

Overall Assessment: The changes look solid and well-tested. No critical issues were identified, and the refactor accomplishes the described goals cleanly.
Minor Suggestions:
- Consider stripping trailing slashes from the compose URL before concatenating API paths in rdcelwrap.py to prevent malformed requests.

🤖 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 Refactors the project into a shared module with two CLI scripts, adds a new wrapper `rdc-el-wrapper` for ELN testing, and improves test coverage. - **Key Changes:** - Extracted shared logic into `src/rmdepcheck/shared.py`. - Added `src/rmdepcheck/rdcelwrap.py` for Enterprise Linux variant testing. - Introduced the concept of 'non-checked base repos'. - Added unit and end-to-end tests for the new EL wrapper. - **Impact:** src/rmdepcheck/rdcelwrap.py, src/rmdepcheck/rmdepcheck.py, src/rmdepcheck/shared.py, tests/ - **Risk Level:** 🟡 Medium - The refactoring introduces a new architectural layout and modifies core CLI behaviors, which carries a risk of regressions, though it is mitigated by high test coverage. ### Detailed Code Review The refactoring successfully separates the concerns of the standard `rmdepcheck` and the opinionated `rdc-el-wrapper` by utilizing a shared module. The test coverage is a great addition, and the logic aligns with the described requirements for variant mapping. Overall, the implementation appears robust. The usage of `ast.literal_eval` for configuration parsing is a secure choice, avoiding arbitrary code execution risks. The argument parser correctly delegates to shared utility functions to validate URLs. #### 📂 File Reviews <details> <summary>📄 `src/rmdepcheck/rdcelwrap.py` - Implements the opinionated wrapper for ELN.</summary> - **Minor** [Suggestion]: When appending paths to `args.compose`, double slashes might be produced if the user provides a URL with a trailing slash. - *Suggestion:* Strip trailing slashes from `args.compose` before building URLs. ```` ```python # Clean URL before using compose_url = args.compose.rstrip('/') resp = SESSION.get(f"{compose_url}/metadata/composeinfo.json") ``` ```` </details> ### ✅ Summary - **Overall Assessment:** The changes look solid and well-tested. No critical issues were identified, and the refactor accomplishes the described goals cleanly. - **Minor Suggestions:** - Consider stripping trailing slashes from the compose URL before concatenating API paths in `rdcelwrap.py` to prevent malformed requests. --- 🤖 **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 changed title from ~~Add rdc-el-wrapper, an opinionated wrapper for ELN testing (#37)~~ to Add EL-specific interface, refactor (#37)

2026-05-15 06:33:37 +00:00

adamwill force-pushed eln-wrapper from 57f086fe8e

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

Details

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

Details

to 4cfe070ed0

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

Details

2026-05-15 21:45:14 +00:00

Compare

adamwill referenced this pull request from a commit

2026-05-15 22:08:52 +00:00

rmdepcheck: handle rdc-repos / rdc-compose split

adamwill referenced this pull request from a commit

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

rmdepcheck: handle rdc-repos / rdc-compose split

adamwill referenced this pull request from quality/os-autoinst-distri-fedora

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

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

adamwill force-pushed eln-wrapper from 4cfe070ed0

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

Details

to 1be91cf954

CI via Tox / tox (pull_request) Failing after 1m31s

Details

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

Details

2026-05-15 22:13:44 +00:00

Compare

adamwill commented

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

Author

Owner

quality/os-autoinst-distri-fedora#552 and https://github.com/fedora-ci/rmdepcheck-pipeline/pull/23 adapt the pipelines to this change, but I've put in a compatibility rmdepcheck.py symlink for now so they'll keep working even if we merge this.

https://forge.fedoraproject.org/quality/os-autoinst-distri-fedora/pulls/552 and https://github.com/fedora-ci/rmdepcheck-pipeline/pull/23 adapt the pipelines to this change, but I've put in a compatibility `rmdepcheck.py` symlink for now so they'll keep working even if we merge this.

forgejo-actions commented

2026-05-15 22:17:49 +00:00

AI Code Review

📋 MR Summary

This PR implements a major refactor of rmdepcheck, splitting the monolithic script into a modular architecture with two distinct CLIs (rdc-repos and rdc-compose). It introduces a shared module, adds the concept of non-checked base repositories, safely parses Pungi configurations to simulate Pungi variant dependencies for ELN compose testing, and expands test coverage significantly.

Key Changes:
- Refactored the monolithic rmdepcheck.py script into src/rmdepcheck/ with shared.py, repos.py, and compose.py modules.
- Introduced rdc-compose CLI which fetches and parses ELN compose metadata (composeinfo.json) and Pungi configuration overrides.
- Added logic to split packages under test into respective isolated variant directories and correctly resolve dependencies across dependent variants.
- Eliminated global state mutation of DNFARGS by isolating runtime arguments and passing them functionally.
- Added comprehensive End-to-End (E2E) and unit tests for the newly separated logic, including mocked HTTP responses for compose data.
Impact: src/rmdepcheck/compose.py, src/rmdepcheck/repos.py, src/rmdepcheck/shared.py, tests/test_rmdepcheck.py
Risk Level: 🟢 Low - The refactor successfully modularizes existing logic while retaining core functionality, avoids unsafe execution vectors (using ast.literal_eval for parsing configurations securely), and resolves a known bug regarding downstream variant checking. Test coverage validates the behavioral preservation.

Detailed Code Review

The architectural transition from a single script to a well-structured Python package is cleanly executed. Separating the CLI boundaries (repos.py for standard repo-level checks and compose.py for Enterprise Linux variant-aware checks) alongside a shared functional core removes duplicate code and creates a clearer contract for dnf execution arguments.

The logic used to evaluate Pungi config relationships safely relies on regex matching and ast.literal_eval(), avoiding dangerous standard eval() calls. Additionally, the move away from mutating global DNFARGS in favor of passing constructed argument lists solves testing pollution and state leakage risks across concurrent invocations.

Robust error handling is maintained for HTTP sessions in rdc-compose via urllib3 Retries. No critical security, performance, or logic issues were found during this review.

✅ Summary

Overall Assessment: Excellent refactor that resolves complex orchestration requirements safely and enhances testability. No critical issues identified; ready for 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 ### 📋 MR Summary This PR implements a major refactor of `rmdepcheck`, splitting the monolithic script into a modular architecture with two distinct CLIs (`rdc-repos` and `rdc-compose`). It introduces a `shared` module, adds the concept of non-checked base repositories, safely parses Pungi configurations to simulate Pungi variant dependencies for ELN compose testing, and expands test coverage significantly. - **Key Changes:** - Refactored the monolithic `rmdepcheck.py` script into `src/rmdepcheck/` with `shared.py`, `repos.py`, and `compose.py` modules. - Introduced `rdc-compose` CLI which fetches and parses ELN compose metadata (composeinfo.json) and Pungi configuration overrides. - Added logic to split packages under test into respective isolated variant directories and correctly resolve dependencies across dependent variants. - Eliminated global state mutation of `DNFARGS` by isolating runtime arguments and passing them functionally. - Added comprehensive End-to-End (E2E) and unit tests for the newly separated logic, including mocked HTTP responses for compose data. - **Impact:** src/rmdepcheck/compose.py, src/rmdepcheck/repos.py, src/rmdepcheck/shared.py, tests/test_rmdepcheck.py - **Risk Level:** 🟢 Low - The refactor successfully modularizes existing logic while retaining core functionality, avoids unsafe execution vectors (using `ast.literal_eval` for parsing configurations securely), and resolves a known bug regarding downstream variant checking. Test coverage validates the behavioral preservation. ### Detailed Code Review The architectural transition from a single script to a well-structured Python package is cleanly executed. Separating the CLI boundaries (`repos.py` for standard repo-level checks and `compose.py` for Enterprise Linux variant-aware checks) alongside a shared functional core removes duplicate code and creates a clearer contract for `dnf` execution arguments. The logic used to evaluate Pungi config relationships safely relies on regex matching and `ast.literal_eval()`, avoiding dangerous standard `eval()` calls. Additionally, the move away from mutating global `DNFARGS` in favor of passing constructed argument lists solves testing pollution and state leakage risks across concurrent invocations. Robust error handling is maintained for HTTP sessions in `rdc-compose` via urllib3 Retries. No critical security, performance, or logic issues were found during this review. ### ✅ Summary - **Overall Assessment:** Excellent refactor that resolves complex orchestration requirements safely and enhances testability. No critical issues identified; ready for 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 1be91cf954

CI via Tox / tox (pull_request) Failing after 1m31s

Details

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

Details

to f036b42298

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

Details

2026-05-16 01:37:42 +00:00

Compare

adamwill merged commit f036b42298 into main

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

adamwill referenced this pull request from a commit

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

rmdepcheck: handle rdc-repos / rdc-compose split

Rows
Columns

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

AI Code Review

📋 MR Summary

Detailed Code Review

📂 File Reviews

✅ Summary

AI Code Review

📋 MR Summary

Detailed Code Review

📂 File Reviews

✅ Summary

AI Code Review

📋 MR Summary

Detailed Code Review

📂 File Reviews

✅ Summary

AI Code Review

📋 MR Summary

Detailed Code Review

📂 File Reviews

✅ Summary

AI Code Review

📋 MR Summary

Detailed Code Review

📂 File Reviews

✅ Summary

AI Code Review

📋 MR Summary

Detailed Code Review

✅ Summary