modify _opm_registry_add calls so opm does not hit error "reached max number of arguments" #1140
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 GuidePrevent opm “reached max number of arguments” errors by introducing a configurable bundle batch size and splitting registry additions into chunks in opm_registry_add_fbc. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ashwgit
force-pushed
the
modify_opm_registry_add_calls
branch
from
86bb0cb to
7607de9
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/opm_operations.py:826` </location>
<code_context>
The format of the token must be in the format "user:password".
:param str container_tool: the container tool to be used to operate on the index image
"""
+ conf = get_worker_config()
+
index_db_file = _get_or_create_temp_index_db_file(
</code_context>
<issue_to_address>
Consider validating conf.iib_max_number_of_bundles_as_cmd_argument for non-positive values.
A positive value check will help avoid runtime errors from invalid loop ranges.
</issue_to_address>
### Comment 2
<location> `iib/workers/config.py:49` </location>
<code_context>
iib_index_configs_gitlab_tokens_map: Optional[Dict[str, Dict[str, str]]] = None
iib_log_level: str = 'INFO'
iib_deprecate_bundles_limit = 200
+ iib_max_number_of_bundles_as_cmd_argument = 500
iib_max_recursive_related_bundles = 15
# list of index images to which we can add bundles without "com.redhat.openshift.versions" label
</code_context>
<issue_to_address>
Default value for iib_max_number_of_bundles_as_cmd_argument may need documentation or justification.
Consider adding a brief comment or reference explaining why 500 was chosen, particularly if it's based on system or tool constraints.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
iib_deprecate_bundles_limit = 200
iib_max_number_of_bundles_as_cmd_argument = 500
iib_max_recursive_related_bundles = 15
=======
iib_deprecate_bundles_limit = 200
# Maximum number of bundles allowed as a command argument (500) is chosen to avoid exceeding shell argument length limits and to ensure compatibility with underlying system tools.
iib_max_number_of_bundles_as_cmd_argument = 500
iib_max_recursive_related_bundles = 15
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/test_workers/test_tasks/test_opm_operations.py:513` </location>
<code_context>
- container_tool=container_tool,
- graph_update_mode=graph_update_mode,
- )
+ if bundles:
+ expected_calls = []
+ for i in range(0, len(bundles), max_bundles):
+ chunk = bundles[i : i + max_bundles]
+ expected_calls.append(
+ call(
+ base_dir=tmpdir,
+ index_db=index_db_file,
+ bundles=chunk,
+ overwrite_csv=overwrite_csv,
+ container_tool=container_tool,
+ graph_update_mode=graph_update_mode,
+ )
+ )
+ mock_ora.assert_has_calls(expected_calls)
mock_om.assert_called_once_with(index_db=index_db_file, base_dir=tmpdir)
</code_context>
<issue_to_address>
Missing test for empty bundles list edge case.
Add a test with an empty 'bundles' list to confirm '_opm_registry_add' is not called and the function handles this case correctly.
Suggested implementation:
```python
max_bundles = mock_config.return_value.iib_max_number_of_bundles_as_cmd_argument
if bundles:
expected_calls = []
for i in range(0, len(bundles), max_bundles):
chunk = bundles[i : i + max_bundles]
expected_calls.append(
call(
base_dir=tmpdir,
index_db=index_db_file,
bundles=chunk,
overwrite_csv=overwrite_csv,
container_tool=container_tool,
graph_update_mode=graph_update_mode,
)
)
mock_ora.assert_has_calls(expected_calls)
else:
mock_ora.assert_not_called()
mock_om.assert_called_once_with(index_db=index_db_file, base_dir=tmpdir)
mock_ogd.assert_called_once_with(
```
If this logic is inside a test function, you should also add a dedicated test function for the empty bundles case, e.g. `def test_opm_operations_empty_bundles(...)`. In that function, set `bundles = []` and verify the mocks as above.
</issue_to_address>
### Comment 4
<location> `tests/test_workers/test_tasks/test_opm_operations.py:515` </location>
<code_context>
- )
+ if bundles:
+ expected_calls = []
+ for i in range(0, len(bundles), max_bundles):
+ chunk = bundles[i : i + max_bundles]
+ expected_calls.append(
+ call(
+ base_dir=tmpdir,
</code_context>
<issue_to_address>
Missing test for batch size greater than bundles length.
Add a test case where the number of bundles is less than the batch size to verify that batching does not occur and only one '_opm_registry_add' call is made.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| The format of the token must be in the format "user:password". | ||
| :param str container_tool: the container tool to be used to operate on the index image | ||
| """ | ||
| conf = get_worker_config() |
There was a problem hiding this comment.
suggestion (bug_risk): Consider validating conf.iib_max_number_of_bundles_as_cmd_argument for non-positive values.
A positive value check will help avoid runtime errors from invalid loop ranges.
Comment on lines
+513
to
+522
| if bundles: | ||
| expected_calls = [] | ||
| for i in range(0, len(bundles), max_bundles): | ||
| chunk = bundles[i : i + max_bundles] | ||
| expected_calls.append( | ||
| call( | ||
| base_dir=tmpdir, | ||
| index_db=index_db_file, | ||
| bundles=chunk, | ||
| overwrite_csv=overwrite_csv, |
There was a problem hiding this comment.
suggestion (testing): Missing test for empty bundles list edge case.
Add a test with an empty 'bundles' list to confirm '_opm_registry_add' is not called and the function handles this case correctly.
Suggested implementation:
max_bundles = mock_config.return_value.iib_max_number_of_bundles_as_cmd_argument
if bundles:
expected_calls = []
for i in range(0, len(bundles), max_bundles):
chunk = bundles[i : i + max_bundles]
expected_calls.append(
call(
base_dir=tmpdir,
index_db=index_db_file,
bundles=chunk,
overwrite_csv=overwrite_csv,
container_tool=container_tool,
graph_update_mode=graph_update_mode,
)
)
mock_ora.assert_has_calls(expected_calls)
else:
mock_ora.assert_not_called()
mock_om.assert_called_once_with(index_db=index_db_file, base_dir=tmpdir)
mock_ogd.assert_called_once_with(If this logic is inside a test function, you should also add a dedicated test function for the empty bundles case, e.g. def test_opm_operations_empty_bundles(...). In that function, set bundles = [] and verify the mocks as above.
There was a problem hiding this comment.
This one as well please @ashwgit
Comment on lines
+515
to
+517
| for i in range(0, len(bundles), max_bundles): | ||
| chunk = bundles[i : i + max_bundles] | ||
| expected_calls.append( |
There was a problem hiding this comment.
suggestion (testing): Missing test for batch size greater than bundles length.
Add a test case where the number of bundles is less than the batch size to verify that batching does not occur and only one '_opm_registry_add' call is made.
There was a problem hiding this comment.
Nice suggestion, ptal @ashwgit
JAVGan
requested changes
Sep 4, 2025
| container_tool=container_tool, | ||
| graph_update_mode=graph_update_mode, | ||
| ) | ||
| for i in range(0, len(bundles), conf.iib_max_number_of_bundles_as_cmd_argument): |
There was a problem hiding this comment.
Taking advantage of the comment from sourcery-ai to avoid negative numbrers, you might even use abs for it:
Suggested change
| for i in range(0, len(bundles), conf.iib_max_number_of_bundles_as_cmd_argument): | |
| for i in range(0, len(bundles), abs(conf.iib_max_number_of_bundles_as_cmd_argument)): |
Comment on lines
+515
to
+517
| for i in range(0, len(bundles), max_bundles): | ||
| chunk = bundles[i : i + max_bundles] | ||
| expected_calls.append( |
There was a problem hiding this comment.
Nice suggestion, ptal @ashwgit
Comment on lines
+513
to
+522
| if bundles: | ||
| expected_calls = [] | ||
| for i in range(0, len(bundles), max_bundles): | ||
| chunk = bundles[i : i + max_bundles] | ||
| expected_calls.append( | ||
| call( | ||
| base_dir=tmpdir, | ||
| index_db=index_db_file, | ||
| bundles=chunk, | ||
| overwrite_csv=overwrite_csv, |
There was a problem hiding this comment.
This one as well please @ashwgit
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
These modification changes the behavior the way _opm_registry_add function is being called inside opm_registry_add_fbc , by calling _opm_registry_add multiple times with chunks of bundles it prevents opm command execution error which happens when a large number of bundles is passed as arguments.
@chandwanitulsi @yashvardhannanavati @xDaile @JAVGan @lipoja PTAL
Summary by Sourcery
Prevent opm command execution errors by limiting the number of bundle arguments per call and processing bundles in configurable chunks
Enhancements:
Tests: