Skip to content

feat(ISV-7180): Fail static check when bundle attempts to replace itself #947

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JakubDurkac merged 2 commits into from
May 7, 2026
Merged

feat(ISV-7180): Fail static check when bundle attempts to replace itself #947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32fbc11
feat(ISV-7180): Fail static check when bundle attempts to replace itself
JakubDurkac May 7, 2026
a307807
fix: Update gitpython to 3.1.50 for pip-audit
JakubDurkac May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions operatorcert/static_tests/common/bundle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,13 @@ def check_replaces_availability(bundle: Bundle) -> Iterator[CheckResult]:
delimiter = ".v" if ".v" in replaces else "."
replaces_version = replaces.split(delimiter, 1)[1]
Comment on lines 248 to 249
Copy link
Copy Markdown

@qodo-code-review qodo-code-review Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Replaces split can indexerror 🐞 Bug ☼ Reliability

check_replaces_availability blindly does replaces.split(...)[1], so a malformed spec.replaces
that lacks the expected delimiter will raise IndexError. The check runner converts uncaught
exceptions into a Fail whose message is the full traceback, leaking noisy internals instead of a
clear validation error.
Agent Prompt 
### Issue description
`check_replaces_availability()` assumes `spec.replaces` always contains the expected delimiter and unconditionally does `split(...)[1]`. If the value is malformed, this raises `IndexError` and the framework surfaces a full traceback as the failure message.

### Issue Context
Static checks should emit actionable `Fail(...)` messages, not Python tracebacks.

### Fix Focus Areas
- operatorcert/static_tests/common/bundle.py[245-257]
- operatorcert/operator_repo/checks/__init__.py[157-175]

### Suggested fix
- Ensure `replaces` is a non-empty string.
- Parse it safely (e.g., via `partition`/`rpartition`) and validate you actually got a version component.
- On invalid format, `yield Fail("Invalid spec.replaces value ... expected <name>.v<version>")` and `return` (instead of throwing).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


if replaces_version == bundle.csv_operator_version:
yield Fail(
f"{bundle} attempts to replace itself. Please set "
"'spec.replaces' field to a different version in the CSV."
)
return

ver_to_dir = {
x.csv_operator_version: x.operator_version
for x in bundle.operator.all_bundles()
Comment on lines 248 to 260
Copy link
Copy Markdown

@qodo-code-review qodo-code-review Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Replaces name not validated 🐞 Bug ≡ Correctness

check_replaces_availability discards the operator name from spec.replaces and resolves the
replaced bundle using only the extracted version, which can select the wrong bundle if the CSV
references a different operator name. This can produce misleading failures (including the new
“replace itself” message) or miss the real configuration error (wrong replaces target name).
Agent Prompt 
### Issue description
`check_replaces_availability()` ignores the operator name portion of `spec.replaces` and matches the replaced bundle only by version. If `spec.replaces` mistakenly points to a different operator CSV name, this check can validate against the wrong bundle (or emit a misleading error).

### Issue Context
`spec.replaces` is a CSV *name*; both name and version parts are meaningful. The code already has access to `bundle.csv_operator_name` and `bundle.csv_operator_version`.

### Fix Focus Areas
- operatorcert/static_tests/common/bundle.py[245-271]

### Suggested fix
- Parse `spec.replaces` into `(replaces_name, replaces_version)`.
- If `replaces_name != bundle.csv_operator_name`, `yield Fail(...)` explaining the mismatch and `return`.
- Keep using `replaces_version` for `ver_to_dir` lookup after name validation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand Down
10 changes: 5 additions & 5 deletions poetry.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions tests/static_tests/common/test_bundle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,19 @@ def test_check_operator_version_directory_name(
},
id="Nonexistent replaces version",
),
pytest.param(
None,
None,
"hello.v0.0.2",
[],
{
Fail(
"Bundle(hello/0.0.2) attempts to replace itself. Please set "
"'spec.replaces' field to a different version in the CSV."
)
},
id="Bundle attempts to replace itself",
),
],
)
@patch("operatorcert.static_tests.common.bundle.utils.get_ocp_supported_versions")
Expand Down
Loading