ART-21309: remove Dockerfile/shell parsing, leverage rpm-lockfile-prototype 0.27.0 - #3191
ART-21309: remove Dockerfile/shell parsing, leverage rpm-lockfile-prototype 0.27.0#3191fgallott wants to merge 8 commits into
Conversation
…pm-lockfile-prototype Remove shell_parser.py (~690 lines) and dockerfile_parser.py (~486 lines) which duplicated parsing now available in upstream rpm-lockfile-prototype. Import analyze_containerfile_stages, StagePackages, and shell_commands directly from the rpm_lockfile package. - Update rpm-lockfile-prototype to latest main (includes reinstall detection from PR openshift-eng#154) - Replace StageInfo/StageAnalysis with upstream StagePackages dataclass - Remove ARCH_SUBSHELL_KEYWORDS/ARCH_VAR_NAMES/ARCH_KEYWORDS from constants.py (now in upstream shell_commands) - Thread arches parameter through to upstream parsing calls - Handle upstream reinstall_targets in generator resolution - Add doozer/tests/conftest.py to register rpm_lockfile as namespace (avoids sys.exit(127) from __init__.py when dnf bindings are missing) - Update tests to use upstream RunCommandResult dataclass API Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
openshift-ci-robot
added
the
jira/valid-reference
|
@fgallott: This pull request references ART-21309 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates ChangesRPM lockfile integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant generate_lockfile
participant DockerfileParser
participant FallbackParser
participant RpmResolver
participant rpm_lockfile_prototype
generate_lockfile->>DockerfileParser: parse Containerfile stages
generate_lockfile->>FallbackParser: reconstruct unavailable generated file
generate_lockfile->>RpmResolver: resolve stage with Containerfile context
RpmResolver->>rpm_lockfile_prototype: submit packagesFromContainerfile
rpm_lockfile_prototype-->>RpmResolver: return resolved lockfile data
RpmResolver-->>generate_lockfile: return stage result
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
doozer/tests/lockfile_prototype/test_fallback.py (1)
73-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a test for malformed/unparseable Dockerfile input.
extract_generated_file_contenthas no exception handling aroundDockerfileParser(see companion comment onfallback.py); a test exercising a malformedDockerfilewould help confirm intended behavior once that's addressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/tests/lockfile_prototype/test_fallback.py` around lines 73 - 99, Add a test method to TestExtractGeneratedFileContent that writes malformed or unparseable Dockerfile content and calls extract_generated_file_content, asserting the intended safe fallback result once DockerfileParser fails. Keep the existing valid-input and missing-file tests unchanged.doozer/doozerlib/lockfile_prototype/fallback.py (3)
132-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSed substitution only supports single-character old/new patterns.
re.search(r"sed\s+'s/(.)/(.)/g'", run_body)captures exactly one character per group via(.). The docstring implies generalsed 's/.../.../g'support, but any multi-character search/replace pattern silently fails to match, leaving the raw%-delimited content unreplaced and producing an incorrect reconstructed package list.♻️ Suggested widening
- sed_match = re.search(r"sed\s+'s/(.)/(.)/g'", run_body) + sed_match = re.search(r"sed\s+'s/([^/]+)/([^/]*)/g'", run_body)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/lockfile_prototype/fallback.py` around lines 132 - 134, Update the sed-pattern handling near sed_match so it accepts arbitrary non-delimiter search and replacement strings, not just single characters, while preserving the existing global substitution behavior. Ensure multi-character patterns in sed 's/.../.../g' commands are captured and applied to resolved.
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
loggeris declared but never used for diagnostics.When a RUN entry matches the redirect regex but neither the heredoc nor echo/printf pattern matches (lines 128-140), the function silently moves on without any log output, making fallback failures hard to diagnose.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/lockfile_prototype/fallback.py` at line 16, Add diagnostic logging in the RUN-entry parsing flow around the redirect-regex match and heredoc/echo/printf handling so unmatched commands emit a useful message through the existing logger symbol. Preserve the current fallback behavior while reporting the command or relevant context when no supported pattern matches.
111-142: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo error handling around Dockerfile parsing/regex extraction.
If any of the three candidate Dockerfiles is malformed,
DockerfileParser(or the subsequent regex matching) can raise, and since this is explicitly a best-effort fallback for cases where "base images can't be pulled," an unhandled exception here would propagate and break the broader lockfile generation flow rather than degrade gracefully.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/lockfile_prototype/fallback.py` around lines 111 - 142, Wrap the Dockerfile parsing and extraction logic in the fallback loop around DockerfileParser, structure access, and regex-based processing with best-effort exception handling so malformed candidate files do not propagate errors. On any parsing or extraction failure, skip that candidate and continue checking the remaining Dockerfile names, ultimately preserving the existing empty-string fallback when none can be processed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doozer/doozerlib/lockfile_prototype/fallback.py`:
- Around line 111-122: Update the Dockerfile parsing loop around
DockerfileParser to open each df_path with a context manager, keeping the parser
construction and entry processing inside the with block so the file handle
closes promptly after use.
---
Nitpick comments:
In `@doozer/doozerlib/lockfile_prototype/fallback.py`:
- Around line 132-134: Update the sed-pattern handling near sed_match so it
accepts arbitrary non-delimiter search and replacement strings, not just single
characters, while preserving the existing global substitution behavior. Ensure
multi-character patterns in sed 's/.../.../g' commands are captured and applied
to resolved.
- Line 16: Add diagnostic logging in the RUN-entry parsing flow around the
redirect-regex match and heredoc/echo/printf handling so unmatched commands emit
a useful message through the existing logger symbol. Preserve the current
fallback behavior while reporting the command or relevant context when no
supported pattern matches.
- Around line 111-142: Wrap the Dockerfile parsing and extraction logic in the
fallback loop around DockerfileParser, structure access, and regex-based
processing with best-effort exception handling so malformed candidate files do
not propagate errors. On any parsing or extraction failure, skip that candidate
and continue checking the remaining Dockerfile names, ultimately preserving the
existing empty-string fallback when none can be processed.
In `@doozer/tests/lockfile_prototype/test_fallback.py`:
- Around line 73-99: Add a test method to TestExtractGeneratedFileContent that
writes malformed or unparseable Dockerfile content and calls
extract_generated_file_content, asserting the intended safe fallback result once
DockerfileParser fails. Keep the existing valid-input and missing-file tests
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 97b93594-2243-4c87-96a3-c43ea3193f26
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
art-cluster/pipelines/config/argocd/project/art-cd/common/image/rpm_lockfile_build.yamlart-cluster/pipelines/data/project/art-cd/image/Containerfile.rpm_lockfile_prototypedoozer/doozerlib/lockfile_prototype/constants.pydoozer/doozerlib/lockfile_prototype/dockerfile_parser.pydoozer/doozerlib/lockfile_prototype/fallback.pydoozer/doozerlib/lockfile_prototype/generator.pydoozer/doozerlib/lockfile_prototype/models.pydoozer/doozerlib/lockfile_prototype/resolver.pydoozer/doozerlib/lockfile_prototype/shell_parser.pydoozer/tests/lockfile_prototype/test_dockerfile_parser.pydoozer/tests/lockfile_prototype/test_fallback.pydoozer/tests/lockfile_prototype/test_generator.pydoozer/tests/lockfile_prototype/test_resolver.pydoozer/tests/lockfile_prototype/test_shell_parser.pypyproject.toml
💤 Files with no reviewable changes (5)
- doozer/doozerlib/lockfile_prototype/constants.py
- doozer/tests/lockfile_prototype/test_shell_parser.py
- doozer/doozerlib/lockfile_prototype/shell_parser.py
- doozer/tests/lockfile_prototype/test_dockerfile_parser.py
- doozer/doozerlib/lockfile_prototype/dockerfile_parser.py
…ckage extraction Align with upstream rpm-lockfile-prototype change that decouples Containerfile package extraction from rpmdb context. The resolver now sets packagesFromContainerfile in rpms.in.yaml instead of context.containerfile, matching the explicit opt-in API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
/hold for testing |
openshift-ci
Bot
added
the
do-not-merge/hold
When packages extracted from the Containerfile by rpm-lockfile-prototype are unavailable in configured repos (e.g. CentOS-only packages in OKD conditional blocks resolved against RHEL repos), the retry loop could not strip them because the upstream tool re-discovers them on every invocation. Add a lenient entry point that patches dnf.Base to set strict=False, making DNF skip unavailable packages with a warning instead of failing. On the first failure with packagesFromContainerfile, the retry switches to lenient mode — same extraction, same resolution, but unavailable packages are skipped. All valid packages are still resolved and pinned. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doozer/doozerlib/lockfile_prototype/generator.py (1)
186-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve actual Dockerfile stage indices for
$(cat ...)packages.Line 187 consumes
cat_packagesas real stage numbers, but_resolve_cat_packages()buildsstage_runsby appending only non-empty stages. A Dockerfile with an empty first stage shifts later$(cat ...)extras into an earlier stage, reading the wrong parent and resolving the packages in the wrong lockfile. Append an entry for everyFROMstage, including empty ones, and add a regression test.Proposed fix
if entry["instruction"] == "FROM": if seen_from: - if current_runs: - stage_runs.append(current_runs) + stage_runs.append(current_runs) seen_from = True current_runs = [] elif entry["instruction"] == "RUN" and seen_from: current_runs.append(entry["value"]) - if current_runs: + if seen_from: stage_runs.append(current_runs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/lockfile_prototype/generator.py` around lines 186 - 187, Update _resolve_cat_packages to preserve Dockerfile stage indices by appending a stage_runs entry for every FROM stage, including stages with no packages, so cat_packages continues to reference the correct downstream_parents stage. Add a regression test covering an empty initial stage followed by a stage using $(cat ...), and verify packages resolve from the corresponding lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@doozer/doozerlib/lockfile_prototype/generator.py`:
- Around line 186-187: Update _resolve_cat_packages to preserve Dockerfile stage
indices by appending a stage_runs entry for every FROM stage, including stages
with no packages, so cat_packages continues to reference the correct
downstream_parents stage. Add a regression test covering an empty initial stage
followed by a stage using $(cat ...), and verify packages resolve from the
corresponding lockfile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c4553018-25a5-4f2c-a3d9-bdddf88e5fa7
📒 Files selected for processing (5)
doozer/doozerlib/lockfile_prototype/constants.pydoozer/doozerlib/lockfile_prototype/generator.pydoozer/doozerlib/lockfile_prototype/resolver.pydoozer/tests/lockfile_prototype/test_generator.pydoozer/tests/lockfile_prototype/test_resolver.py
_resolve_cat_packages() only appended stages with RUN commands to stage_runs, so an empty first stage (e.g. COPY-only builder) shifted all later indices. $(cat ...) packages would resolve against the wrong parent image. Always append an entry for every FROM stage, even empty ones. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
packagesFromContainerfile does not extract packages from bare dnf/yum/microdnf update commands (no named packages). Without upgrade targets in the lockfile, the bare update remains in the Dockerfile but cachi2 repos have an inconsistent package set, causing EVR conflicts at build time (e.g. ose-frr glibc mismatch). Detect stages with bare update commands, query the base image for installed packages, and pass them as upgradePackages. On failure, drop all upgrade packages and set upgrades_dropped=True so the rebaser strips the bare update from the Dockerfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
…uce log noise Track actual removals in the retry loop instead of counting all fully_missing entries—containerfile-extracted packages absent from our lists caused phantom retries that exhausted the 5-retry limit. Write the lenient entry point script to a temp file instead of passing via -c to avoid multi-line Python appearing in Jenkins logs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
…st conf.strict dnf.Base.install_specs() has its own strict=True default parameter that ignores conf.strict, so patching __init__ alone had no effect. Now also patches install_specs to force strict=False, which actually makes DNF skip unavailable packages instead of raising MarkingErrors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
@fgallott: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…g upgrades reinstallPackages pins the installed EVR which wins over upgradePackages for the same package, silently preventing upgrades. This caused lockfile resolution to succeed without actually upgrading packages, leaving upgrades_dropped=False and the bare dnf update in the Dockerfile. At build time, dnf update hit EVR conflicts (e.g. glibc vs glibc-minimal-langpack) because only partial updates were available. Also mark upgrades_dropped when a stage has a bare update but no pullspec (stage alias), so the bare update gets stripped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
2 participants
test build ironic
test fallback case (base + children)
test cat /more-pkgs
Summary by CodeRabbit
New Features
Bug Fixes
FROMstages and strengthened retry behavior when packages are missing.Tests