Add a containerized version of the ADD API endpoint #1273
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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lipoja
force-pushed
the
containerized_add
branch
2 times, most recently
from
517384c to
f1735c2
Compare
Assisted-by: JetBrains AI/Gemini [CLOUDDST-28643]
lipoja
force-pushed
the
containerized_add
branch
2 times, most recently
from
9d5cd98 to
15b2ef9
Compare
Contributor
Author
|
/reset |
lipoja
force-pushed
the
containerized_add
branch
from
15b2ef9 to
d110931
Compare
[CLOUDDST-28643]
lipoja
force-pushed
the
containerized_add
branch
from
d110931 to
7456021
Compare
Reviewer's GuideImplements a new containerized add-bundles workflow and wires the API to use it, including a new Celery task that drives a GitOps/Konflux-based index rebuild, plus supporting opm/FBC helpers and tests. Sequence diagram for the new containerized add-bundles workflowsequenceDiagram
actor User
participant API as API_add_bundles
participant Celery as Celery_Broker
participant Task as handle_containerized_add_request
participant Registry as Container_Registry
participant Git as Git_Repository
participant Konflux as Konflux_Pipeline
User->>API: HTTP POST /api/v1/add
API->>Celery: enqueue handle_containerized_add_request(args)
Celery->>Task: start task
Task->>Task: reset_docker_config
Task->>Registry: set_registry_token(overwrite_from_index_token, from_index)
Task->>Registry: get_resolved_bundles(bundles)
Task->>Task: verify_labels(resolved_bundles)
Task->>Registry: inspect_related_images (optional)
Task->>Task: prebuild_info = prepare_request_for_build(...)
Task->>Task: Opm.set_opm_version(from_index_resolved)
Task->>Task: _update_index_image_build_state(request_id, prebuild_info)
Task->>Git: prepare_git_repository_for_build(request_id, from_index, branch)
Git-->>Task: index_git_repo, local_git_repo_path, localized_git_catalog_path
Task->>Registry: fetch_and_verify_index_db_artifact(from_index)
Registry-->>Task: artifact_index_db_file
alt from_index is set
Task->>Registry: _get_present_bundles(localized_git_catalog_path, temp_dir)
Registry-->>Task: present_bundles, present_bundles_pull_spec
Task->>Task: filtered_bundles = _get_missing_bundles(...)
Task->>Task: resolved_bundles = filtered_bundles
end
Task->>Task: _opm_registry_add(index_db, resolved_bundles)
Task->>Task: deprecation_bundles = get_bundles_from_deprecation_list(...)
alt deprecation_bundles not empty
Task->>Task: deprecate_bundles_fbc_containerized(...)
end
Task->>Task: catalog_from_db = opm_migrate(index_db)
Task->>Task: remove deprecated bundles from localized_git_catalog_path
Task->>Task: merge_catalogs_dirs(catalog_from_db, localized_git_catalog_path)
Task->>Task: chmod_recursively(temp_dir)
Task->>Git: write_build_metadata(...)
Task->>Git: git_commit_and_create_mr_or_push(...)
Git-->>Task: mr_details, last_commit_sha
Task->>Konflux: monitor_pipeline_and_extract_image(last_commit_sha)
Konflux-->>Task: image_url
Task->>Registry: replicate_image_to_tagged_destinations(image_url, build_tags)
Registry-->>Task: output_pull_specs
Task->>Task: _update_index_image_pull_spec(output_pull_spec, ...)
Task->>Registry: push_index_db_artifact(index_db_path, operators, overwrite_from_index)
Registry-->>Task: original_index_db_digest
Task->>Git: cleanup_merge_request_if_exists(mr_details, index_git_repo)
Task->>API: set_request_state(request_id, complete)
API-->>User: 200 OK (request completed)
opt on_failure
Task->>Git: cleanup_on_failure(...)
Task->>API: set_request_state(request_id, failed)
API-->>User: error status
end
Class diagram for new containerized add task and helpersclassDiagram
class build_containerized_add {
<<module>>
+handle_containerized_add_request(bundles, request_id, binary_image, from_index, add_arches, cnr_token, organization, force_backport, overwrite_from_index, overwrite_from_index_token, distribution_scope, greenwave_config, binary_image_config, deprecation_list, build_tags, graph_update_mode, check_related_images, index_to_gitlab_push_map, username, traceparent) void
}
class opm_operations {
<<module>>
+deprecate_bundles_fbc_containerized(bundles, base_dir, binary_image, index_db_file) void
+opm_migrate(index_db, base_dir, generate_cache) tuple
+_opm_registry_add(base_dir, index_db, bundles, overwrite_csv, graph_update_mode) void
+Opm
}
class Opm {
+opm_version str
+set_opm_version(from_index) void
}
class build_module {
<<module>>
+_get_present_bundles(input_data, base_dir) Tuple~List~BundleImage~~, List~str~~
+_get_missing_bundles(present_bundles, resolved_bundles) List~str~
+_update_index_image_pull_spec(output_pull_spec, request_id, arches, from_index, overwrite_from_index, overwrite_from_index_token, resolved_prebuild_from_index, add_or_rm, is_image_fbc, index_repo_map) void
+_update_index_image_build_state(request_id, prebuild_info) void
+inspect_related_images(resolved_bundles, request_id, replacement_registry) void
}
class containerized_utils {
<<module>>
+prepare_git_repository_for_build(request_id, from_index, temp_dir, branch, index_to_gitlab_push_map) tuple
+fetch_and_verify_index_db_artifact(from_index, temp_dir) str
+write_build_metadata(local_git_repo_path, opm_version, ocp_version, distribution_scope, binary_image_resolved, request_id) void
+git_commit_and_create_mr_or_push(request_id, local_git_repo_path, index_git_repo, branch, commit_message, overwrite_from_index) tuple
+monitor_pipeline_and_extract_image(request_id, last_commit_sha) str
+replicate_image_to_tagged_destinations(request_id, image_url, build_tags) List~str~
+push_index_db_artifact(request_id, from_index, index_db_path, operators, operators_in_db, overwrite_from_index, request_type) str
+cleanup_merge_request_if_exists(mr_details, index_git_repo) void
+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) void
}
class fbc_utils {
<<module>>
+merge_catalogs_dirs(src_catalog_dir, dest_catalog_dir) void
}
class utils_module {
<<module>>
+chmod_recursively(path, dir_mode, file_mode) void
+get_bundles_from_deprecation_list(all_bundles, deprecation_list) List~str~
+get_resolved_bundles(bundles) List~str~
+request_logger
+reset_docker_config() void
+set_registry_token(token, index, append) contextmanager
+RequestConfigAddRm
+get_image_label(pull_spec, label_name) str
+verify_labels(resolved_bundles) void
+prepare_request_for_build(request_id, request_config) dict
}
class api_v1 {
<<module>>
+add_bundles() Tuple~Response, int~
+add_rm_batch() Tuple~Response, int~
}
class CeleryApp {
+task
}
build_containerized_add --> CeleryApp : uses
build_containerized_add --> build_module : uses
build_containerized_add --> opm_operations : uses
build_containerized_add --> containerized_utils : uses
build_containerized_add --> fbc_utils : uses
build_containerized_add --> utils_module : uses
opm_operations --> Opm : defines
api_v1 --> build_containerized_add : enqueues_handle_containerized_add_request
build_module ..> BundleImage
build_containerized_add ..> GreenwaveConfig
utils_module ..> RequestConfigAddRm
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The index.db artifact is created inside the TemporaryDirectory context but
push_index_db_artifactis called after thewith tempfile.TemporaryDirectory(...)block; once the context exits the file is deleted, so you should either move the push into thewithblock or keep the temp directory alive until after the artifact is pushed. deprecate_bundles_fbc_containerizedacceptsbinary_imageand capturesfbc_dirfromopm_migratebut never uses either; consider removing these parameters/variables or wiring them into the implementation to avoid dead code and confusion about what this helper actually does.- In
handle_containerized_add_requestthe error raised on failure saysFailed to add FBC fragment, which doesn't match the function’s behavior/name and may be confusing when troubleshooting; it would be clearer to use a message specific to containerized add requests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The index.db artifact is created inside the TemporaryDirectory context but `push_index_db_artifact` is called after the `with tempfile.TemporaryDirectory(...)` block; once the context exits the file is deleted, so you should either move the push into the `with` block or keep the temp directory alive until after the artifact is pushed.
- `deprecate_bundles_fbc_containerized` accepts `binary_image` and captures `fbc_dir` from `opm_migrate` but never uses either; consider removing these parameters/variables or wiring them into the implementation to avoid dead code and confusion about what this helper actually does.
- In `handle_containerized_add_request` the error raised on failure says `Failed to add FBC fragment`, which doesn't match the function’s behavior/name and may be confusing when troubleshooting; it would be clearer to use a message specific to containerized add requests.
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/build_containerized_add.py:278-287` </location>
<code_context>
+ # Write build metadata to a file to be added with the commit
</code_context>
<issue_to_address>
**issue (bug_risk):** Artifacts and repo path are used after the TemporaryDirectory is cleaned up
The `tempfile.TemporaryDirectory` context ends before `write_build_metadata`, `git_commit_and_create_mr_or_push`, and `push_index_db_artifact` run, but `local_git_repo_path` and `artifact_index_db_file` live under `temp_dir`. By then the directory has been deleted, so Git and index.db operations will fail and the "still inside the context manager" comment is inaccurate. Please either move these calls inside the `with tempfile.TemporaryDirectory(...)` block or manage the temp directory lifecycle manually and clean it up after all operations complete.
</issue_to_address>
### Comment 2
<location> `iib/workers/tasks/opm_operations.py:503-512` </location>
<code_context>
+def deprecate_bundles_fbc_containerized(
</code_context>
<issue_to_address>
**suggestion:** Function parameters and return value are unused or misleading
In `deprecate_bundles_fbc_containerized`, `binary_image` is never used and the `fbc_dir` returned from `opm_migrate` is ignored. The docstring also describes creating a Dockerfile without building, but this function only updates the SQLite DB and calls `opm_migrate`. Please either remove or use the unused parameter/variable (and update callers as needed), and update the docstring to match the actual behavior.
Suggested implementation:
```python
def deprecate_bundles_fbc_containerized(
bundles: List[str],
base_dir: str,
index_db_file: str,
) -> None:
"""
Deprecate the specified bundles from the FBC index image in a containerized workflow.
This function updates the SQLite database for the index image and invokes
``opm migrate``; it does not build or push any container images.
```
1. Update all call sites of `deprecate_bundles_fbc_containerized` to remove the `binary_image` argument from the function call.
2. If the body of `deprecate_bundles_fbc_containerized` currently assigns the return value of `opm_migrate` to a variable such as `fbc_dir` and leaves it unused, either:
- drop the assignment entirely (e.g. just call `opm_migrate(...)`), or
- explicitly use the returned value if it is actually needed for later operations.
3. If there are references to `binary_image` in comments or docstrings elsewhere related to this function, update or remove them to match the new signature and behavior.
</issue_to_address>
### Comment 3
<location> `tests/test_workers/test_tasks/test_build_containerized_add.py:118-120` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 4
<location> `tests/test_workers/test_tasks/test_build_containerized_add.py:124-130` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 5
<location> `tests/test_workers/test_tasks/test_build_containerized_add.py:167-170` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 6
<location> `tests/test_workers/test_tasks/test_build_containerized_add.py:184-189` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 7
<location> `tests/test_workers/test_tasks/test_build_containerized_add.py:201-210` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 8
<location> `tests/test_workers/test_tasks/test_opm_operations.py:1426-1444` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 9
<location> `iib/workers/tasks/build_containerized_add.py:376` </location>
<code_context>
@app.task
@request_logger
@instrument_tracing(span_name="workers.tasks.handle_add_request", attributes=get_binary_versions())
def handle_containerized_add_request(
bundles: List[str],
request_id: int,
binary_image: Optional[str] = None,
from_index: Optional[str] = None,
add_arches: Optional[Set[str]] = None,
cnr_token: Optional[str] = None,
organization: Optional[str] = None,
force_backport: bool = False,
overwrite_from_index: bool = False,
overwrite_from_index_token: Optional[str] = None,
distribution_scope: Optional[str] = None,
greenwave_config: Optional[GreenwaveConfig] = None,
binary_image_config: Optional[Dict[str, Dict[str, str]]] = None,
deprecation_list: Optional[List[str]] = None,
build_tags: Optional[List[str]] = None,
graph_update_mode: Optional[str] = None,
check_related_images: bool = False,
index_to_gitlab_push_map: Optional[Dict[str, str]] = None,
username: Optional[str] = None,
traceparent: Optional[str] = None,
) -> None:
"""
Coordinate the the work needed to build the index image with the input bundles.
:param list bundles: a list of strings representing the pull specifications of the bundles to
add to the index image being built.
: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 str cnr_token: (deprecated) legacy support was disabled.
the token required to push backported packages to the legacy app registry via OMPS.
:param str organization: (deprecated) legacy support was disabled.
organization name in the legacy app registry to which the backported packages
should be pushed to.
:param bool force_backport: (deprecated) legacy support was disabled.
if True, always export packages to the legacy app registry via OMPS.
:param bool overwrite_from_index: if True, overwrite the input ``from_index`` with the built
index image.
:param str overwrite_from_index_token: the token used for overwriting the input
``from_index`` image. This is required to use ``overwrite_from_index``.
The format of the token must be in the format "user:password".
:param str distribution_scope: the scope for distribution of the index image, defaults to
``None``.
:param dict greenwave_config: the dict of config required to query Greenwave to gate bundles.
:param dict binary_image_config: the dict of config required to identify the appropriate
``binary_image`` to use.
:param list deprecation_list: list of deprecated bundles for the target index image. Defaults
to ``None``.
:param list build_tags: List of tags which will be applied to intermediate index images.
:param str graph_update_mode: Graph update mode that defines how channel graphs are updated
in the index.
: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 str traceparent: the traceparent header value to be used for tracing the request.
:raises IIBError: if the index image build fails.
"""
reset_docker_config()
# Resolve bundles to their digests
set_request_state(request_id, 'in_progress', 'Resolving the bundles')
with set_registry_token(overwrite_from_index_token, from_index, append=True):
resolved_bundles = get_resolved_bundles(bundles)
verify_labels(resolved_bundles)
if check_related_images:
inspect_related_images(
resolved_bundles,
request_id,
worker_config.iib_related_image_registry_replacement.get(username),
)
prebuild_info = prepare_request_for_build(
request_id,
RequestConfigAddRm(
_binary_image=binary_image,
from_index=from_index,
overwrite_from_index_token=overwrite_from_index_token,
add_arches=add_arches,
bundles=bundles,
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']
operators = list(prebuild_info['bundle_mapping'].keys())
index_to_gitlab_push_map = index_to_gitlab_push_map or {}
# Variables mr_details, last_commit_sha and original_index_db_digest
# needs to be assigned; otherwise cleanup_on_failure() fails when an exception is raised.
mr_details: Optional[Dict[str, str]] = None
last_commit_sha: Optional[str] = None
original_index_db_digest: Optional[str] = None
Opm.set_opm_version(from_index_resolved)
_update_index_image_build_state(request_id, prebuild_info)
present_bundles: List[BundleImage] = []
present_bundles_pull_spec: List[str] = []
with tempfile.TemporaryDirectory(prefix=f'iib-{request_id}-') as temp_dir:
branch = prebuild_info['ocp_version']
# Set up and clone Git repository
(
index_git_repo,
local_git_repo_path,
localized_git_catalog_path,
) = prepare_git_repository_for_build(
request_id=request_id,
from_index=str(from_index),
temp_dir=temp_dir,
branch=branch,
index_to_gitlab_push_map=index_to_gitlab_push_map,
)
# Pull index.db artifact (uses ImageStream cache if configured, otherwise pulls directly)
artifact_index_db_file = fetch_and_verify_index_db_artifact(
from_index=str(from_index),
temp_dir=temp_dir,
)
if from_index:
msg = 'Checking if bundles are already present in index image'
log.info(msg)
set_request_state(request_id, 'in_progress', msg)
with set_registry_token(overwrite_from_index_token, from_index_resolved, append=True):
present_bundles, present_bundles_pull_spec = _get_present_bundles(
localized_git_catalog_path, temp_dir
)
filtered_bundles = _get_missing_bundles(present_bundles, resolved_bundles)
excluded_bundles = [
bundle for bundle in resolved_bundles if bundle not in filtered_bundles
]
resolved_bundles = filtered_bundles
if excluded_bundles:
log.info(
'Following bundles are already present in the index image: %s',
' '.join(excluded_bundles),
)
# This is a replacement for opm_registry_add_fbc for a containerized version of IIB.
# Note: only index.db is modified (FBC directory is unchanged)
_opm_registry_add(
base_dir=temp_dir,
index_db=artifact_index_db_file,
bundles=resolved_bundles,
overwrite_csv=(prebuild_info['distribution_scope'] in ['dev', 'stage']),
graph_update_mode=graph_update_mode,
)
deprecation_bundles = get_bundles_from_deprecation_list(
present_bundles_pull_spec + resolved_bundles, deprecation_list or []
)
arches = prebuild_info['arches']
if deprecation_bundles:
deprecate_bundles_fbc_containerized(
bundles=deprecation_bundles,
base_dir=temp_dir,
binary_image=prebuild_info['binary_image'],
index_db_file=artifact_index_db_file,
)
os.makedirs(os.path.join(temp_dir, 'from_db'), exist_ok=True)
# get catalog from SQLite index.db (hidden db) - not opted in operators
catalog_from_db, _ = opm_migrate(
index_db=artifact_index_db_file,
base_dir=os.path.join(temp_dir, 'from_db'),
generate_cache=False,
)
catalog_from_index = localized_git_catalog_path
# we have to remove all `deprecation_bundles` from `catalog_from_index`
# before merging catalogs otherwise if catalog was deprecated and
# removed from `index.db` it stays on FBC (from_index)
# Therefore we have to remove the directory before merging
for deprecate_bundle_pull_spec in deprecation_bundles:
# remove deprecated operators from FBC stored in index image
deprecate_bundle = get_image_label(
deprecate_bundle_pull_spec, 'operators.operatorframework.io.bundle.package.v1'
)
bundle_from_index = os.path.join(catalog_from_index, deprecate_bundle)
if os.path.exists(bundle_from_index):
log.debug(
"Removing deprecated bundle from catalog before merging: %s",
deprecate_bundle,
)
shutil.rmtree(bundle_from_index)
# overwrite data in `catalog_from_index` by data from `catalog_from_db`
# this adds changes on not opted in operators to final
merge_catalogs_dirs(catalog_from_db, catalog_from_index)
# If the container-tool podman is used in the opm commands above, opm will create temporary
# files and directories without the write permission. This will cause the context manager
# to fail to delete these files. Adjust the file modes to avoid this error.
chmod_recursively(
temp_dir,
dir_mode=(stat.S_IRWXU | stat.S_IRWXG),
file_mode=(stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IWGRP),
)
# 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'],
str(distribution_scope),
binary_image_resolved,
request_id,
)
try:
# Commit changes and create MR or push directly
mr_details, last_commit_sha = git_commit_and_create_mr_or_push(
request_id=request_id,
local_git_repo_path=local_git_repo_path,
index_git_repo=index_git_repo,
branch=branch,
commit_message=(
f"IIB: Add bundles for request {request_id}\n\n" f"Bundles: {', '.join(bundles)}"
),
overwrite_from_index=overwrite_from_index,
)
# Wait for Konflux pipeline and extract built image URL
image_url = monitor_pipeline_and_extract_image(
request_id=request_id,
last_commit_sha=last_commit_sha,
)
# Copy built index to all output pull specs
output_pull_specs = replicate_image_to_tagged_destinations(
request_id=request_id,
image_url=image_url,
build_tags=build_tags,
)
# 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=str(from_index),
index_db_path=artifact_index_db_file,
operators=operators,
# we are adding operators, we do not check for their existence in the index.db
operators_in_db=set(),
overwrite_from_index=overwrite_from_index,
request_type='add',
)
# Close MR if it was opened
cleanup_merge_request_if_exists(mr_details, index_git_repo)
set_request_state(
request_id,
'complete',
'The operator bundle(s) were successfully added to 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=str(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>
**suggestion (code-quality):** We've found these issues:
- 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\_add\_request - 22% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
```suggestion
raise IIBError(f"Failed to add FBC fragment: {e}") from e
```
<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 10
<location> `tests/test_workers/test_tasks/test_build_containerized_add.py:45` </location>
<code_context>
@pytest.mark.parametrize('check_related_images', (True, False))
@pytest.mark.parametrize('with_deprecations', (True, False))
@pytest.mark.parametrize('from_index', ('index:latest', None))
@mock.patch('iib.workers.tasks.build_containerized_add.os.makedirs')
@mock.patch('iib.workers.tasks.build_containerized_add.cleanup_on_failure')
@mock.patch('iib.workers.tasks.build_containerized_add.set_request_state')
@mock.patch('iib.workers.tasks.build_containerized_add.cleanup_merge_request_if_exists')
@mock.patch('iib.workers.tasks.build_containerized_add.push_index_db_artifact')
@mock.patch('iib.workers.tasks.build_containerized_add._update_index_image_pull_spec')
@mock.patch('iib.workers.tasks.build_containerized_add.replicate_image_to_tagged_destinations')
@mock.patch('iib.workers.tasks.build_containerized_add.monitor_pipeline_and_extract_image')
@mock.patch('iib.workers.tasks.build_containerized_add.git_commit_and_create_mr_or_push')
@mock.patch('iib.workers.tasks.build_containerized_add.write_build_metadata')
@mock.patch('iib.workers.tasks.build_containerized_add.chmod_recursively')
@mock.patch('iib.workers.tasks.build_containerized_add.merge_catalogs_dirs')
@mock.patch('iib.workers.tasks.build_containerized_add.shutil.rmtree')
@mock.patch('iib.workers.tasks.build_containerized_add.os.path.exists')
@mock.patch('iib.workers.tasks.build_containerized_add.get_image_label')
@mock.patch('iib.workers.tasks.build_containerized_add.opm_migrate')
@mock.patch('iib.workers.tasks.build_containerized_add.deprecate_bundles_fbc_containerized')
@mock.patch('iib.workers.tasks.build_containerized_add.get_bundles_from_deprecation_list')
@mock.patch('iib.workers.tasks.build_containerized_add._opm_registry_add')
@mock.patch('iib.workers.tasks.build_containerized_add._get_missing_bundles')
@mock.patch('iib.workers.tasks.build_containerized_add._get_present_bundles')
@mock.patch('iib.workers.tasks.build_containerized_add.fetch_and_verify_index_db_artifact')
@mock.patch('iib.workers.tasks.build_containerized_add.prepare_git_repository_for_build')
@mock.patch('iib.workers.tasks.build_containerized_add.tempfile.TemporaryDirectory')
@mock.patch('iib.workers.tasks.build_containerized_add._update_index_image_build_state')
@mock.patch('iib.workers.tasks.build_containerized_add.Opm')
@mock.patch('iib.workers.tasks.build_containerized_add.prepare_request_for_build')
@mock.patch('iib.workers.tasks.build_containerized_add.inspect_related_images')
@mock.patch('iib.workers.tasks.build_containerized_add.verify_labels')
@mock.patch('iib.workers.tasks.build_containerized_add.get_resolved_bundles')
@mock.patch('iib.workers.tasks.build_containerized_add.set_registry_token')
@mock.patch('iib.workers.tasks.build_containerized_add.reset_docker_config')
def test_handle_containerized_add_request(
mock_reset_docker,
mock_set_token,
mock_get_resolved,
mock_verify_labels,
mock_inspect,
mock_prepare_req,
mock_opm,
mock_update_build_state,
mock_td,
mock_prepare_git,
mock_fetch_index_db,
mock_get_present,
mock_get_missing,
mock_opm_add,
mock_get_deprecations,
mock_deprecate,
mock_opm_migrate,
mock_get_image_label,
mock_os_exists,
mock_rmtree,
mock_merge,
mock_chmod,
mock_write_meta,
mock_git_commit,
mock_monitor,
mock_replicate,
mock_update_pull_spec,
mock_push_index_db,
mock_cleanup_mr,
mock_set_state,
mock_cleanup_failure,
mock_makedirs,
from_index,
with_deprecations,
check_related_images,
):
# Mock input data
bundles = ['some-bundle:latest']
request_id = 123
binary_image = 'binary-image:latest'
resolved_bundles = ['some-bundle@sha256:123456']
index_db_path = '/tmp/index.db'
temp_dir_path = '/tmp/iib-123-temp'
mock_get_resolved.return_value = resolved_bundles
mock_td.return_value.__enter__.return_value = temp_dir_path
# Mock prebuild info
prebuild_info = {
'from_index_resolved': 'from-index@sha256:abcdef',
'binary_image_resolved': 'binary-image@sha256:fedcba',
'arches': {'amd64'},
'bundle_mapping': {'some-operator': resolved_bundles},
'ocp_version': 'v4.12',
'distribution_scope': 'prod',
'binary_image': binary_image,
}
mock_prepare_req.return_value = prebuild_info
# Mock git preparation
index_git_repo = mock.Mock()
local_git_repo_path = '/tmp/git_repo'
localized_git_catalog_path = '/tmp/git_repo/catalog'
mock_prepare_git.return_value = (
index_git_repo,
local_git_repo_path,
localized_git_catalog_path,
)
mock_fetch_index_db.return_value = index_db_path
# Mock bundle presence checks
if from_index:
mock_get_present.return_value = ([], [])
mock_get_missing.return_value = resolved_bundles
# Mock deprecation handling
deprecation_list = ['deprecated-bundle:1.0'] if with_deprecations else None
if with_deprecations:
mock_get_deprecations.return_value = ['deprecated-bundle@sha256:old']
mock_get_image_label.return_value = 'deprecated-operator-package'
mock_os_exists.return_value = True
else:
mock_get_deprecations.return_value = []
mock_os_exists.return_value = False
# Mock OPM migration
catalog_from_db = '/tmp/from_db'
mock_opm_migrate.return_value = (catalog_from_db, None)
# Mock commit and push
mock_git_commit.return_value = ({'mr_id': 1}, 'commit_sha_123')
# Mock pipeline monitoring
image_url = 'registry.example.com/output-image:tag'
mock_monitor.return_value = image_url
# Mock replication
output_pull_specs = ['registry.example.com/final-image:123']
mock_replicate.return_value = output_pull_specs
# Mock final artifact push
mock_push_index_db.return_value = 'sha256:index_db_digest'
# Call the function
build_containerized_add.handle_containerized_add_request(
bundles=bundles,
request_id=request_id,
binary_image=binary_image,
from_index=from_index,
check_related_images=check_related_images,
deprecation_list=deprecation_list,
overwrite_from_index_token="user:pass",
)
# Verifications
mock_reset_docker.assert_called_once()
mock_set_state.assert_called()
mock_get_resolved.assert_called_once_with(bundles)
mock_verify_labels.assert_called_once_with(resolved_bundles)
if check_related_images:
mock_inspect.assert_called_once()
else:
mock_inspect.assert_not_called()
mock_prepare_req.assert_called_once()
# Verify git preparation
mock_prepare_git.assert_called_once_with(
request_id=request_id,
from_index=str(from_index),
temp_dir=temp_dir_path,
branch='v4.12',
index_to_gitlab_push_map={},
)
# Verify bundle checks if from_index is provided
if from_index:
mock_get_present.assert_called_once()
mock_get_missing.assert_called_once()
else:
mock_get_present.assert_not_called()
mock_get_missing.assert_not_called()
# Verify OPM operations
mock_opm_add.assert_called_once_with(
base_dir=temp_dir_path,
index_db=index_db_path,
bundles=resolved_bundles,
overwrite_csv=False,
graph_update_mode=None,
)
# Verify deprecation handling
if with_deprecations:
mock_get_deprecations.assert_called_once()
mock_deprecate.assert_called_once()
mock_rmtree.assert_called()
# Verify we tried to remove the deprecated bundle from the catalog
expected_path = os.path.join(localized_git_catalog_path, 'deprecated-operator-package')
mock_rmtree.assert_any_call(expected_path)
else:
mock_deprecate.assert_not_called()
mock_rmtree.assert_not_called()
# Verify makedirs was called for from_db
mock_makedirs.assert_called_with(os.path.join(temp_dir_path, 'from_db'), exist_ok=True)
mock_opm_migrate.assert_called_once_with(
index_db=index_db_path,
base_dir=os.path.join(temp_dir_path, 'from_db'),
generate_cache=False,
)
mock_merge.assert_called_once_with(catalog_from_db, localized_git_catalog_path)
mock_chmod.assert_called_once()
mock_write_meta.assert_called_once()
mock_git_commit.assert_called_once()
mock_monitor.assert_called_once_with(request_id=request_id, last_commit_sha='commit_sha_123')
mock_replicate.assert_called_once()
mock_update_pull_spec.assert_called_once_with(
output_pull_spec=output_pull_specs[0],
request_id=request_id,
arches={'amd64'},
from_index=from_index,
overwrite_from_index=False,
overwrite_from_index_token="user:pass",
resolved_prebuild_from_index='from-index@sha256:abcdef',
add_or_rm=True,
is_image_fbc=True,
index_repo_map={},
)
mock_push_index_db.assert_called_once()
mock_cleanup_mr.assert_called_once()
mock_cleanup_failure.assert_not_called()
</code_context>
<issue_to_address>
**issue (code-quality):** Low code quality found in test\_handle\_containerized\_add\_request - 24% ([`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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+278
to
+287
| # 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'], | ||
| str(distribution_scope), | ||
| binary_image_resolved, | ||
| request_id, | ||
| ) |
There was a problem hiding this comment.
issue (bug_risk): Artifacts and repo path are used after the TemporaryDirectory is cleaned up
The tempfile.TemporaryDirectory context ends before write_build_metadata, git_commit_and_create_mr_or_push, and push_index_db_artifact run, but local_git_repo_path and artifact_index_db_file live under temp_dir. By then the directory has been deleted, so Git and index.db operations will fail and the "still inside the context manager" comment is inaccurate. Please either move these calls inside the with tempfile.TemporaryDirectory(...) block or manage the temp directory lifecycle manually and clean it up after all operations complete.
Collaborator
There was a problem hiding this comment.
@lipoja sourcery is right. All of the code should be in the tempfile.TemporaryDirectory context
Comment on lines
+503
to
+512
| def deprecate_bundles_fbc_containerized( | ||
| bundles: List[str], | ||
| base_dir: str, | ||
| binary_image: str, | ||
| index_db_file: str, | ||
| ) -> None: | ||
| """ | ||
| Deprecate the specified bundles from the FBC index image. | ||
| Dockerfile is created only, no build is performed. |
There was a problem hiding this comment.
suggestion: Function parameters and return value are unused or misleading
In deprecate_bundles_fbc_containerized, binary_image is never used and the fbc_dir returned from opm_migrate is ignored. The docstring also describes creating a Dockerfile without building, but this function only updates the SQLite DB and calls opm_migrate. Please either remove or use the unused parameter/variable (and update callers as needed), and update the docstring to match the actual behavior.
Suggested implementation:
def deprecate_bundles_fbc_containerized(
bundles: List[str],
base_dir: str,
index_db_file: str,
) -> None:
"""
Deprecate the specified bundles from the FBC index image in a containerized workflow.
This function updates the SQLite database for the index image and invokes
``opm migrate``; it does not build or push any container images.- Update all call sites of
deprecate_bundles_fbc_containerizedto remove thebinary_imageargument from the function call. - If the body of
deprecate_bundles_fbc_containerizedcurrently assigns the return value ofopm_migrateto a variable such asfbc_dirand leaves it unused, either:- drop the assignment entirely (e.g. just call
opm_migrate(...)), or - explicitly use the returned value if it is actually needed for later operations.
- drop the assignment entirely (e.g. just call
- If there are references to
binary_imagein comments or docstrings elsewhere related to this function, update or remove them to match the new signature and behavior.
yashvardhannanavati
requested changes
Dec 5, 2025
| from_index: Optional[str] = None, | ||
| add_arches: Optional[Set[str]] = None, | ||
| cnr_token: Optional[str] = None, | ||
| organization: Optional[str] = None, |
Collaborator
There was a problem hiding this comment.
can we get rid of lines 72-75 as we don't use them in the handler anymore?
| overwrite_from_index: bool = False, | ||
| overwrite_from_index_token: Optional[str] = None, | ||
| distribution_scope: Optional[str] = None, | ||
| greenwave_config: Optional[GreenwaveConfig] = None, |
Collaborator
There was a problem hiding this comment.
same with this, it's not required in the new pipeline and was moved to ET in the old pipeline.
| check_related_images: bool = False, | ||
| index_to_gitlab_push_map: Optional[Dict[str, str]] = None, | ||
| username: Optional[str] = None, | ||
| traceparent: Optional[str] = None, |
Collaborator
There was a problem hiding this comment.
this isn't used anywhere in the code either. am I missing something?
| set_request_state(request_id, 'in_progress', msg) | ||
|
|
||
| with set_registry_token(overwrite_from_index_token, from_index_resolved, append=True): | ||
| present_bundles, present_bundles_pull_spec = _get_present_bundles( |
Collaborator
There was a problem hiding this comment.
Consider this, _get_present_bundles calls get_list_bundles which runs opm render. You are passing a rendered catalog as param. Maybe extract part of get_list_bundles in a new helper function that is only called if the operator package name is present in localized_git_catalog_path. Also, only pass the catalog.json for the package in question to that helper function. That way, the computation will be much less. thoughts?
Something like
for operator_package in operators:
operator_dir = Path(path_string) / operator_package
if operator_dir.is_dir():
call_helper_function
| conf = get_worker_config() | ||
|
|
||
| # Break the bundles into chunks of at max iib_deprecate_bundles_limit bundles | ||
| for i in range( |
Collaborator
There was a problem hiding this comment.
This is identical to line 555 - 566 in opm_operations.py in deprecate_bundles_fbc, consider replacing that part with a call to this function?
Also consider renaming one of the functions? If you do not want to rename them (as the old code won't be used for much), add a brief comment in deprecate_bundles_fbc
| index_db=index_db_file, | ||
| bundles=bundles[i : i + conf.iib_deprecate_bundles_limit], # Pass a chunk starting at i | ||
| ) | ||
| fbc_dir, _ = opm_migrate(index_db_file, base_dir) |
Collaborator
There was a problem hiding this comment.
Why are we migrating index.db here? It's not being used anywhere
| generate_cache=False, | ||
| ) | ||
|
|
||
| catalog_from_index = localized_git_catalog_path |
Collaborator
There was a problem hiding this comment.
Is this assignment required? Can it stay localized_git_catalog_path?
| # Therefore we have to remove the directory before merging | ||
| for deprecate_bundle_pull_spec in deprecation_bundles: | ||
| # remove deprecated operators from FBC stored in index image | ||
| deprecate_bundle = get_image_label( |
Collaborator
There was a problem hiding this comment.
Consider renaming deprecate_bundle to deprecate_bundle_package
Consider using Path. It's seems to be a best practice. We should also change it in other PRs.
deperecate_bundle_package_dir = Path(path_string) / deperecate_bundle_package
if deperecate_bundle_package_dir.is_dir():
shutil.rmtree(bundle_from_index)
|
|
||
| # 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( |
Collaborator
There was a problem hiding this comment.
It just struck me that we should consider writing arches to the build_metadata too. It might be helpful for use cases like v4.12 where we only want to build for 2 arches given OCP has stopped building for s390x and ppc64le
Collaborator
There was a problem hiding this comment.
Commit to change teh function signature : 5d59cd7
Comment on lines
+278
to
+287
| # 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'], | ||
| str(distribution_scope), | ||
| binary_image_resolved, | ||
| request_id, | ||
| ) |
Collaborator
There was a problem hiding this comment.
@lipoja sourcery is right. All of the code should be in the tempfile.TemporaryDirectory context
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by Sourcery
Introduce a containerized implementation of the add-bundles workflow and route API v1 add requests through it while wiring it into the worker configuration and supporting OPM operations on FBC indexes.
New Features:
Enhancements:
Tests: