merge_index_image: latest bundles for deprecation #1180
Merged
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 GuideIntroduce semantic version sorting for deprecation bundles by adding a new sort_bundles_by_semver utility and integrating it into the merge index workflow, bump dependency versions (including semver), and update tests to cover and mock the new sorting logic. Sequence diagram for semver-based sorting of deprecation bundles in handle_merge_requestsequenceDiagram
participant H as "handle_merge_request()"
participant S as "sort_bundles_by_semver()"
participant O as "opm deprecatetruncate"
H->>S: Call sort_bundles_by_semver(deprecation_bundles, all_bundles)
S-->>H: Return sorted deprecation_bundles
H->>O: Pass sorted deprecation_bundles to opm deprecatetruncate
Class diagram for sort_bundles_by_semver utility and BundleImage usageclassDiagram
class BundleImage {
+bundlePath: str
+version: str
}
class IIBError {
}
class build_merge_index_image {
+sort_bundles_by_semver(bundles: List[str], bundle_images: List[BundleImage]) List[str]
}
build_merge_index_image ..> BundleImage : uses
build_merge_index_image ..> IIBError : raises
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- Consider splitting the new semver sorting feature and the large dependency/version bumps into separate PRs for easier review and rollback.
- Refactor sort_bundles_by_semver to build a dict mapping bundlePath to version instead of filtering the full list for better clarity and performance.
- Add a unit test covering the IIBError case in sort_bundles_by_semver to verify that missing bundle images are handled correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider splitting the new semver sorting feature and the large dependency/version bumps into separate PRs for easier review and rollback.
- Refactor sort_bundles_by_semver to build a dict mapping bundlePath to version instead of filtering the full list for better clarity and performance.
- Add a unit test covering the IIBError case in sort_bundles_by_semver to verify that missing bundle images are handled correctly.
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/build_merge_index_image.py:617-619` </location>
<code_context>
+ ]
+
+ if len(bundle_versions) != len(bundles):
+ uniq_verions = [x[1] for x in bundle_versions]
+ diff = set(bundles) - set(uniq_verions)
+ raise IIBError(f"Failed to sort bundles by semver, missing bundle images: {diff}")
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in variable name 'uniq_verions'.
Please rename 'uniq_verions' to 'uniq_versions' for accuracy.
```suggestion
uniq_versions = [x[1] for x in bundle_versions]
diff = set(bundles) - set(uniq_versions)
raise IIBError(f"Failed to sort bundles by semver, missing bundle images: {diff}")
```
</issue_to_address>
### Comment 2
<location> `tests/test_workers/test_tasks/test_build_merge_index_image.py:1076-1080` </location>
<code_context>
+ ),
+ ],
+)
+def test_sort_bundles_by_semver(bundle_images, expected) -> None:
+ bundles = [b['bundlePath'] for b in bundle_images]
+
+ res = build_merge_index_image.sort_bundles_by_semver(bundles, bundle_images)
+ assert res == expected
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test cases for error handling in sort_bundles_by_semver.
Add tests where `bundles` includes pullspecs missing from `bundle_images` to confirm that IIBError is raised.
</issue_to_address>
### Comment 3
<location> `tests/test_workers/test_tasks/test_build_merge_index_image.py:460-463` </location>
<code_context>
'target_index_resolved': target_index_resolved,
'distribution_scope': 'stage',
}
+ # Return the bundles list as it is for mock
+ mock_sortbundles.side_effect = lambda x, _: x
# returns different values to test different cases
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for empty bundles input to sort_bundles_by_semver.
Please add a test where both `bundles` and `bundle_images` are empty lists to verify the function returns an empty list without errors.
```suggestion
# Return the bundles list as it is for mock
mock_sortbundles.side_effect = lambda x, _: x
# Test: sort_bundles_by_semver with empty bundles and bundle_images
empty_bundles = []
empty_bundle_images = []
result = mock_sortbundles(empty_bundles, empty_bundle_images)
assert result == [], "Expected empty list when sorting empty bundles and bundle_images"
# returns different values to test different cases
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JAVGan
force-pushed
the
semver_sort_merge
branch
from
484a1f8 to
25fe4cf
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider moving sort_bundles_by_semver into a shared utility module rather than embedding it in build_merge_index_image to improve reusability and maintainability.
- Add error handling around semver.VersionInfo.parse in sort_bundles_by_semver to catch invalid version strings and wrap them in an IIBError for clearer feedback.
- In the handle_merge_request tests, assert not just that sort_bundles_by_semver is called but also that the deprecation bundles list is passed in the expected sorted order.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving sort_bundles_by_semver into a shared utility module rather than embedding it in build_merge_index_image to improve reusability and maintainability.
- Add error handling around semver.VersionInfo.parse in sort_bundles_by_semver to catch invalid version strings and wrap them in an IIBError for clearer feedback.
- In the handle_merge_request tests, assert not just that sort_bundles_by_semver is called but also that the deprecation bundles list is passed in the expected sorted order.
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/build_merge_index_image.py:616-618` </location>
<code_context>
+ ]
+
+ if len(bundle_versions) != len(bundles):
+ uniq_verions = [x[1] for x in bundle_versions]
+ diff = set(bundles) - set(uniq_verions)
+ raise IIBError(f"Failed to sort bundles by semver, missing bundle images: {diff}")
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in variable name 'uniq_verions'.
Please rename 'uniq_verions' to 'uniq_versions' for clarity.
```suggestion
uniq_versions = [x[1] for x in bundle_versions]
diff = set(bundles) - set(uniq_versions)
raise IIBError(f"Failed to sort bundles by semver, missing bundle images: {diff}")
```
</issue_to_address>
### Comment 2
<location> `tests/test_workers/test_tasks/test_build_merge_index_image.py:1030-1032` </location>
<code_context>
+ ([], []),
+ ],
+)
+def test_sort_bundles_by_semver(bundle_images, expected):
+ bundles = [b['bundlePath'] for b in bundle_images]
+ res = build_merge_index_image.sort_bundles_by_semver(bundles, bundle_images)
+ assert res == expected
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test case for bundles with invalid or non-semver version strings.
All current tests use valid semver strings. Please add a test with invalid or non-semver versions to ensure sort_bundles_by_semver handles these cases correctly.
```suggestion
([], []),
# Test case with invalid/non-semver version strings
(
[
{'bundlePath': 'foo-operator@sha256:1.0.0', "packageName": "foo-operator", "version": "1.0.0"},
{'bundlePath': 'bar-operator@sha256:xyz', "packageName": "bar-operator", "version": "xyz"},
{'bundlePath': 'baz-operator@sha256:', "packageName": "baz-operator", "version": ""},
{'bundlePath': 'qux-operator@sha256:2.0.0', "packageName": "qux-operator", "version": "2.0.0"},
],
[
'foo-operator@sha256:1.0.0',
'qux-operator@sha256:2.0.0',
'bar-operator@sha256:xyz',
'baz-operator@sha256:',
],
),
],
)
```
</issue_to_address>
### Comment 3
<location> `tests/test_workers/test_tasks/test_build_merge_index_image.py:188-189` </location>
<code_context>
assert mock_add_label_to_index.call_count == 2
mock_uiips.assert_called_once()
+ mock_sortbundles.assert_called_once()
mock_sov.assert_called_once_with(target_index_resolved)
mock_run_cmd.assert_called_once()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider verifying the actual sorted output in addition to mock invocation.
Verifying the sorted order of deprecation_bundles will ensure the test covers both the invocation and the correctness of the output, which is important if downstream logic depends on the order.
Suggested implementation:
```python
mock_sortbundles.assert_called_once()
# Verify the actual sorted output of deprecation_bundles
expected_sorted_bundles = [
# Fill in with the expected sorted bundle dicts, e.g.:
# {'bundlePath': 'bundleA:1.0', "packageName": "bundleA"},
# {'bundlePath': 'bundleB:2.0', "packageName": "bundleB"},
]
actual_sorted_bundles = mock_sortbundles.call_args[0][0]
assert actual_sorted_bundles == expected_sorted_bundles, "deprecation_bundles are not sorted as expected"
mock_sov.assert_called_once_with(target_index_resolved)
mock_run_cmd.assert_called_once()
```
You will need to fill in `expected_sorted_bundles` with the correct expected sorted output for your test case. This should match the order that your sorting logic produces.
</issue_to_address>
### Comment 4
<location> `tests/test_workers/test_tasks/test_build_merge_index_image.py:1069-1072` </location>
<code_context>
@pytest.mark.parametrize(
"bundles,bundle_images,diff_bundles",
[
(
["operator1@sha256:1234", "operator2@sha256:1000"],
[
{'bundlePath': 'operator1@sha256:1234', 'version': '1.2.34'},
],
set(["operator2@sha256:1000"]),
),
(
[
"missing-operator@sha256:1234",
"missing-operator@sha256:0001",
"operator1@sha256:1000",
],
[
{'bundlePath': 'operator1@sha256:1000', 'version': '1.0.0'},
{'bundlePath': 'operator1@sha256:2000', 'version': '2.0.0'},
{'bundlePath': 'operator1@sha256:3000', 'version': '3.0.0'},
],
set(
[
"missing-operator@sha256:1234",
"missing-operator@sha256:0001",
]
),
),
],
)
def test_sort_bundles_by_semver_missing_bundles(bundles, bundle_images, diff_bundles):
err = f"Failed to sort bundles by semver, missing bundle images: {diff_bundles}"
with pytest.raises(IIBError, match=err):
build_merge_index_image.sort_bundles_by_semver(bundles, bundle_images)
</code_context>
<issue_to_address>
**issue (code-quality):** Unwrap a constant iterable constructor [×2] ([`unwrap-iterable-construction`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/unwrap-iterable-construction/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JAVGan
force-pushed
the
semver_sort_merge
branch
3 times, most recently
from
4e0fb34 to
2b9440b
Compare
JAVGan
force-pushed
the
semver_sort_merge
branch
2 times, most recently
from
c9f593d to
fd81bba
Compare
Contributor
Author
|
@yashvardhannanavati please check the commit 084b653 If it looks good to you I might remove the previous one and change this PR's title and description to fit the new code |
JAVGan
force-pushed
the
semver_sort_merge
branch
2 times, most recently
from
084b653 to
58631fc
Compare
Collaborator
@JAVGan LGTM! A late note on requirements.txt, I was trying the same for the Kubernetes package only to realize it might have dependencies on other package versions that need to be upgraded. Maybe it makes sense to just have a second commit in the PR that upgrades all dependencies along with adding semver? |
Contributor
Only if it is required by |
Contributor
Author
|
@yashvardhannanavati @lipoja let's do the deps in a separate PR. As the CI is passing it shows that the current requirements are at least installable |
JAVGan
changed the title
JAVGan
force-pushed
the
semver_sort_merge
branch
from
58631fc to
ff42c08
Compare
This commit introduces a new function for `iib.workers.tasks.build_merge_index_image` named `get_bundles_latest_version` which receives a list of bundles pullspecs to and a list of bundle images and returns the pullspaces of the bundles in their latest versions. It's used on `handle_merge_request` to have the bundles latest versions by their `semver` version in order to avoid errors during the `opm deprecatetruncate` execution that might happen if minor version gets deprecated before a major one. By just passing the latest versions we are sure that the `opm deprecatetruncate` will deprecate all early versions alongside the latest one. Refers to CLOUDDST-28807 Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
JAVGan
force-pushed
the
semver_sort_merge
branch
from
ff42c08 to
eda5404
Compare
Contributor
Author
|
@yashvardhannanavati @lipoja I did squash the commits into a single one and edited the PR's description, please ack if you're ok with it. I'll open another PR once this gets merged for the deps bump |
yashvardhannanavati
approved these changes
Sep 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This commit introduces a new function for
iib.workers.tasks.build_merge_index_imagenamedget_bundles_latest_versionwhich receives a list of bundles pullspecs toand a list of bundle images and returns the pullspaces of the bundles
in their latest versions.
It's used on
handle_merge_requestto have the bundles latest versionsby their
semverversion in order to avoid errors during theopm deprecatetruncateexecution that might happen if minor versiongets deprecated before a major one. By just passing the latest versions
we are sure that the
opm deprecatetruncatewill deprecate all earlyversions alongside the latest one.