Handling of fbc-operations for containerized IIB #1257
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideImplements a new containerized FBC operations workflow that orchestrates Git/Konflux-based catalog rebuilds, adds utilities for index artifacts and output pullspecs, wires the API to the new Celery task, and extends tests/utilities for safer image/label handling. Sequence diagram for containerized FBC operation handlingsequenceDiagram
actor Client as "Client"
participant API as "API v1 (fbc_operations)"
participant Celery as "Celery Broker"
participant Task as "Task: handle_containerized_fbc_operation_request"
participant Registry as "Container Registry"
participant ArtifactReg as "Artifact Registry (index.db)"
participant GitHost as "Git Host (e.g., GitLab)"
participant Konflux as "Konflux Pipeline"
Client->>API: "POST /fbc-operations (from_index, fbc_fragments, ...)"
API->>Celery: "enqueue handle_containerized_fbc_operation_request(request_id, ...)"
Celery-->>API: "202 Accepted (request queued)"
Celery->>Task: "dispatch task with args"
Task->>Task: "_cleanup() and set_request_state('in_progress', 'Resolving the fbc fragments')"
loop "Resolve each FBC fragment"
Task->>Registry: "set_registry_token(..., fbc_fragment) and get_resolved_image(fbc_fragment)"
Registry-->>Task: "resolved fragment pullspec"
end
Task->>Task: "prepare_request_for_build(...RequestConfigFBCOperation...)"
Task->>Registry: "set_registry_token(..., from_index_resolved) and Opm.set_opm_version(from_index_resolved)"
Task->>ArtifactReg: "pull_index_db_artifact(from_index, temp_dir)"
ArtifactReg-->>Task: "'/tmp/.../index.db' directory path"
Task->>Task: "resolve_git_url(from_index, index_to_gitlab_push_map)"
Task->>GitHost: "get_git_token() and clone_git_repo(index_git_repo, branch, token, local_path)"
GitHost-->>Task: "local clone with 'configs' catalog"
Task->>Task: "opm_registry_add_fbc_fragment_containerized(request_id, temp_dir, configs_dir, resolved_fbc_fragments, overwrite_from_index_token, index_db_path)"
Task->>Task: "write_build_metadata(local_git_repo_path, ...)"
alt "overwrite_from_index_token is not set (throw-away request)"
Task->>GitHost: "create_mr(..., commit_message='IIB: Add data from FBC fragments ...')"
GitHost-->>Task: "merge request details (mr_url, mr_id)"
else "overwrite_from_index_token is set (persistent change)"
Task->>GitHost: "commit_and_push(..., branch, commit_message)"
GitHost-->>Task: "updated branch with new commit"
end
Task->>GitHost: "get_last_commit_sha(local_git_repo_path)"
GitHost-->>Task: "last_commit_sha"
Task->>Konflux: "find_pipelinerun(last_commit_sha)"
Konflux-->>Task: "pipelinerun list (including name)"
Task->>Konflux: "wait_for_pipeline_completion(pipelinerun_name)"
Konflux-->>Task: "completed run with IMAGE_URL result"
Task->>Task: "get_pipelinerun_image_url(pipelinerun_name, run)"
Task->>Task: "get_list_of_output_pullspec(request_id, build_tags)"
loop "For each output pullspec"
Task->>Registry: "_skopeo_copy('docker://' + IMAGE_URL, 'docker://' + output_pull_spec, copy_all=True)"
Registry-->>Task: "index copied to IIB registry tag"
end
Task->>Task: "_update_index_image_pull_spec(..., output_pull_spec, arches, overwrite_from_index, ...)"
opt "overwrite_from_index and operators_in_db are set (update index.db artifact)"
Task->>ArtifactReg: "push_index_db_artifact(request_id, from_index, index_db_path, operators, operators_in_db, overwrite_from_index, request_type='rm')"
ArtifactReg-->>Task: "original_index_db_digest or null"
end
opt "Merge request was created"
Task->>GitHost: "close_mr(mr_details, index_git_repo)"
GitHost-->>Task: "MR closed (best-effort)"
end
Task->>Task: "set_request_state(request_id, 'complete', 'The operator(s) ... were successfully removed from the index image')"
Flow diagram for handle_containerized_fbc_operation_request taskflowchart TD
A["Start task\nhandle_containerized_fbc_operation_request"] --> B["_cleanup() and set_request_state('in_progress', 'Resolving the fbc fragments')"]
B --> C["Resolve all fbc_fragments via set_registry_token() and get_resolved_image()"]
C --> D["prepare_request_for_build(request_id, RequestConfigFBCOperation(...))"]
D --> E["Set Opm.opm_version using Opm.set_opm_version(from_index_resolved)"]
E --> F["Create TemporaryDirectory(prefix='iib-{request_id}-')"]
F --> G["pull_index_db_artifact(from_index, temp_dir)\n-> artifact_dir and 'index.db' path"]
G --> H{"index.db exists at artifact_dir/index.db?"}
H -- "No" --> H_err["Raise IIBError('Artifact DB file not found ...')"]
H_err --> Z["cleanup_on_failure() (via Celery error callback) and fail request"]
H -- "Yes" --> I["resolve_git_url(from_index, index_to_gitlab_push_map) -> index_git_repo"]
I --> J{"index_git_repo resolved?"}
J -- "No" --> J_err["Raise IIBError('Cannot resolve the git repository for from_index')"]
J_err --> Z
J -- "Yes" --> K["get_git_token(index_git_repo) and determine branch (ocp_version)"]
K --> L["Create local_git_repo_path under temp_dir and clone_git_repo(index_git_repo, branch, token)"]
L --> M{"Catalog directory 'configs' exists in local repo?"}
M -- "No" --> M_err["Raise IIBError('Catalogs directory not found in local repo')"]
M_err --> Z
M -- "Yes" --> N["Call opm_registry_add_fbc_fragment_containerized(request_id, temp_dir, configs_dir, resolved_fbc_fragments, overwrite_from_index_token, index_db_path) -> updated_catalog_path, new_index_db_path, local_cache_path, operators_in_db"]
N --> O["write_build_metadata(local_git_repo_path, opm_version, ocp_version, distribution_scope, binary_image_resolved, request_id)"]
O --> P{"overwrite_from_index_token provided?"}
P -- "No (throw-away request)" --> Q1["create_mr(request_id, local_repo_path, repo_url, branch, commit_message) -> mr_details"]
P -- "Yes (persistent change)" --> Q2["commit_and_push(request_id, local_repo_path, repo_url, branch, commit_message)"]
Q1 --> R["get_last_commit_sha(local_git_repo_path) -> last_commit_sha"]
Q2 --> R
R --> S["find_pipelinerun(last_commit_sha) -> pipelines (with retry)"]
S --> T["Select first pipelinerun and get its name"]
T --> U["wait_for_pipeline_completion(pipelinerun_name) -> run (completed)"]
U --> V["get_pipelinerun_image_url(pipelinerun_name, run) -> IMAGE_URL"]
V --> W["get_list_of_output_pullspec(request_id, build_tags) -> list of output_pull_specs"]
W --> X["For each output_pull_spec: _skopeo_copy('docker://' + IMAGE_URL, 'docker://' + output_pull_spec, copy_all=True)"]
X --> Y["_update_index_image_pull_spec(output_pull_spec=output_pull_specs[0], request_id, arches, from_index, overwrite_from_index, overwrite_from_index_token, resolved_prebuild_from_index, add_or_rm=True, is_image_fbc=True, index_repo_map={})"]
Y --> Y1{"operators_in_db and new_index_db_path set?"}
Y1 -- "Yes" --> Y2["push_index_db_artifact(request_id, from_index, new_index_db_path, operators_in_db, set(operators_in_db), overwrite_from_index, request_type='rm') -> original_index_db_digest or None"]
Y1 -- "No" --> Y3["Skip index.db artifact push"]
Y2 --> Y4["If MR was created: close_mr(mr_details, index_git_repo) (best-effort)"]
Y3 --> Y4
Y4 --> Y5["set_request_state(request_id, 'complete', 'The operator(s) ... were successfully removed from the index image')"]
Y5 --> END["End task (success)"]
subgraph "Error handling (try/except around main flow)"
E1["Any exception after opm_registry_add_fbc_fragment_containerized"] --> E2["cleanup_on_failure(mr_details, last_commit_sha, index_git_repo, overwrite_from_index, request_id, from_index, index_repo_map, original_index_db_digest, reason)"]
E2 --> E3["Raise IIBError('Failed to add FBC fragment: ...')"]
E3 --> Z
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
lipoja
force-pushed
the
containerized_fbc_operations
branch
7 times, most recently
from
fe9f569 to
304ff91
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In handle_containerized_fbc_operation_request, the context manager that wraps Opm.set_opm_version uses set_registry_token(overwrite_from_index_token, fbc_fragment, ...), but at that point you likely want to target from_index_resolved instead of the last fragment, otherwise auth/resolution for the base index image may fail or be inconsistent.
- handle_containerized_fbc_operation_request defines index_to_gitlab_push_map with a mutable default argument ({}), which can cause state leakage between task invocations; default this to None and normalize to an empty dict inside the function instead.
- In the main try/except of handle_containerized_fbc_operation_request, variables like last_commit_sha and original_index_db_digest are only assigned partway through the try-block but are unconditionally passed to cleanup_on_failure in the except path; initialize them to None before the try to avoid potential UnboundLocalError on early failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In handle_containerized_fbc_operation_request, the context manager that wraps Opm.set_opm_version uses set_registry_token(overwrite_from_index_token, fbc_fragment, ...), but at that point you likely want to target from_index_resolved instead of the last fragment, otherwise auth/resolution for the base index image may fail or be inconsistent.
- handle_containerized_fbc_operation_request defines index_to_gitlab_push_map with a mutable default argument ({}), which can cause state leakage between task invocations; default this to None and normalize to an empty dict inside the function instead.
- In the main try/except of handle_containerized_fbc_operation_request, variables like last_commit_sha and original_index_db_digest are only assigned partway through the try-block but are unconditionally passed to cleanup_on_failure in the except path; initialize them to None before the try to avoid potential UnboundLocalError on early failures.
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/build_containerized_fbc_operations.py:201-210` </location>
<code_context>
safe_args = _get_safe_args(args, payload)
error_callback = failed_request_callback.s(request.id)
try:
- handle_fbc_operation_request.apply_async(
+ handle_containerized_fbc_operation_request.apply_async(
</code_context>
<issue_to_address>
**issue (bug_risk):** `last_commit_sha` and `original_index_db_digest` may be referenced before assignment in the exception handler.
In the `except` block, `cleanup_on_failure` uses `last_commit_sha` and `original_index_db_digest`, but these are only set later in the `try`. If an error happens first, the handler will raise `UnboundLocalError` and hide the original exception. Please initialize both to `None` before the `try:` (as with `mr_details`) so `cleanup_on_failure` can safely handle missing values.
</issue_to_address>
### Comment 2
<location> `iib/workers/tasks/utils.py:1119` </location>
<code_context>
:rtype: str
"""
log.debug('Getting the label of %s from %s', label, pull_spec)
+ if "index.db" in pull_spec:
+ raise IIBError(f'Cannot get label "{label}" from {pull_spec}')
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a simple substring check for `"index.db"` in the pull spec may be overly broad.
This guard will raise an IIBError for any pull spec that merely contains the substring "index.db", which risks rejecting valid images whose repo or tag names include it. Consider narrowing the check (for example, to the final path segment or a specific artifact naming pattern) so that only true index.db artifacts are blocked and false positives are avoided.
</issue_to_address>
### Comment 3
<location> `iib/workers/tasks/build_containerized_fbc_operations.py:62` </location>
<code_context>
@app.task
@request_logger
@instrument_tracing(
span_name="workers.tasks.build.handle_containerized_fbc_operation_request",
attributes=get_binary_versions(),
)
def handle_containerized_fbc_operation_request(
request_id: int,
fbc_fragments: List[str],
from_index: str,
binary_image: Optional[str] = None,
distribution_scope: str = '',
overwrite_from_index: bool = False,
overwrite_from_index_token: Optional[str] = None,
build_tags: Optional[List[str]] = None,
add_arches: Optional[Set[str]] = None,
binary_image_config: Optional[Dict[str, Dict[str, str]]] = None,
index_to_gitlab_push_map: Dict[str, str] = {},
used_fbc_fragment: bool = False,
) -> None:
"""
Add fbc fragments to an fbc index image.
:param list fbc_fragments: list of fbc fragments that need to be added to final FBC index image
:param int request_id: the ID of the IIB build request
:param str binary_image: the pull specification of the container image where the opm binary
gets copied from.
:param str from_index: the pull specification of the container image containing the index that
the index image build will be based from.
:param set add_arches: the set of arches to build in addition to the arches ``from_index`` is
currently built for; if ``from_index`` is ``None``, then this is used as the list of arches
to build the index image for
:param dict index_to_gitlab_push_map: the dict mapping index images (keys) to GitLab repos
(values) in order to push their catalogs into GitLab.
:param bool used_fbc_fragment: flag indicating if the original request used fbc_fragment
(single) instead of fbc_fragments (array). Used for backward compatibility.
"""
_cleanup()
set_request_state(request_id, 'in_progress', 'Resolving the fbc fragments')
# Resolve all fbc fragments
resolved_fbc_fragments = []
for fbc_fragment in fbc_fragments:
with set_registry_token(overwrite_from_index_token, fbc_fragment, append=True):
resolved_fbc_fragment = get_resolved_image(fbc_fragment)
resolved_fbc_fragments.append(resolved_fbc_fragment)
prebuild_info = prepare_request_for_build(
request_id,
RequestConfigFBCOperation(
_binary_image=binary_image,
from_index=from_index,
overwrite_from_index_token=overwrite_from_index_token,
add_arches=add_arches,
fbc_fragments=fbc_fragments,
distribution_scope=distribution_scope,
binary_image_config=binary_image_config,
),
)
from_index_resolved = prebuild_info['from_index_resolved']
binary_image_resolved = prebuild_info['binary_image_resolved']
arches = prebuild_info['arches']
# Variable mr_details needs to be assigned, otherwise
# cleanup_on_failure() fails when an exception is raised.
mr_details: Optional[Dict[str, str]] = None
with set_registry_token(overwrite_from_index_token, fbc_fragment, append=True):
Opm.set_opm_version(from_index_resolved)
# Store all resolved fragments
prebuild_info['fbc_fragments_resolved'] = resolved_fbc_fragments
# For backward compatibility, only populate old fields if original request used fbc_fragment
# This flag should be passed from the API layer
if used_fbc_fragment and resolved_fbc_fragments:
prebuild_info['fbc_fragment_resolved'] = resolved_fbc_fragments[0]
_update_index_image_build_state(request_id, prebuild_info)
with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir:
artifact_dir = pull_index_db_artifact(
from_index,
temp_dir,
)
artifact_index_db_file = os.path.join(artifact_dir, "index.db")
log.debug("Artifact DB path %s", artifact_index_db_file)
if not os.path.exists(artifact_index_db_file):
log.error("Artifact DB file not found at %s", artifact_index_db_file)
raise IIBError(f"Artifact DB file not found at {artifact_index_db_file}")
index_git_repo = resolve_git_url(
from_index=from_index, index_repo_map=index_to_gitlab_push_map
)
if not index_git_repo:
raise IIBError(f"Cannot resolve the git repository for {from_index}")
log.info(
"Git repo for %s: %s",
from_index,
index_git_repo,
)
token_name, git_token = get_git_token(index_git_repo)
branch = prebuild_info['ocp_version']
set_request_state(request_id, 'in_progress', 'Cloning Git repository')
local_git_repo_path = os.path.join(temp_dir, 'git', branch)
os.makedirs(local_git_repo_path, exist_ok=True)
# TODO - GitClone takes time
# can we keep the copy and just pull the difference? (6min on dev-env)
clone_git_repo(index_git_repo, branch, token_name, git_token, local_git_repo_path)
localized_git_catalog_path = os.path.join(local_git_repo_path, 'configs')
if not os.path.exists(localized_git_catalog_path):
raise IIBError(f"Catalogs directory not found in {local_git_repo_path}")
set_request_state(request_id, 'in_progress', 'Adding fbc fragment')
(
updated_catalog_path,
index_db_path,
_,
operators_in_db,
) = opm_registry_add_fbc_fragment_containerized(
request_id=request_id,
temp_dir=temp_dir,
from_index_configs_dir=localized_git_catalog_path,
fbc_fragments=resolved_fbc_fragments,
overwrite_from_index_token=overwrite_from_index_token,
generate_cache=False,
index_db_path=artifact_index_db_file,
)
# Write build metadata to a file to be added with the commit
set_request_state(request_id, 'in_progress', 'Writing build metadata')
write_build_metadata(
local_git_repo_path,
Opm.opm_version,
prebuild_info['ocp_version'],
distribution_scope,
binary_image_resolved,
request_id,
)
try:
# Commit changes and create PR or push directly
set_request_state(request_id, 'in_progress', 'Committing changes to Git repository')
log.info("Committing changes to Git repository. Triggering KONFLUX pipeline.")
# Determine if this is a throw-away request (no overwrite_from_index_token)
if not overwrite_from_index_token:
# Create MR for throw-away requests
mr_details = create_mr(
request_id=request_id,
local_repo_path=local_git_repo_path,
repo_url=index_git_repo,
branch=branch,
commit_message=(
f"IIB: Add data from FBC fragments for request {request_id}\n\n"
f"FBC fragments: {', '.join(fbc_fragments)}"
),
)
log.info("Created merge request: %s", mr_details.get('mr_url'))
else:
# Push directly to the branch
commit_and_push(
request_id=request_id,
local_repo_path=local_git_repo_path,
repo_url=index_git_repo,
branch=branch,
commit_message=(
f"IIB: Add data from FBC fragments for request {request_id}\n\n"
f"FBC fragments: {', '.join(fbc_fragments)}"
),
)
# Get commit SHA before waiting for the pipeline (while the temp directory still exists)
last_commit_sha = get_last_commit_sha(local_repo_path=local_git_repo_path)
# Wait for Konflux pipeline
set_request_state(request_id, 'in_progress', 'Waiting on KONFLUX build')
# find_pipelinerun has retry decorator to handle delays in pipelinerun creation
pipelines = find_pipelinerun(last_commit_sha)
# Get the first pipelinerun (should typically be only one)
pipelinerun = pipelines[0]
pipelinerun_name = pipelinerun.get('metadata', {}).get('name')
if not pipelinerun_name:
raise IIBError("Pipelinerun name not found in pipeline metadata")
run = wait_for_pipeline_completion(pipelinerun_name)
set_request_state(request_id, 'in_progress', 'Copying built index to IIB registry')
# Extract IMAGE_URL from pipelinerun results
image_url = get_pipelinerun_image_url(pipelinerun_name, run)
output_pull_specs = get_list_of_output_pullspec(request_id, build_tags)
# Copy the built index from Konflux to all output pull specs
for spec in output_pull_specs:
_skopeo_copy(
source=f'docker://{image_url}',
destination=f'docker://{spec}',
copy_all=True,
exc_msg=f'Failed to copy built index from Konflux to {spec}',
)
log.info("Successfully copied image to %s", spec)
# Use the first output_pull_spec as the primary one for request updates
output_pull_spec = output_pull_specs[0]
# Update request with final output
if not output_pull_spec:
raise IIBError(
"output_pull_spec was not set. "
"This should not happen if the pipeline completed successfully."
)
_update_index_image_pull_spec(
output_pull_spec=output_pull_spec,
request_id=request_id,
arches=arches,
from_index=from_index,
overwrite_from_index=overwrite_from_index,
overwrite_from_index_token=overwrite_from_index_token,
resolved_prebuild_from_index=from_index_resolved,
add_or_rm=True,
is_image_fbc=True,
# Passing an empty index_repo_map is intentional. In IIB 1.0, if
# the overwrite_from_index token is given, we push to git by default
# at the end of a request. In IIB 2.0, the commit is pushed earlier to trigger
# a Konflux pipelinerun. So the old workflow isn't needed.
index_repo_map={},
)
# Push updated index.db if overwrite_from_index_token is provided
# We can push it directly from temp_dir since we're still inside the
# context manager. Do it as the last step to avoid rolling back the
# index.db file if the pipeline fails.
original_index_db_digest = push_index_db_artifact(
request_id=request_id,
from_index=from_index,
index_db_path=index_db_path,
operators=operators_in_db,
operators_in_db=set(operators_in_db),
overwrite_from_index=overwrite_from_index,
request_type='rm',
)
# Close MR if it was opened
if mr_details and index_git_repo:
try:
close_mr(mr_details, index_git_repo)
log.info("Closed merge request: %s", mr_details.get('mr_url'))
except IIBError as e:
log.warning("Failed to close merge request: %s", e)
set_request_state(
request_id,
'complete',
f"The operator(s) {operators_in_db} were successfully removed "
"from the index image",
)
except Exception as e:
cleanup_on_failure(
mr_details=mr_details,
last_commit_sha=last_commit_sha,
index_git_repo=index_git_repo,
overwrite_from_index=overwrite_from_index,
request_id=request_id,
from_index=from_index,
index_repo_map=index_to_gitlab_push_map or {},
original_index_db_digest=original_index_db_digest,
reason=f"error: {e}",
)
raise IIBError(f"Failed to add FBC fragment: {e}")
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Replace mutable default arguments with None ([`default-mutable-arg`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/default-mutable-arg/))
- Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
- Low code quality found in handle\_containerized\_fbc\_operation\_request - 19% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 4
<location> `iib/workers/tasks/opm_operations.py:1152` </location>
<code_context>
def opm_registry_add_fbc_fragment_containerized(
request_id: int,
temp_dir: str,
from_index_configs_dir: str,
fbc_fragments: List[str],
overwrite_from_index_token: Optional[str],
index_db_path: Optional[str] = None,
generate_cache=True,
) -> Tuple[str, str, Optional[str], List[str]]:
"""
Add FBC fragments to the from_index image.
This only produces the index.Dockerfile file and does not build the container image.
This also removes operators from index_db_path file if any are present.
:param int request_id: the id of IIB request
:param str temp_dir: the base directory to generate the database and index.Dockerfile in.
:param str from_index_configs_dir: path to the file-based catalog directory
:param list fbc_fragments: the list of pull specifications of fbc fragments to be added.
:param str overwrite_from_index_token: token used to access the image
:param str index_db_path: path to the index database file
"""
set_request_state(
request_id,
'in_progress',
f'Extracting operator packages from {len(fbc_fragments)} fbc fragment(s)',
)
# Single pass: Extract all fragment paths and operators
fragment_data = []
all_fragment_operators = []
for i, fbc_fragment in enumerate(fbc_fragments):
# fragment path will look like /tmp/iib-**/fbc-fragment-{index}
fragment_path, fragment_operators = extract_fbc_fragment(
temp_dir=temp_dir, fbc_fragment=fbc_fragment, fragment_index=i
)
fragment_data.append((fragment_path, fragment_operators))
all_fragment_operators.extend(fragment_operators)
# Single verification: Check for operators that already exist in the database
operators_in_db, index_db_path_local = verify_operators_exists(
from_index=None,
base_dir=temp_dir,
operator_packages=all_fragment_operators,
overwrite_from_index_token=overwrite_from_index_token,
index_db_path=index_db_path,
)
# Remove existing operators if any conflicts found
if operators_in_db:
remove_operator_deprecations(
from_index_configs_dir=from_index_configs_dir, operators=operators_in_db
)
log.info('Removing %s from index.db ', operators_in_db)
_opm_registry_rm(
index_db_path=index_db_path_local, operators=operators_in_db, base_dir=temp_dir
)
# migrated_catalog_dir path will look like /tmp/iib-**/catalog
migrated_catalog_dir, _ = opm_migrate(
index_db=index_db_path_local,
base_dir=temp_dir,
generate_cache=False,
)
log.info("Migrated catalog after removing from db at %s", migrated_catalog_dir)
# copy the content of migrated_catalog to from_index's config
log.info("Copying content of %s to %s", migrated_catalog_dir, from_index_configs_dir)
for operator_package in os.listdir(migrated_catalog_dir):
shutil.copytree(
os.path.join(migrated_catalog_dir, operator_package),
os.path.join(from_index_configs_dir, operator_package),
dirs_exist_ok=True,
)
# Copy operators to config directory using the collected data
for i, (fragment_path, fragment_operators) in enumerate(fragment_data):
set_request_state(
request_id,
'in_progress',
f'Adding package(s) {fragment_operators} from fbc fragment '
f'{i + 1}/{len(fbc_fragments)} to from_index',
)
for fragment_operator in fragment_operators:
# copy fragment_operator to from_index configs
fragment_opr_src_path = os.path.join(fragment_path, fragment_operator)
fragment_opr_dest_path = os.path.join(from_index_configs_dir, fragment_operator)
if os.path.exists(fragment_opr_dest_path):
shutil.rmtree(fragment_opr_dest_path)
log.info(
"Copying content of %s to %s",
fragment_opr_src_path,
fragment_opr_dest_path,
)
shutil.copytree(fragment_opr_src_path, fragment_opr_dest_path)
if generate_cache:
local_cache_path = os.path.join(temp_dir, 'cache')
generate_cache_locally(
base_dir=temp_dir, fbc_dir=from_index_configs_dir, local_cache_path=local_cache_path
)
else:
local_cache_path = None
return from_index_configs_dir, index_db_path_local, local_cache_path, operators_in_db
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into function ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+201
to
+210
| try: | ||
| # Commit changes and create PR or push directly | ||
| set_request_state(request_id, 'in_progress', 'Committing changes to Git repository') | ||
| log.info("Committing changes to Git repository. Triggering KONFLUX pipeline.") | ||
|
|
||
| # Determine if this is a throw-away request (no overwrite_from_index_token) | ||
| if not overwrite_from_index_token: | ||
| # Create MR for throw-away requests | ||
| mr_details = create_mr( | ||
| request_id=request_id, |
There was a problem hiding this comment.
issue (bug_risk): last_commit_sha and original_index_db_digest may be referenced before assignment in the exception handler.
In the except block, cleanup_on_failure uses last_commit_sha and original_index_db_digest, but these are only set later in the try. If an error happens first, the handler will raise UnboundLocalError and hide the original exception. Please initialize both to None before the try: (as with mr_details) so cleanup_on_failure can safely handle missing values.
| :rtype: str | ||
| """ | ||
| log.debug('Getting the label of %s from %s', label, pull_spec) | ||
| if "index.db" in pull_spec: |
There was a problem hiding this comment.
suggestion (bug_risk): Using a simple substring check for "index.db" in the pull spec may be overly broad.
This guard will raise an IIBError for any pull spec that merely contains the substring "index.db", which risks rejecting valid images whose repo or tag names include it. Consider narrowing the check (for example, to the final path segment or a specific artifact naming pattern) so that only true index.db artifacts are blocked and false positives are avoided.
| span_name="workers.tasks.build.handle_containerized_fbc_operation_request", | ||
| attributes=get_binary_versions(), | ||
| ) | ||
| def handle_containerized_fbc_operation_request( |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace mutable default arguments with None (
default-mutable-arg) - Explicitly raise from a previous error (
raise-from-previous-error) - Low code quality found in handle_containerized_fbc_operation_request - 19% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
|
|
||
| # Remove existing operators if any conflicts found | ||
| if operators_in_db: | ||
| remove_operator_deprecations( |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function (extract-method)
Assisted by: Gemini, Claude [CLOUDDST-28644]
Assisted by: Gemini, Claude [CLOUDDST-28644]
lipoja
force-pushed
the
containerized_fbc_operations
branch
from
ff9bc1b to
ca94b99
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Handling of fbc-operations for containerized IIB.
Assisted by: Gemini, ChatGPT, Claude
[CLOUDDST-28644]
Summary by Sourcery
Add support for containerized FBC operations driven by Konflux, including Git-based catalog updates, pipeline integration, and artifact handling for index database updates.
New Features:
Bug Fixes:
Enhancements:
Tests: