fix(doozer): collect skips from all FBC channel entries, not just the first

[eemurphy]

Summary

Fix FBC build failures for operators with branching upgrade graphs (e.g. ACM on OCP 4.21/4.22) caused by the skips migration logic in _update_channel only collecting skips from the first entry with a skips field.

The existing next() + pop + union logic is unchanged. A small loop is added after it to collect skips from any remaining entries that also have them — this is a no-op for linear chains (every team today) and only activates for branching graphs (ACM).

skipRange does not count for reachability in opm validate — only explicit skips and replaces. So all entries with skip relationships must be collected to prevent stranded bundles.

Test plan

• New unit test test_rebase_dir_branching_skips_graph — verifies ACM-style branching graph with 3 entries having independent skips
• All 77 existing tests in test_konflux_fbc.py pass (backward compatible — linear chains produce identical results)
• Re-run build-fbc Jenkins job for multiclusterhub-operator on OCP 4.21/4.22 to verify end-to-end

Summary by CodeRabbit

• Bug Fixes
  • Improved rebasing behavior for catalog channel skip links when multiple channel entries define skips.
  • When a replaces-managed update graph is detected, existing replaces/skips details are now preserved instead of being migrated or consolidated incorrectly.
  • For branching channels using skips without replaces, the latest entry now receives the full union of skipped versions, with ownership moved off prior entries and head selection corrected.

[openshift-ci]

Hi @eemurphy. Thanks for your PR.

I'm waiting for a openshift-eng member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 kubernetes-sigs/prow repository.

[coderabbitai]

Walkthrough

KonfluxFbcRebaser._rebase_dir now preserves replaces-based channel graphs, aggregates skips across skips-based entries, and accounts for skips when selecting the current head. Tests cover both graph behaviors.

Changes

Konflux FBC channel graph rebasing

Layer / File(s)	Summary
Channel graph detection and head selection doozer/doozerlib/backend/konflux_fbc.py	Rebase logic preserves channels with replaces relationships, aggregates skips from multiple entries otherwise, and includes skipped names when determining the current head.
Graph-specific rebase tests doozer/tests/backend/test_konflux_fbc.py	Tests verify preservation of replaces chains and migration of branching skips into the new head.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: ashwindasr, locriandev, lgarciaaco

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: aggregating skips across FBC channel entries instead of only the first one.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Weak-Crypto	✅ Passed	Changed files only adjust FBC channel/skip logic and tests; searches found no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or secret comparisons.
Container-Privileges	✅ Passed	Only a backend Python file changed; no container/K8s manifests or privileged settings were introduced in the PR.
No-Sensitive-Data-In-Logs	✅ Passed	Changed code adds only a generic channel log; no new logging of secrets, PII, hostnames, or customer data was found in the touched files.
No-Hardcoded-Secrets	✅ Passed	No added lines in the PR contain hardcoded secrets, embedded credentials, or suspicious secret-like literals; changes only adjust skips/replaces graph handling and tests.
No-Injection-Vectors	✅ Passed	PASS: The diff only adjusts FBC skip/replaces graph logic; no SQL/shell/eval/pickle/yaml.load/os.system/dangerouslySetInnerHTML injection vector was added.
Ai-Attribution	✅ Passed	PR commits 1b5c29c and 9a7979e have no AI-tool mentions or Co-Authored-By trailers; no Assisted-by/Generated-by attribution was required.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ashwindasr]

/ok-to-test

[coderabbitai]

🧹 Nitpick comments (1)

doozer/doozerlib/backend/konflux_fbc.py (1)

1204-1232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Optional: extract skip-aggregation into a small helper for testability.

The nested branch inside _update_channel (find first entry with skips, aggregate, then loop remaining entries) is flagged as medium complexity. Extracting this into a standalone helper (e.g. _aggregate_channel_skips(channel, olm_bundle_name)) would let it be unit-tested independently of the full _rebase_dir flow, reducing the amount of mocking needed for future edge-case tests.

🤖 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/backend/konflux_fbc.py` around lines 1204 - 1232, The skip
aggregation logic inside _update_channel is too nested and hard to test
directly. Extract the block that finds the first entry with skips, merges skips
from the remaining channel['entries'], and removes olm_bundle_name into a small
helper such as _aggregate_channel_skips(channel, olm_bundle_name). Keep the
behavior unchanged, but have _update_channel call the helper so the aggregation
can be unit-tested independently.

🤖 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 `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 1204-1232: The skip aggregation logic inside _update_channel is
too nested and hard to test directly. Extract the block that finds the first
entry with skips, merges skips from the remaining channel['entries'], and
removes olm_bundle_name into a small helper such as
_aggregate_channel_skips(channel, olm_bundle_name). Keep the behavior unchanged,
but have _update_channel call the helper so the aggregation can be unit-tested
independently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c55b24de-ced8-408d-8334-cb75cdb40031

📥 Commits

Reviewing files that changed from the base of the PR and between e272a02 and 1b5c29c.

📒 Files selected for processing (2)

• doozer/doozerlib/backend/konflux_fbc.py
• doozer/tests/backend/test_konflux_fbc.py

[eemurphy]

/retest

[rayfordj]

/hold for layered-product tests (ACM, Logging, MTC, & OADP)

[rayfordj]

• 🟢 ACM -> fbc-acm-2-16-multiclusterhub-operator-t6dpb
• 🟢 Logging -> fbc-logging-6-4-loki-operator-bdlxs
• 🟢 MTC -> fbc-mtc-1-8-openshift-migration-operator-2hfht
• 🟢 OADP -> fbc-oadp-1-6-oadp-operator-ql9lg

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

doozer/doozerlib/backend/konflux_fbc.py (1)

1231-1237: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Compute the channel head before migrating skips.

The migration removes the edges needed by head detection, while the regression test only checks that some replaces value exists.

• doozer/doozerlib/backend/konflux_fbc.py#L1231-L1237: snapshot superseded names before popping existing skips.
• doozer/tests/backend/test_konflux_fbc.py#L2310-L2310: assert that the new entry replaces advanced-cluster-management.v2.16.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 `@doozer/doozerlib/backend/konflux_fbc.py` around lines 1231 - 1237, The
channel head must be determined before migrating and removing existing skips. In
doozer/doozerlib/backend/konflux_fbc.py lines 1231-1237, snapshot the superseded
entry names and compute the head before popping skips, then perform the
migration using that snapshot. In doozer/tests/backend/test_konflux_fbc.py line
2310, strengthen the regression assertion so the new entry explicitly replaces
advanced-cluster-management.v2.16.0.

🤖 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/tests/backend/test_konflux_fbc.py`:
- Line 2310: Update the assertion for new_entry to verify that its replacement
value exactly matches the unsuperseded v2.16.0 entry, rather than only checking
that the "replaces" field exists. Preserve the surrounding test setup and use an
equality assertion against the expected replacement head.

---

Outside diff comments:
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 1231-1237: The channel head must be determined before migrating
and removing existing skips. In doozer/doozerlib/backend/konflux_fbc.py lines
1231-1237, snapshot the superseded entry names and compute the head before
popping skips, then perform the migration using that snapshot. In
doozer/tests/backend/test_konflux_fbc.py line 2310, strengthen the regression
assertion so the new entry explicitly replaces
advanced-cluster-management.v2.16.0.

🪄 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: 9ab5ad0d-6edf-44fe-b379-0977f2af8cbf

📥 Commits

Reviewing files that changed from the base of the PR and between 1b5c29c and 9a7979e.

📒 Files selected for processing (2)

• doozer/doozerlib/backend/konflux_fbc.py
• doozer/tests/backend/test_konflux_fbc.py

[rayfordj]

/hold for @eemurphy review against art-fbc:multiclusterhub-operator-fbc-2.16.3-20260713214157.ocp4.22
/label tide/merge-method-squash
/lgtm
/approve

[rayfordj]

/unhold
/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rayfordj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [rayfordj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

🧹 Nitpick comments (1)

doozer/doozerlib/backend/konflux_fbc.py (1)

1293-1299: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Safeguard against explicit null values in YAML.

If the parsed catalog YAML explicitly contains skips: null, it.get('skips', []) will return None rather than the default [], leading to a TypeError when passed to update(). Using it.get('skips') or [] ensures that a falsy None is safely evaluated as an empty list.

♻️ Proposed refactor

-                        for it in all_entries:
-                            superseded_names.update(it.get('skips', []))
+                        for it in all_entries:
+                            superseded_names.update(it.get('skips') or [])

🤖 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/backend/konflux_fbc.py` around lines 1293 - 1299, Update the
superseded-name collection in the current-head computation to normalize an
explicit null skips value to an empty list before passing it to
superseded_names.update(). Modify the it.get('skips', []) expression within the
loop over all_entries, preserving existing behavior for populated skip lists.

🤖 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 `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 1293-1299: Update the superseded-name collection in the
current-head computation to normalize an explicit null skips value to an empty
list before passing it to superseded_names.update(). Modify the it.get('skips',
[]) expression within the loop over all_entries, preserving existing behavior
for populated skip lists.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ef2abed4-bbbb-4d05-907e-a6d6fd34ea88

📥 Commits

Reviewing files that changed from the base of the PR and between 1b5c29c and c679c00.

📒 Files selected for processing (2)

• doozer/doozerlib/backend/konflux_fbc.py
• doozer/tests/backend/test_konflux_fbc.py

🚧 Files skipped from review as they are similar to previous changes (1)

• doozer/tests/backend/test_konflux_fbc.py

[openshift-ci]

@eemurphy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	c679c00	link	false	/test security

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.