ART-14474 add latest assembly release to fbc catalog#2432
ART-14474 add latest assembly release to fbc catalog#2432Ximinhan wants to merge 13 commits intoopenshift-eng:mainfrom
Conversation
|
[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:
WalkthroughAdd assembly-bundle CSV discovery and optional bundle-blob retrieval to Konflux FBC importer/rebaser; integrate discovered Changes
Sequence Diagram(s)sequenceDiagram
participant Rebase as Rebase Flow
participant Importer as KonfluxFbcImporter/Rebaser
participant Shipment as Shipment Resolver
participant Git as Git Repo
participant Catalog as Catalog Builder
Rebase->>Importer: _get_assembly_bundle_csv_info(metadata, logger)
Importer->>Shipment: resolve assembly & shipment config
Shipment-->>Importer: shipment config (bundle/component info)
Importer->>Git: _fetch_csv_from_git(bundle_git_url, revision, work_dir, logger)
Git-->>Importer: CSV content (csv_name, skip_range)
alt bundle blob present
Importer->>Catalog: fetch/render bundle -> create olm.bundle blob
Catalog-->>Importer: bundle_blob
end
Importer-->>Rebase: AssemblyBundleCsvInfo(csv_name, skip_range, bundle_blob?)
Rebase->>Rebase: add csv_name to channel skips, add assembly channel entry, include bundle_blob in catalog_blobs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ 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
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)
934-967:⚠️ Potential issue | 🟠 MajorGuard against adding the current bundle to its own
skips.
Ifassembly_csv_info.csv_nameequalsolm_bundle_name(likely for named-assembly builds), Line 961 will add a self‑skip. That can corrupt the upgrade graph.🔧 Suggested guard
- if assembly_csv_info: + if assembly_csv_info and assembly_csv_info.csv_name != olm_bundle_name:
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 678-757: The current _get_assembly_bundle_csv_info uses a shared
work_dir (self.base_dir / f"assembly_csv_{assembly}") which can collide under
concurrent runs; change it to create a unique per-call temp directory (e.g., use
tempfile.TemporaryDirectory() or mkdtemp with a uuid) and pass that path into
_fetch_csv_from_git instead of the shared self.base_dir path, ensuring the temp
dir is created, used for the git checkout, and cleaned up afterwards; update
references to work_dir and the call to _fetch_csv_from_git so each invocation of
_get_assembly_bundle_csv_info gets an isolated directory.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 744-754: The current substring match using "if operator_name in
component.name" can return wrong components; update the matching in the loop
that inspects metadata_shipment.shipment.snapshot.spec.components to use a
stricter rule (e.g., exact equality or suffix matching based on your bundle
naming convention) when assigning bundle_component (inspect component.name) —
for example require component.name to end with the expected bundle suffix or
match a constructed canonical name derived from operator_name, and keep the
existing debug/return behavior if no bundle_component is found; ensure you
update the condition so it no longer performs a simple substring contains check.
- Around line 107-120: The current code embeds GITHUB_TOKEN into clone_url
(variable clone_url set from git_url and modified when constants.GITHUB_TOKEN is
present) which may leak secrets via git_helper.run_git_async call; change to
avoid putting the token in the URL by using a credential helper or
environment-based auth: when constants.GITHUB_TOKEN exists, keep clone_url as
the plain git_url and instead supply credentials via a temporary git credential
helper or by setting a protected environment/askpass mechanism for the
subprocess invoked by git_helper.run_git_async (do not modify git_url), ensure
the token is never concatenated into strings that will be logged (referencing
clone_url, git_helper.run_git_async, github_token and repo_dir to locate edits).
🧹 Nitpick comments (1)
doozer/doozerlib/backend/konflux_fbc.py (1)
943-945: Remove redundant initialization.The variable is initialized to
Noneand immediately reassigned on the next line.♻️ Suggested simplification
# Get assembly bundle info from the latest named assembly's shipment - assembly_csv_info: Optional[AssemblyBundleCsvInfo] = None - assembly_csv_info = await self._get_assembly_bundle_csv_info(metadata, logger) + assembly_csv_info = await self._get_assembly_bundle_csv_info(metadata, logger)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 135-146: The CSV lookup in the block that defines manifests_dir,
csv_files, and csv_file is non-recursive and will miss files nested under
manifests/<bundle>/...; change the search to a recursive search (use
manifests_dir.rglob or equivalent) to find clusterserviceversion.yaml anywhere
under manifests, keep the existing "no results" warning, and add an explicit
multiple-match guard: if more than one CSV is found log/warn about the ambiguity
and return None (or otherwise handle deterministically) instead of silently
proceeding.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 1004-1014: The current block in konflux_fbc.py adds
assembly_csv_info.csv_name to skips and channel['entries'] unconditionally,
which can cause a bundle to skip itself when assembly_csv_info.csv_name ==
olm_bundle_name; update the logic to first check that assembly_csv_info.csv_name
!= olm_bundle_name and only then add to skips (the skips set) and append the
assembly_entry to channel['entries']; ensure you still initialize skips as a set
when None and preserve skipRange handling on assembly_entry.
- Around line 797-803: The opm.render call for the FBC image
(opm.render(fbc_pullspec)) omits registry authentication and can silently fail
for private images; construct the registry auth using the same pattern used
elsewhere in this module (the auth construction used near the other opm.render
calls) and pass it into opm.render for the fbc_pullspec invocation so the render
uses credentials (update the call where fbc_component.containerImage is used and
ensure the resulting AssemblyBundleCsvInfo path remains the same).
| # 8. Render the FBC image to get the olm.bundle blob | ||
| fbc_pullspec = fbc_component.containerImage | ||
| logger.info("Rendering FBC image %s to get bundle blob", fbc_pullspec) | ||
| blobs = await opm.render(fbc_pullspec) | ||
| if not blobs: | ||
| logger.warning("No catalog blobs found in FBC image %s", fbc_pullspec) | ||
| return AssemblyBundleCsvInfo(csv_name=csv_name, skip_range=skip_range, bundle_blob=None) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all opm.render call sites and auth file usage patterns
echo "=== opm.render call sites ==="
rg -n "opm\.render" --type py -C3
echo -e "\n=== KONFLUX_ART_IMAGES_AUTH_FILE usage ==="
rg -n "KONFLUX_ART_IMAGES_AUTH_FILE" --type py -C3
echo -e "\n=== OpmRegistryAuth references ==="
rg -n "OpmRegistryAuth" --type py -C2Repository: openshift-eng/art-tools
Length of output: 22675
Add registry auth to prevent silent failures for private FBC images.
Line 800's opm.render() call lacks authentication while other render calls in the same file (lines 286, 445, 1277) all pass auth. If the FBC image is in a private registry, this will silently fail and drop the bundle blob. Use the established pattern from line 1274 for constructing auth:
Fix
+ registry_auth = opm.OpmRegistryAuth(
+ path=os.environ.get("KONFLUX_ART_IMAGES_AUTH_FILE"),
+ )
- blobs = await opm.render(fbc_pullspec)
+ blobs = await opm.render(fbc_pullspec, auth=registry_auth)🤖 Prompt for AI Agents
In `@doozer/doozerlib/backend/konflux_fbc.py` around lines 797 - 803, The
opm.render call for the FBC image (opm.render(fbc_pullspec)) omits registry
authentication and can silently fail for private images; construct the registry
auth using the same pattern used elsewhere in this module (the auth construction
used near the other opm.render calls) and pass it into opm.render for the
fbc_pullspec invocation so the render uses credentials (update the call where
fbc_component.containerImage is used and ensure the resulting
AssemblyBundleCsvInfo path remains the same).
Ximinhan
added
the
tide/merge-method-squash
| try: | ||
| # Use GITHUB_TOKEN for authentication if available | ||
| clone_url = git_url | ||
| github_token = os.environ.get(constants.GITHUB_TOKEN) |
There was a problem hiding this comment.
Why do we need to use the token here? Wouldn't buildvm ssh be enough?
There was a problem hiding this comment.
from my test job, seems find an issue at clone
| def get_fbc_name(image_name: str): | ||
| return f"{image_name}-fbc" | ||
|
|
||
| async def _get_assembly_bundle_csv_info( |
There was a problem hiding this comment.
I'm trying to think if there's any other way than looking up shipment MR fbc build .. we can use the candidate index instead of stable to rebase?
| if not release_names: | ||
| logger.debug("No releases found in releases.yml") | ||
| return None | ||
| assembly = release_names[0] |
There was a problem hiding this comment.
We should not rely on the an assembly being latest based on order in the yaml file. Some combination and validation of assembly type, release date, and assembly name sorting would be ideal. Example scenario - a non-standard assembly artXYZ being be at the top of releases.yaml.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 1148-1156: When adding the assembly bundle blob in konflux_fbc
(the block using assembly_csv_info, assembly_bundle_name and
categorized_catalog_blobs), guard against adding the package's own bundle by
checking that assembly_bundle_name != olm_bundle_name before logging or
inserting; only proceed to call
categorized_catalog_blobs[olm_package].setdefault("olm.bundle", {}) and append
to catalog_blobs when the names differ to avoid self-referential/duplicate
entries. Ensure the same symbols (assembly_csv_info.bundle_blob,
assembly_bundle_name, olm_bundle_name, categorized_catalog_blobs, catalog_blobs)
are used in the conditional so behavior matches the existing skip logic
elsewhere.
🧹 Nitpick comments (2)
doozer/doozerlib/backend/konflux_fbc.py (2)
147-149: Catching bareExceptionhides the root cause of failures.The broad exception handler logs only a warning and returns
None, which makes debugging difficult when things go wrong. Consider catching more specific exceptions or at least logging the full traceback.🔧 Suggested improvement
- except Exception as e: - logger.warning("Failed to fetch CSV from git: %s", e) + except Exception: + logger.exception("Failed to fetch CSV from git") return None
912-914: Broad exception handler masks specific failure causes.The catch-all
except Exceptionat line 912 makes it difficult to distinguish between different failure modes (network errors, parsing errors, missing data, etc.). Consider catching more specific exceptions or at least logging the exception type.🔧 Suggested improvement
- except Exception as e: - logger.warning("Failed to get assembly bundle CSV info: %s", e) + except Exception: + logger.exception("Failed to get assembly bundle CSV info") return None
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1-13:⚠️ Potential issue | 🟡 MinorPipeline failure: Ruff format check failed.
The CI pipeline indicates that this file needs reformatting. Run
ruff format doozer/doozerlib/backend/konflux_fbc.pyto fix the formatting issues before merging.
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 701-711: Fix the typo in the info log: change the message logged
by the logger.info call that currently says "get relese date {release_date}" to
"get release date {release_date}" (the call referencing
release_name/release_date in the block that constructs group_config via
assembly_config_struct(Model(releases_config), release_name, "group", {}) and
then checks artlib_util.is_future_release_date(release_date)); update only the
log string to correct "relese" → "release".
- Around line 789-796: The call passes self.base_dir into _fetch_csv_from_git
which creates and reinitializes a csv_repo subdirectory, causing race conditions
when multiple operators run concurrently; update the code that calls
_fetch_csv_from_git (the call site using bundle_component and logger) to create
and pass a unique per-call temporary working directory (e.g., via
tempfile.mkdtemp or tempfile.TemporaryDirectory) instead of self.base_dir,
ensure that directory is cleaned up after the fetch (or let a context manager
handle cleanup), and keep using _fetch_csv_from_git, csv_repo, and
bundle_component references so the change is local to the call parameters.
| git_url = bundle_component.source.git.url | ||
| revision = bundle_component.source.git.revision | ||
| logger.info("Found bundle component %s at %s@%s", bundle_component.name, git_url, revision) | ||
|
|
||
| csv_data = await _fetch_csv_from_git(git_url, revision, self.base_dir, logger) | ||
| if not csv_data: | ||
| logger.warning("Could not fetch CSV from bundle source") | ||
| return None |
There was a problem hiding this comment.
Potential race condition with shared work directory.
Line 793 passes self.base_dir directly to _fetch_csv_from_git, which creates a csv_repo subdirectory. If multiple operators are processed concurrently (e.g., via asyncio.gather), they will collide on the same directory path since _fetch_csv_from_git deletes and recreates csv_repo.
Consider using a unique temporary directory per call:
🔧 Suggested fix
+ with TemporaryDirectory(prefix=f"assembly_csv_{bundle_component.name}_") as tmpdir:
- csv_data = await _fetch_csv_from_git(git_url, revision, self.base_dir, logger)
+ csv_data = await _fetch_csv_from_git(git_url, revision, Path(tmpdir), logger)
if not csv_data:
logger.warning("Could not fetch CSV from bundle source")
return None🤖 Prompt for AI Agents
In `@doozer/doozerlib/backend/konflux_fbc.py` around lines 789 - 796, The call
passes self.base_dir into _fetch_csv_from_git which creates and reinitializes a
csv_repo subdirectory, causing race conditions when multiple operators run
concurrently; update the code that calls _fetch_csv_from_git (the call site
using bundle_component and logger) to create and pass a unique per-call
temporary working directory (e.g., via tempfile.mkdtemp or
tempfile.TemporaryDirectory) instead of self.base_dir, ensure that directory is
cleaned up after the fetch (or let a context manager handle cleanup), and keep
using _fetch_csv_from_git, csv_repo, and bundle_component references so the
change is local to the call parameters.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Around line 1095-1108: The nested function _update_channel contains a bare
"continue" (when assembly_csv_info.csv_name == olm_bundle_name) which is invalid
outside a loop; replace that control flow by guarding the subsequent logic with
an if/else (or early return from _update_channel) so processing is skipped when
assembly_csv_info.csv_name == olm_bundle_name; specifically, in the block that
checks assembly_csv_info, remove the continue and wrap the skips.add(...),
channel['entries'] append logic inside an if assembly_csv_info.csv_name !=
olm_bundle_name (or use return to exit _update_channel), keeping the skips
initialization (skips = set()) and use assembly_csv_info, olm_bundle_name,
skips, and channel to locate the code to change.
- Around line 769-771: The loop over shipment.shipment.snapshot.spec.components
uses a substring check "if operator_name in component.name" which can yield
false positives; change it to a stricter match (e.g., use equality:
"component.name == operator_name", or if names include suffixes, use a
deterministic pattern match such as "component.name.startswith(operator_name +
'-')") so only the intended component is returned; update the check in the loop
that references operator_name and component.name accordingly.
🧹 Nitpick comments (2)
doozer/doozerlib/backend/konflux_fbc.py (2)
19-19: Unused import:assembly_config_struct.The
assembly_config_structfunction is imported but not used anywhere in the code. The_find_future_release_assemblymethod accessesreleases_configdirectly instead of using this function.🧹 Suggested fix
-from artcommonlib.assembly import assembly_config_struct
749-754: Unusedloggerparameter.The
loggerparameter is declared but never used in this method. Either use it for debug logging or remove it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/konflux_fbc.py`:
- Line 19: Remove the unused import assembly_config_struct from the top of the
module (the line "from artcommonlib.assembly import assembly_config_struct") in
konflux_fbc.py; simply delete that import so unused symbol
assembly_config_struct is not imported but leave other imports and surrounding
code unchanged.
|
A lot of finicky logic here that is easy to break. Would be good with unit tests that define the correct results with a set of inputs. |
|
@Ximinhan: 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. |
3 participants
ref: https://redhat-internal.slack.com/archives/GDBRP5YJH/p1769759933814849
class AssemblyBundleCsvInfo(NamedTuple):
csv_name: str
skip_range: Optional[str]
need openshift-eng/aos-cd-jobs#4567
Testing
To verify:
Look at the catalog part,