Skip to content

modify _opm_registry_add calls so opm does not hit error "reached max number of arguments" #1140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

modify _opm_registry_add calls so opm does not hit error "reached max number of arguments" #1140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions iib/workers/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class Config(object):
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
iib_no_ocp_label_allow_list: List[str] = []
Expand Down
19 changes: 11 additions & 8 deletions iib/workers/tasks/opm_operations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,21 +823,24 @@ def opm_registry_add_fbc(
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()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


index_db_file = _get_or_create_temp_index_db_file(
base_dir=base_dir,
from_index=from_index,
overwrite_from_index_token=overwrite_from_index_token,
ignore_existing=True,
)

_opm_registry_add(
base_dir=base_dir,
index_db=index_db_file,
bundles=bundles,
overwrite_csv=overwrite_csv,
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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The we would need to call abs whenever he uses this constant.
My questions are:

Is it necessary to have this check? If yes, can we do it on one place?

_opm_registry_add(
base_dir=base_dir,
index_db=index_db_file,
bundles=bundles[i : i + conf.iib_max_number_of_bundles_as_cmd_argument],
overwrite_csv=overwrite_csv,
container_tool=container_tool,
graph_update_mode=graph_update_mode,
)

fbc_dir, _ = opm_migrate(index_db=index_db_file, base_dir=base_dir)
# we should keep generating Dockerfile here
Expand Down
32 changes: 24 additions & 8 deletions tests/test_workers/test_tasks/test_opm_operations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import socket

from unittest import mock
from unittest.mock import call

from iib.exceptions import IIBError, AddressAlreadyInUse
from iib.workers.config import get_worker_config
Expand Down Expand Up @@ -36,6 +37,7 @@ def mock_config():
mock_config.iib_grpc_max_port_tries = 3
mock_config.iib_grpc_max_tries = 3
mock_config.iib_deprecate_bundles_limit = 5
mock_config.iib_max_number_of_bundles_as_cmd_argument = 1
mc.return_value = mock_config
yield mc

Expand Down Expand Up @@ -462,6 +464,7 @@ def test_opm_registry_add(
@pytest.mark.parametrize('overwrite_csv', (True, False))
@pytest.mark.parametrize('container_tool', (None, 'podwoman'))
@pytest.mark.parametrize('graph_update_mode', (None, 'semver-skippatch'))
@mock.patch('iib.workers.tasks.opm_operations._get_or_create_temp_index_db_file')
@mock.patch('iib.workers.tasks.opm_operations.create_dockerfile')
@mock.patch('iib.workers.tasks.opm_operations.opm_migrate')
@mock.patch('iib.workers.tasks.opm_operations._opm_registry_add')
Expand All @@ -475,13 +478,15 @@ def test_opm_registry_add_fbc(
mock_ora,
mock_om,
mock_ogd,
mock_get_db_file,
from_index,
bundles,
overwrite_csv,
container_tool,
graph_update_mode,
is_fbc,
tmpdir,
mock_config,
):
index_db_file = os.path.join(tmpdir, 'database/index.db')
fbc_dir = os.path.join(tmpdir, 'catalogs')
Expand All @@ -491,6 +496,9 @@ def test_opm_registry_add_fbc(
mock_om.return_value = (fbc_dir, cache_dir)
mock_iifbc.return_value = is_fbc

index_db_file = os.path.join(tmpdir, 'database/index.db')
mock_get_db_file.return_value = index_db_file

opm_operations.opm_registry_add_fbc(
base_dir=tmpdir,
bundles=bundles,
Expand All @@ -500,15 +508,23 @@ def test_opm_registry_add_fbc(
overwrite_csv=overwrite_csv,
container_tool=container_tool,
)
max_bundles = mock_config.return_value.iib_max_number_of_bundles_as_cmd_argument

mock_ora.assert_called_once_with(
base_dir=tmpdir,
index_db=index_db_file,
bundles=bundles,
overwrite_csv=overwrite_csv,
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(
Comment on lines +515 to +517
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice suggestion, ptal @ashwgit

call(
base_dir=tmpdir,
index_db=index_db_file,
bundles=chunk,
overwrite_csv=overwrite_csv,
Comment on lines +513 to +522
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one as well please @ashwgit

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)
mock_ogd.assert_called_once_with(
Expand Down