ART-14695: Add optional verify-art-manifests presubmit (LSO pilot)#79879
ART-14695: Add optional verify-art-manifests presubmit (LSO pilot)#79879fbladilo wants to merge 4 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fbladilo: This pull request references ART-14695 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds an ocp-art validation step to release pipelines and implements a validator (Bash wrapper with embedded Python and a standalone Python CLI), step-registry wiring/metadata/OWNERS, a sync script to generate the wrapper, and unit tests covering rules R1–R3. ChangesART Manifest Validation Step
🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (1)
ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.sh (1)
25-26: ⚡ Quick winEmbedded validator duplicates the tested standalone script; guard against drift.
The Python block here (lines 25–525) is a verbatim copy of
hack/art-manifests-validate/validate_art_manifests.py, but the unit tests only exercise the standalone file. The copy that actually runs in CI is untested, so the two can silently diverge. Since step-registry naming requires embedding the script, consider adding a small test/CI check that asserts the embeddedPYVALIDATORblock matches the standalone source.🤖 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 `@ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.sh` around lines 25 - 26, The embedded PYVALIDATOR Python block in ocp-art-validate-art-manifests-commands.sh duplicates the standalone script validate_art_manifests.py and can drift; add a CI/unit test that reads the PYVALIDATOR heredoc from the shell step (identify by the marker PYVALIDATOR in ocp-art-validate-art-manifests-commands.sh) and compares it byte-for-byte (or normalized whitespace) to the contents of hack/art-manifests-validate/validate_art_manifests.py (identify by that filename) failing the job on any mismatch so the embedded block and the standalone script cannot diverge.
🤖 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 `@hack/art-manifests-validate/validate_art_manifests.py`:
- Around line 142-148: load_image_references currently calls yaml.safe_load and
assumes the result is a mapping, which causes an AttributeError for empty or
non-mapping YAML; update load_image_references to validate that the loaded data
is a dict (e.g., isinstance(data, dict)) before using data.get, and if not,
raise a ValueError with a clear message referencing the Path (path) so
validate_repo can catch it; keep the subsequent logic for extracting "spec" and
"tags" unchanged.
- Around line 282-284: The validator assumes art_yaml_data is a mapping and
calls art_yaml_data.get("updates", []), which will raise AttributeError if
art_yaml_data is None or a non-mapping; update the code in
validate_art_manifests.py to explicitly guard the parsed value (art_yaml_data)
with an isinstance check (e.g., collections.abc.Mapping or dict) and handle
non-mapping results by returning early or raising a controlled error so .get is
only called on mappings; apply the same defensive check to the equivalent logic
in ocp-art-validate-art-manifests-commands.sh to avoid uncaught AttributeError
crashes.
---
Nitpick comments:
In
`@ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.sh`:
- Around line 25-26: The embedded PYVALIDATOR Python block in
ocp-art-validate-art-manifests-commands.sh duplicates the standalone script
validate_art_manifests.py and can drift; add a CI/unit test that reads the
PYVALIDATOR heredoc from the shell step (identify by the marker PYVALIDATOR in
ocp-art-validate-art-manifests-commands.sh) and compares it byte-for-byte (or
normalized whitespace) to the contents of
hack/art-manifests-validate/validate_art_manifests.py (identify by that
filename) failing the job on any mismatch so the embedded block and the
standalone script cannot diverge.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7a73fa9e-c2d2-49c2-a76f-48ddef601179
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (8)
ci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23.yamlci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0.yamlci-operator/step-registry/ocp-art/validate/art-manifests/OWNERSci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.shci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.metadata.jsonci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.yamlhack/art-manifests-validate/validate_art_manifests.pyhack/art-manifests-validate/validate_art_manifests_test.py
| def load_image_references(path: Path) -> list[dict]: | ||
| with path.open(encoding="utf-8") as handle: | ||
| data = yaml.safe_load(handle) | ||
| tags = data.get("spec", {}).get("tags", []) | ||
| if not isinstance(tags, list) or not tags: | ||
| raise ValueError(f"Data in {path} is not a valid image-references file") | ||
| return tags |
There was a problem hiding this comment.
Guard against non-mapping YAML documents.
yaml.safe_load returns None for an empty file (or a scalar for a non-mapping document), so data.get(...) raises AttributeError, which validate_repo does not catch (only ValueError is handled). An empty/malformed image-references file would crash with a traceback instead of producing a clean R1 violation.
🛡️ Proposed guard
def load_image_references(path: Path) -> list[dict]:
with path.open(encoding="utf-8") as handle:
data = yaml.safe_load(handle)
- tags = data.get("spec", {}).get("tags", [])
+ if not isinstance(data, dict):
+ raise ValueError(f"Data in {path} is not a valid image-references file")
+ tags = data.get("spec", {}).get("tags", [])
if not isinstance(tags, list) or not tags:
raise ValueError(f"Data in {path} is not a valid image-references file")
return tags🤖 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 `@hack/art-manifests-validate/validate_art_manifests.py` around lines 142 -
148, load_image_references currently calls yaml.safe_load and assumes the result
is a mapping, which causes an AttributeError for empty or non-mapping YAML;
update load_image_references to validate that the loaded data is a dict (e.g.,
isinstance(data, dict)) before using data.get, and if not, raise a ValueError
with a clear message referencing the Path (path) so validate_repo can catch it;
keep the subsequent logic for extracting "spec" and "tags" unchanged.
| updates = art_yaml_data.get("updates", []) | ||
| if not updates: | ||
| return |
There was a problem hiding this comment.
Same non-mapping risk for art.yaml.
If the expanded art.yaml parses to None or a non-mapping value, art_yaml_data.get("updates", []) raises AttributeError, which is not in the caught (yaml.YAMLError, ValueError) set and will crash the validator. Note this also applies to the embedded copy in ocp-art-validate-art-manifests-commands.sh.
🛡️ Proposed guard
- updates = art_yaml_data.get("updates", [])
+ if not isinstance(art_yaml_data, dict):
+ violations.append(
+ Violation(
+ rule="R3",
+ message="art.yaml did not parse to a mapping after template expansion",
+ art_yaml_path=art_yaml_path,
+ )
+ )
+ return
+ updates = art_yaml_data.get("updates", [])🤖 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 `@hack/art-manifests-validate/validate_art_manifests.py` around lines 282 -
284, The validator assumes art_yaml_data is a mapping and calls
art_yaml_data.get("updates", []), which will raise AttributeError if
art_yaml_data is None or a non-mapping; update the code in
validate_art_manifests.py to explicitly guard the parsed value (art_yaml_data)
with an isinstance check (e.g., collections.abc.Mapping or dict) and handle
non-mapping results by returning early or raising a controlled error so .get is
only called on mappings; apply the same defensive check to the equivalent logic
in ocp-art-validate-art-manifests-commands.sh to avoid uncaught AttributeError
crashes.
|
/pj-rehearse pull-ci-openshift-local-storage-operator-release-5.0-verify-art-manifests |
|
@fbladilo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@fbladilo, |
fbladilo
force-pushed
the
art-14695-verify-art-manifests
branch
from
02f9fa0 to
d073a37
Compare
|
/pj-rehearse pull-ci-openshift-local-storage-operator-release-5.0-verify-art-manifests |
|
@fbladilo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fbladilo 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/art-manifests-validate/validate_art_manifests_test.py (1)
3-13: 💤 Low valueRemove duplicated import block.
tempfile,textwrap,unittest, andpathlib.Pathare imported twice. Consolidate into a single block (keepingimportlib.utilandsys).♻️ Proposed cleanup
-import tempfile -import textwrap -import unittest -from pathlib import Path - -import importlib.util -import sys -import tempfile -import textwrap -import unittest -from pathlib import Path +import importlib.util +import sys +import tempfile +import textwrap +import unittest +from pathlib import Path🤖 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 `@hack/art-manifests-validate/validate_art_manifests_test.py` around lines 3 - 13, The file validate_art_manifests_test.py contains duplicated import statements; remove the redundant second block and consolidate into a single import section that imports tempfile, textwrap, unittest, and Path once while preserving importlib.util and sys. Edit the top-level imports so only one import of tempfile, textwrap, unittest, and from pathlib import Path remains alongside importlib.util and sys to eliminate duplication.
🤖 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.
Nitpick comments:
In `@hack/art-manifests-validate/validate_art_manifests_test.py`:
- Around line 3-13: The file validate_art_manifests_test.py contains duplicated
import statements; remove the redundant second block and consolidate into a
single import section that imports tempfile, textwrap, unittest, and Path once
while preserving importlib.util and sys. Edit the top-level imports so only one
import of tempfile, textwrap, unittest, and from pathlib import Path remains
alongside importlib.util and sys to eliminate duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 99b89866-9725-4b6c-a305-21d91121c5db
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (8)
ci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23.yamlci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0.yamlci-operator/step-registry/ocp-art/validate/art-manifests/OWNERSci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.shci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.metadata.jsonci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.yamlhack/art-manifests-validate/validate_art_manifests.pyhack/art-manifests-validate/validate_art_manifests_test.py
✅ Files skipped from review due to trivial changes (2)
- ci-operator/step-registry/ocp-art/validate/art-manifests/OWNERS
- ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.metadata.json
🚧 Files skipped from review as they are similar to previous changes (5)
- ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.yaml
- ci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23.yaml
- ci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0.yaml
- hack/art-manifests-validate/validate_art_manifests.py
- ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.sh
…-14695) Introduce ocp-art-validate-art-manifests step-registry to catch image-references and art.yaml misconfigurations before rebase, and pilot it on local-storage-operator release-4.23 and release-5.0 with optional: true. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Use microdnf/dnf to install python3-pyyaml when pip is unavailable in operator build-root containers. Add sync-commands.sh to regenerate the embedded validator from hack/art-manifests-validate/validate_art_manifests.py. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
fbladilo
force-pushed
the
art-14695-verify-art-manifests
branch
from
d073a37 to
4021e4d
Compare
|
/pj-rehearse pull-ci-openshift-local-storage-operator-release-5.0-verify-art-manifests |
|
@fbladilo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hack/art-manifests-validate/validate_art_manifests_test.py (1)
225-286: ⚡ Quick winAdd malformed-YAML regression cases.
The suite covers the expected rule paths, but it doesn't lock in behavior for empty/non-mapping
image-references, non-mappingart.yaml, or malformedupdate_listentries. A couple of negative fixtures here would protect the validator from regressing back to uncaughtAttributeErrors.🤖 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 `@hack/art-manifests-validate/validate_art_manifests_test.py` around lines 225 - 286, Add unit tests to ValidateArtManifestsTest that exercise malformed-YAML regression cases so the validator won't raise AttributeError for unexpected shapes: add tests similar to test_skip_when_no_image_references that point repo fixtures like "fail-malformed-image-references" (empty or non-mapping image-references), "fail-nonmapping-artyaml" (art.yaml not a mapping), and "fail-malformed-update-list" (update_list entries that are not mappings) and call validate_repo(repo, "release-4.23") (or relevant branch) asserting either specific rule violations or an empty list as appropriate; use find_image_references_files where relevant and rely on existing helper build_fixture_tree to add these fixtures so the new tests reference the same symbols (ValidateArtManifestsTest, validate_repo, find_image_references_files).
🤖 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 `@hack/art-manifests-validate/sync-commands.sh`:
- Around line 12-15: The current trim fails to remove the existing if __name__
== "__main__": block because the parameter expansion is misquoted; change to
first assign the marker (e.g. marker='if __name__ == "__main__":') and then
remove everything from that marker to the end using
py_content="${py_content%%"$marker"*}" (this correctly strips the marker and
following content), then keep the existing whitespace-trim line
py_content="${py_content%"${py_content##*[![:space:]]}"}" so the generated
wrapper will not contain a duplicate __main__ entrypoint.
In `@hack/art-manifests-validate/validate_art_manifests.py`:
- Around line 105-131: The function find_csv_for_image_refs currently returns
unique[0] when multiple candidate CSVs remain, which silently picks arbitrarily;
instead, change this fallback to raise an explicit error (e.g., ValueError or
RuntimeError) that includes a clear message and the list of ambiguous candidate
paths so callers see the ambiguity; modify the end of find_csv_for_image_refs to
raise that exception (referencing the function name find_csv_for_image_refs and
the variable unique) rather than returning unique[0], so ambiguity fails fast
and surfaces the candidates for debugging.
- Around line 296-330: The validator currently constructs target_path =
manifests_base / relative_file and can be tricked into escaping the manifests
tree via "../" or symlinks; update the logic in validate_art_manifests.py (the
block handling relative_file/update_list/target_path) to canonicalize and
constrain targets: reject absolute paths, resolve both manifests_base.resolve()
and candidate = (manifests_base / relative_file).resolve(strict=False) and
verify candidate is within manifests_base (use a path containment check like
comparing commonpath or prefix), and also ensure the resolved candidate is a
regular file inside the manifests tree (and not a symlink that points outside)
before opening; if the containment or file checks fail, append the R3 Violation
and continue.
---
Nitpick comments:
In `@hack/art-manifests-validate/validate_art_manifests_test.py`:
- Around line 225-286: Add unit tests to ValidateArtManifestsTest that exercise
malformed-YAML regression cases so the validator won't raise AttributeError for
unexpected shapes: add tests similar to test_skip_when_no_image_references that
point repo fixtures like "fail-malformed-image-references" (empty or non-mapping
image-references), "fail-nonmapping-artyaml" (art.yaml not a mapping), and
"fail-malformed-update-list" (update_list entries that are not mappings) and
call validate_repo(repo, "release-4.23") (or relevant branch) asserting either
specific rule violations or an empty list as appropriate; use
find_image_references_files where relevant and rely on existing helper
build_fixture_tree to add these fixtures so the new tests reference the same
symbols (ValidateArtManifestsTest, validate_repo, find_image_references_files).
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: adf57b14-a55a-4406-b5c7-5296e8015984
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (9)
ci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23.yamlci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0.yamlci-operator/step-registry/ocp-art/validate/art-manifests/OWNERSci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.shci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.metadata.jsonci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.yamlhack/art-manifests-validate/sync-commands.shhack/art-manifests-validate/validate_art_manifests.pyhack/art-manifests-validate/validate_art_manifests_test.py
✅ Files skipped from review due to trivial changes (2)
- ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.metadata.json
- ci-operator/step-registry/ocp-art/validate/art-manifests/OWNERS
🚧 Files skipped from review as they are similar to previous changes (4)
- ci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-5.0.yaml
- ci-operator/config/openshift/local-storage-operator/openshift-local-storage-operator-release-4.23.yaml
- ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-ref.yaml
- ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.sh
| def find_csv_for_image_refs(image_refs_path: Path) -> Optional[Path]: | ||
| csv_files: list[Path] = [] | ||
| for directory in candidate_csv_dirs(image_refs_path): | ||
| csv_files.extend(sorted(directory.glob("*.clusterserviceversion.yaml"))) | ||
| unique = [] | ||
| seen = set() | ||
| for path in csv_files: | ||
| if path not in seen: | ||
| unique.append(path) | ||
| seen.add(path) | ||
| if not unique: | ||
| return None | ||
| if len(unique) == 1: | ||
| return unique[0] | ||
| refs_dir = image_refs_path.parent | ||
| same_dir = [path for path in unique if path.parent == refs_dir] | ||
| if len(same_dir) == 1: | ||
| return same_dir[0] | ||
| stable_dir = refs_dir / "stable" | ||
| stable_matches = [path for path in unique if path.parent == stable_dir] | ||
| if len(stable_matches) == 1: | ||
| return stable_matches[0] | ||
| manifests_dir = refs_dir / "manifests" | ||
| manifests_matches = [path for path in unique if path.parent == manifests_dir] | ||
| if len(manifests_matches) == 1: | ||
| return manifests_matches[0] | ||
| return unique[0] |
There was a problem hiding this comment.
Fail closed when CSV discovery stays ambiguous.
Line 131 silently picks the first sorted CSV when multiple candidates remain after the directory heuristics. That can validate against the wrong CSV and either miss a real R1 failure or report a false one. Return an explicit ambiguity failure instead of choosing arbitrarily.
🤖 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 `@hack/art-manifests-validate/validate_art_manifests.py` around lines 105 -
131, The function find_csv_for_image_refs currently returns unique[0] when
multiple candidate CSVs remain, which silently picks arbitrarily; instead,
change this fallback to raise an explicit error (e.g., ValueError or
RuntimeError) that includes a clear message and the list of ambiguous candidate
paths so callers see the ambiguity; modify the end of find_csv_for_image_refs to
raise that exception (referencing the function name find_csv_for_image_refs and
the variable unique) rather than returning unique[0], so ambiguity fails fast
and surfaces the candidates for debugging.
| relative_file = update.get("file") | ||
| update_list = update.get("update_list", []) | ||
| if not relative_file: | ||
| violations.append( | ||
| Violation( | ||
| rule="R3", | ||
| message="art.yaml update is missing `file`", | ||
| art_yaml_path=art_yaml_path, | ||
| ) | ||
| ) | ||
| continue | ||
| if not update_list: | ||
| violations.append( | ||
| Violation( | ||
| rule="R3", | ||
| message=f"art.yaml update_list is empty for file {relative_file!r}", | ||
| art_yaml_path=art_yaml_path, | ||
| target_file=manifests_base / relative_file, | ||
| ) | ||
| ) | ||
| continue | ||
|
|
||
| target_path = manifests_base / relative_file | ||
| if not target_path.is_file(): | ||
| violations.append( | ||
| Violation( | ||
| rule="R3", | ||
| message="art.yaml target file does not exist", | ||
| art_yaml_path=art_yaml_path, | ||
| target_file=target_path, | ||
| ) | ||
| ) | ||
| continue | ||
|
|
||
| with target_path.open(encoding="utf-8") as handle: |
There was a problem hiding this comment.
Constrain art.yaml update targets to the manifests tree.
file comes from repo content, but manifests_base / relative_file accepts ../ traversal and symlinks. A crafted PR can make this validator probe arbitrary files on the CI worker instead of only manifest files.
🔒 Proposed fix
- target_path = manifests_base / relative_file
+ base_dir = manifests_base.resolve()
+ target_path = (manifests_base / relative_file).resolve()
+ if target_path != base_dir and base_dir not in target_path.parents:
+ violations.append(
+ Violation(
+ rule="R3",
+ message="art.yaml target file must stay within the manifests directory",
+ art_yaml_path=art_yaml_path,
+ target_file=target_path,
+ )
+ )
+ continue
if not target_path.is_file():
violations.append(
Violation(As per coding guidelines, "Path traversal: canonicalize paths, reject ../".
🤖 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 `@hack/art-manifests-validate/validate_art_manifests.py` around lines 296 -
330, The validator currently constructs target_path = manifests_base /
relative_file and can be tricked into escaping the manifests tree via "../" or
symlinks; update the logic in validate_art_manifests.py (the block handling
relative_file/update_list/target_path) to canonicalize and constrain targets:
reject absolute paths, resolve both manifests_base.resolve() and candidate =
(manifests_base / relative_file).resolve(strict=False) and verify candidate is
within manifests_base (use a path containment check like comparing commonpath or
prefix), and also ensure the resolved candidate is a regular file inside the
manifests tree (and not a symlink that points outside) before opening; if the
containment or file checks fail, append the R3 Violation and continue.
RHEL9 operator build-root containers have python3 but no pip and cannot run dnf in the test pod. Match the openshift/release pattern from single-node/conf/aws: ensurepip --user then pip install --user pyyaml. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
/pj-rehearse pull-ci-openshift-local-storage-operator-release-5.0-verify-art-manifests |
|
@fbladilo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/art-manifests-validate/sync-commands.sh (1)
27-31: ⚡ Quick winInstall via
python3 -m pipto match the capability check.The presence check uses
python3 -m pip --version, but the install falls back to thepip3executable. When the pip module exists (soensurepipis skipped) but nopip3wrapper is onPATH, line 31 fails undererrexit. Using the module form keeps the interpreter/site consistent and avoids depending on apip3script.♻️ Proposed change (heredoc body)
- pip3 install --user --disable-pip-version-check --no-cache-dir 'pyyaml==6.0' + python3 -m pip install --user --disable-pip-version-check --no-cache-dir 'pyyaml==6.0'🤖 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 `@hack/art-manifests-validate/sync-commands.sh` around lines 27 - 31, The check uses the pip module via "python3 -m pip", but the install falls back to the "pip3" executable which can be missing; change the install to use the module form instead of the pip3 wrapper so the same interpreter/site is used. Specifically, replace the "pip3 install --user --disable-pip-version-check --no-cache-dir 'pyyaml==6.0'" invocation with "python3 -m pip install --user --disable-pip-version-check --no-cache-dir 'pyyaml==6.0'" (keep the existing ensurepip and PATH export logic intact).
🤖 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.
Nitpick comments:
In `@hack/art-manifests-validate/sync-commands.sh`:
- Around line 27-31: The check uses the pip module via "python3 -m pip", but the
install falls back to the "pip3" executable which can be missing; change the
install to use the module form instead of the pip3 wrapper so the same
interpreter/site is used. Specifically, replace the "pip3 install --user
--disable-pip-version-check --no-cache-dir 'pyyaml==6.0'" invocation with
"python3 -m pip install --user --disable-pip-version-check --no-cache-dir
'pyyaml==6.0'" (keep the existing ensurepip and PATH export logic intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: df8ddac5-f980-4d6e-86b1-b4ccf89644ce
📒 Files selected for processing (2)
ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.shhack/art-manifests-validate/sync-commands.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/ocp-art/validate/art-manifests/ocp-art-validate-art-manifests-commands.sh
Delegate branch resolution to resolve_release_branch() so pj-rehearse and real presubmits use PULL_BASE_REF or extra_refs instead of openshift/release's main. RELEASE_BRANCH overrides conflicting sources. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
/pj-rehearse pull-ci-openshift-local-storage-operator-release-5.0-verify-art-manifests |
|
@fbladilo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@fbladilo: 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. |
2 participants
Summary
ocp-art-validate-art-manifestsstep-registry step that validates operatorimage-referencesandart.yamlagainst the CSV before merge (R1 pullspec match, R2 branch-awareregistry.redhat.iorules, R3 art.yaml search match).local-storage-operatorforrelease-4.23andrelease-5.0withoptional: true(phase 1 soak per ART-14695).hack/art-manifests-validate/; the step embeds the script inocp-art-validate-art-manifests-commands.sh(step-registry only allows standard filenames).Expected pilot behavior
On current
release-5.0/release-4.23LSO trees, the job should fail on the stale mustgather pullspec:registry.redhat.io/openshift4/ose-local-storage-mustgather-rhel9:v4.22.0This demonstrates R2 (wrong namespace on
release-5.0, z-stream tag on both branches). The job is optional so merges are not blocked until ART-14696 phase 2.Test plan
python3 hack/art-manifests-validate/validate_art_manifests_test.py -v(8 unit tests)make registry-metadataandmake jobs/pj-rehearse pull-ci-openshift-local-storage-operator-release-5.0-verify-art-manifests/pj-rehearse pull-ci-openshift-local-storage-operator-release-4.23-verify-art-manifestsLinks
Summary by CodeRabbit
This PR introduces an optional presubmit that validates operator ART manifests and pilots it in the local-storage-operator repo for release-5.0 and release-4.23 (ART-14695). The change adds a step-registry test, the validator implementation and tests, and wires the new optional presubmit into the LSO ci-operator configs so failures are reported during a soak without blocking merges.
What changed (practical impact)
Validator behavior (rules enforced)
Implementation notes / differences to earlier description
Pilot expectations and testing
Other small changes