From 794d77e5584db9c99369853dfe6d7c93438b9591 Mon Sep 17 00:00:00 2001 From: whitfiea Date: Fri, 6 Mar 2026 08:44:10 +0000 Subject: [PATCH 1/6] [minor] Allow installplanapproval and startingcsv to be set on subscription --- src/mas/devops/olm.py | 13 ++-- src/mas/devops/templates/subscription.yml.j2 | 6 ++ test/src/test_olm.py | 64 ++++++++++++++++++++ 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/mas/devops/olm.py b/src/mas/devops/olm.py index d9351031..cd27c9fa 100644 --- a/src/mas/devops/olm.py +++ b/src/mas/devops/olm.py @@ -11,6 +11,7 @@ import logging from time import sleep from os import path +from typing import Optional from kubernetes.dynamic.exceptions import NotFoundError from openshift.dynamic import DynamicClient @@ -117,7 +118,7 @@ def getSubscription(dynClient: DynamicClient, namespace: str, packageName: str): return subscriptions.items[0] -def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str, packageChannel: str = None, catalogSource: str = None, catalogSourceNamespace: str = "openshift-marketplace", config: dict = None, installMode: str = "OwnNamespace"): +def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str, packageChannel: Optional[str] = None, catalogSource: Optional[str] = None, catalogSourceNamespace: str = "openshift-marketplace", config: Optional[dict] = None, installMode: str = "OwnNamespace", installPlanApproval: Optional[str] = None, startingCSV: Optional[str] = None): """ Create or update an operator subscription in a namespace. @@ -133,6 +134,8 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str catalogSourceNamespace (str, optional): Namespace of the catalog source. Defaults to "openshift-marketplace". config (dict, optional): Additional subscription configuration. Defaults to None. installMode (str, optional): Install mode for the OperatorGroup. Defaults to "OwnNamespace". + installPlanApproval (str, optional): Install plan approval mode ("Automatic" or "Manual"). Defaults to None. + startingCSV (str, optional): The specific CSV version to install. Defaults to None. Returns: Subscription: The created or updated subscription resource @@ -190,7 +193,9 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str package_name=packageName, package_channel=packageChannel, catalog_name=catalogSource, - catalog_namespace=catalogSourceNamespace + catalog_namespace=catalogSourceNamespace, + install_plan_approval=installPlanApproval, + starting_csv=startingCSV ) subscription = yaml.safe_load(renderedTemplate) subscriptionsAPI.apply(body=subscription, namespace=namespace) @@ -208,8 +213,8 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str raise OLMException(f"Found 0 InstallPlans for {packageName}") elif len(installPlanResources.items) > 1: logger.warning(f"More than 1 InstallPlan found for {packageName}") - else: - installPlanName = installPlanResources.items[0].metadata.name + + installPlanName = installPlanResources.items[0].metadata.name # Wait for InstallPlan to complete logger.debug(f"Waiting for InstallPlan {installPlanName}") diff --git a/src/mas/devops/templates/subscription.yml.j2 b/src/mas/devops/templates/subscription.yml.j2 index e9439ece..23bb3a66 100644 --- a/src/mas/devops/templates/subscription.yml.j2 +++ b/src/mas/devops/templates/subscription.yml.j2 @@ -9,6 +9,12 @@ spec: channel: {{ package_channel }} source: {{ catalog_name }} sourceNamespace: {{ catalog_namespace }} + {%- if install_plan_approval is not none %} + installPlanApproval: {{ install_plan_approval }} + {%- endif %} + {%- if starting_csv is not none %} + startingCSV: {{ starting_csv }} + {%- endif %} {%- if subscription_config is not none %} config: {{ subscription_config }} {%- endif %} diff --git a/test/src/test_olm.py b/test/src/test_olm.py index 0e148388..9fbf7990 100644 --- a/test/src/test_olm.py +++ b/test/src/test_olm.py @@ -79,3 +79,67 @@ def test_crud_with_config(): olm.deleteSubscription(dynClient, namespace, "ibm-sls") olm.deleteSubscription(dynClient, namespace, "ibm-truststore-mgr") ocp.deleteNamespace(dynClient, namespace) + + +def test_crud_with_manual_approval(): + namespace = "cli-fvt-3" + subscription = olm.applySubscription( + dynClient, + namespace, + "ibm-sls", + packageChannel="3.x", + installPlanApproval="Manual" + ) + assert subscription.metadata.name == "ibm-sls" + assert subscription.metadata.namespace == namespace + assert subscription.spec.installPlanApproval == "Manual" + + # When we install the ibm-sls subscription OLM will automatically create the ibm-truststore-mgr + # subscription, but when we delete the subscription, OLM will not automatically remove the latter + olm.deleteSubscription(dynClient, namespace, "ibm-sls") + olm.deleteSubscription(dynClient, namespace, "ibm-truststore-mgr") + ocp.deleteNamespace(dynClient, namespace) + + +def test_crud_with_starting_csv(): + namespace = "cli-fvt-4" + # Note: This test assumes a specific CSV version exists in the catalog + # You may need to adjust the version based on what's available + subscription = olm.applySubscription( + dynClient, + namespace, + "ibm-sls", + packageChannel="3.x", + startingCSV="ibm-sls.v3.8.0" + ) + assert subscription.metadata.name == "ibm-sls" + assert subscription.metadata.namespace == namespace + assert subscription.spec.startingCSV == "ibm-sls.v3.8.0" + + # When we install the ibm-sls subscription OLM will automatically create the ibm-truststore-mgr + # subscription, but when we delete the subscription, OLM will not automatically remove the latter + olm.deleteSubscription(dynClient, namespace, "ibm-sls") + olm.deleteSubscription(dynClient, namespace, "ibm-truststore-mgr") + ocp.deleteNamespace(dynClient, namespace) + + +def test_crud_with_manual_approval_and_starting_csv(): + namespace = "cli-fvt-5" + subscription = olm.applySubscription( + dynClient, + namespace, + "ibm-sls", + packageChannel="3.x", + installPlanApproval="Manual", + startingCSV="ibm-sls.v3.8.0" + ) + assert subscription.metadata.name == "ibm-sls" + assert subscription.metadata.namespace == namespace + assert subscription.spec.installPlanApproval == "Manual" + assert subscription.spec.startingCSV == "ibm-sls.v3.8.0" + + # When we install the ibm-sls subscription OLM will automatically create the ibm-truststore-mgr + # subscription, but when we delete the subscription, OLM will not automatically remove the latter + olm.deleteSubscription(dynClient, namespace, "ibm-sls") + olm.deleteSubscription(dynClient, namespace, "ibm-truststore-mgr") + ocp.deleteNamespace(dynClient, namespace) From 36f87aa0613d0cc247283bac58a51950489c3bda Mon Sep 17 00:00:00 2001 From: whitfiea Date: Fri, 6 Mar 2026 08:45:36 +0000 Subject: [PATCH 2/6] fix ing liniting --- src/mas/devops/olm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mas/devops/olm.py b/src/mas/devops/olm.py index cd27c9fa..fe944b11 100644 --- a/src/mas/devops/olm.py +++ b/src/mas/devops/olm.py @@ -213,7 +213,7 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str raise OLMException(f"Found 0 InstallPlans for {packageName}") elif len(installPlanResources.items) > 1: logger.warning(f"More than 1 InstallPlan found for {packageName}") - + installPlanName = installPlanResources.items[0].metadata.name # Wait for InstallPlan to complete From b793f0e7e641b84ad37e74359e54f2b046f54fb3 Mon Sep 17 00:00:00 2001 From: whitfiea Date: Fri, 6 Mar 2026 10:42:52 +0000 Subject: [PATCH 3/6] fixes for idempotentence --- src/mas/devops/olm.py | 113 +++++- test/src/test_olm.py | 18 + test/src/test_olm_installplan_selection.py | 421 +++++++++++++++++++++ 3 files changed, 542 insertions(+), 10 deletions(-) create mode 100644 test/src/test_olm_installplan_selection.py diff --git a/src/mas/devops/olm.py b/src/mas/devops/olm.py index fe944b11..1153aca6 100644 --- a/src/mas/devops/olm.py +++ b/src/mas/devops/olm.py @@ -125,6 +125,10 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str Automatically detects default channel and catalog source from PackageManifest if not provided. Ensures an OperatorGroup exists before creating the subscription. + When installPlanApproval is set to "Manual" and a startingCSV is specified, this function will + automatically approve the InstallPlan for the first-time installation to move to that startingCSV. + Subsequent upgrades will still require manual approval. + Parameters: dynClient (DynamicClient): OpenShift Dynamic Client namespace (str): The namespace to create the subscription in @@ -135,7 +139,8 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str config (dict, optional): Additional subscription configuration. Defaults to None. installMode (str, optional): Install mode for the OperatorGroup. Defaults to "OwnNamespace". installPlanApproval (str, optional): Install plan approval mode ("Automatic" or "Manual"). Defaults to None. - startingCSV (str, optional): The specific CSV version to install. Defaults to None. + startingCSV (str, optional): The specific CSV version to install. When combined with Manual approval, + the first InstallPlan to this CSV will be automatically approved. Defaults to None. Returns: Subscription: The created or updated subscription resource @@ -204,6 +209,7 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str logger.debug(f"Waiting for {packageName}.{namespace} InstallPlans") installPlanAPI = dynClient.resources.get(api_version="operators.coreos.com/v1alpha1", kind="InstallPlan") + # Use label selector to get InstallPlans (standard approach) installPlanResources = installPlanAPI.get(label_selector=labelSelector, namespace=namespace) while len(installPlanResources.items) == 0: installPlanResources = installPlanAPI.get(label_selector=labelSelector, namespace=namespace) @@ -212,17 +218,93 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str if len(installPlanResources.items) == 0: raise OLMException(f"Found 0 InstallPlans for {packageName}") elif len(installPlanResources.items) > 1: - logger.warning(f"More than 1 InstallPlan found for {packageName}") + logger.warning(f"More than 1 InstallPlan found for {packageName} using label selector") + + # Select the InstallPlan to use + installPlanResource = None + + # Special handling for Manual approval with startingCSV + if installPlanApproval == "Manual" and startingCSV is not None: + logger.debug(f"Manual approval with startingCSV {startingCSV} - checking if label selector returned correct InstallPlan") + + # Check if any of the InstallPlans from label selector match the startingCSV + for plan in installPlanResources.items: + csvNames = getattr(plan.spec, "clusterServiceVersionNames", []) + logger.debug(f"InstallPlan {plan.metadata.name} (from label selector) contains CSVs: {csvNames}") + if csvNames and startingCSV in csvNames: + installPlanResource = plan + logger.info(f"Found InstallPlan {plan.metadata.name} matching startingCSV {startingCSV} via label selector") + break + + # If no match found via label selector, search all InstallPlans owned by this subscription + if installPlanResource is None: + logger.warning(f"Label selector did not return InstallPlan matching startingCSV {startingCSV}") + logger.debug(f"Searching all InstallPlans in {namespace} owned by subscription {name}") + + allInstallPlans = installPlanAPI.get(namespace=namespace) + for plan in allInstallPlans.items: + # Check if this InstallPlan is owned by our subscription + owner_refs = getattr(plan.metadata, 'ownerReferences', []) + is_owned_by_subscription = any( + ref.kind == "Subscription" and ref.name == name + for ref in owner_refs + ) + + if is_owned_by_subscription: + csvNames = getattr(plan.spec, "clusterServiceVersionNames", []) + logger.debug(f"InstallPlan {plan.metadata.name} (owned by subscription) contains CSVs: {csvNames}") + if csvNames and startingCSV in csvNames: + installPlanResource = plan + logger.info(f"Found InstallPlan {plan.metadata.name} matching startingCSV {startingCSV} via subscription ownership") + break + + if installPlanResource is None: + logger.warning(f"No InstallPlan found matching startingCSV {startingCSV}, using first from label selector") + installPlanResource = installPlanResources.items[0] + else: + # Standard case: use first InstallPlan from label selector + installPlanResource = installPlanResources.items[0] - installPlanName = installPlanResources.items[0].metadata.name + installPlanName = installPlanResource.metadata.name + installPlanPhase = installPlanResource.status.phase - # Wait for InstallPlan to complete - logger.debug(f"Waiting for InstallPlan {installPlanName}") - installPlanPhase = installPlanResources.items[0].status.phase - while installPlanPhase != "Complete": - installPlanResource = installPlanAPI.get(name=installPlanName, namespace=namespace) - installPlanPhase = installPlanResource.status.phase - sleep(30) + # If the InstallPlan for our startingCSV is already Complete, we're done + if installPlanPhase == "Complete": + logger.info(f"InstallPlan {installPlanName} for {startingCSV} is already Complete") + else: + # Wait for InstallPlan to complete + logger.debug(f"Waiting for InstallPlan {installPlanName}") + + # Track if we've already approved this install plan + approved_manual_install = False + + while installPlanPhase != "Complete": + installPlanResource = installPlanAPI.get(name=installPlanName, namespace=namespace) + installPlanPhase = installPlanResource.status.phase + + # If InstallPlan requires approval and this is the first installation to startingCSV + if installPlanPhase == "RequiresApproval" and not approved_manual_install: + # Check if this is the first installation by verifying the CSV matches startingCSV + if startingCSV is not None: + csvName = getattr(installPlanResource.spec, "clusterServiceVersionNames", []) + if csvName and startingCSV in csvName: + logger.info(f"Approving InstallPlan {installPlanName} for first-time installation to {startingCSV}") + # Patch the InstallPlan to approve it + installPlanResource.spec.approved = True + installPlanAPI.patch( + body=installPlanResource, + name=installPlanName, + namespace=namespace, + content_type="application/merge-patch+json" + ) + approved_manual_install = True + logger.info(f"InstallPlan {installPlanName} approved successfully") + else: + logger.debug(f"InstallPlan CSV {csvName} does not match startingCSV {startingCSV}, waiting for manual approval") + else: + logger.debug(f"No startingCSV specified, InstallPlan {installPlanName} requires manual approval") + + sleep(30) # Wait for Subscription to complete logger.debug(f"Waiting for Subscription {name} in {namespace}") @@ -230,9 +312,20 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str subscriptionResource = subscriptionsAPI.get(name=name, namespace=namespace) state = getattr(subscriptionResource.status, "state", None) + # When manual approval is used with startingCSV, the state will be "UpgradePending" + # after the initial installation completes (indicating newer versions are available + # but require manual approval). For automatic approval, the state will be "AtLatestKnown". if state == "AtLatestKnown": logger.debug(f"Subscription {name} in {namespace} reached state: {state}") return subscriptionResource + elif state == "UpgradePending" and installPlanApproval == "Manual" and startingCSV is not None: + # Verify the installed CSV matches the startingCSV + installedCSV = getattr(subscriptionResource.status, "installedCSV", None) + if installedCSV == startingCSV: + logger.debug(f"Subscription {name} in {namespace} reached state: {state} with installedCSV: {installedCSV}") + return subscriptionResource + else: + logger.debug(f"Subscription {name} in {namespace} state is {state} but installedCSV ({installedCSV}) does not match startingCSV ({startingCSV}), retrying...") logger.debug(f"Subscription {name} in {namespace} not ready yet (state = {state}), retrying...") sleep(30) diff --git a/test/src/test_olm.py b/test/src/test_olm.py index 9fbf7990..0fbade94 100644 --- a/test/src/test_olm.py +++ b/test/src/test_olm.py @@ -124,6 +124,15 @@ def test_crud_with_starting_csv(): def test_crud_with_manual_approval_and_starting_csv(): + """ + Test that when installPlanApproval is Manual and startingCSV is specified, + the first InstallPlan is automatically approved to reach the startingCSV. + This allows the initial installation to proceed without manual intervention. + + Note: With Manual approval and startingCSV, the subscription state will be + "UpgradePending" after installation (indicating newer versions are available + but require manual approval), not "AtLatestKnown". + """ namespace = "cli-fvt-5" subscription = olm.applySubscription( dynClient, @@ -138,6 +147,15 @@ def test_crud_with_manual_approval_and_starting_csv(): assert subscription.spec.installPlanApproval == "Manual" assert subscription.spec.startingCSV == "ibm-sls.v3.8.0" + # Verify that the subscription reached UpgradePending state + # This confirms the InstallPlan was automatically approved and installed + # UpgradePending indicates newer versions are available but require manual approval + assert subscription.status.state == "UpgradePending" + + # Verify the installed CSV matches the startingCSV + installedCSV = subscription.status.installedCSV + assert installedCSV == "ibm-sls.v3.8.0" + # When we install the ibm-sls subscription OLM will automatically create the ibm-truststore-mgr # subscription, but when we delete the subscription, OLM will not automatically remove the latter olm.deleteSubscription(dynClient, namespace, "ibm-sls") diff --git a/test/src/test_olm_installplan_selection.py b/test/src/test_olm_installplan_selection.py new file mode 100644 index 00000000..bd84b459 --- /dev/null +++ b/test/src/test_olm_installplan_selection.py @@ -0,0 +1,421 @@ +# ***************************************************************************** +# Copyright (c) 2024 IBM Corporation and other Contributors. +# +# All rights reserved. This program and the accompanying materials +# are made available under the terms of the Eclipse Public License v1.0 +# which accompanies this distribution, and is available at +# http://www.eclipse.org/legal/epl-v10.html +# +# ***************************************************************************** + +""" +Unit tests for InstallPlan selection logic in applySubscription. + +These tests verify the fix for the bug where completed InstallPlans +might not be returned by label selector queries, causing infinite loops +when using Manual approval with startingCSV. +""" + +import pytest +from unittest.mock import Mock, patch +from mas.devops import olm + + +class MockResource: + """Mock Kubernetes resource object""" + def __init__(self, name, labels=None, owner_refs=None, csv_names=None, phase="Complete"): + self.metadata = Mock() + self.metadata.name = name + self.metadata.labels = labels or {} + self.metadata.ownerReferences = owner_refs or [] + + self.spec = Mock() + self.spec.clusterServiceVersionNames = csv_names or [] + + self.status = Mock() + self.status.phase = phase + + +class MockResourceList: + """Mock Kubernetes resource list""" + def __init__(self, items): + self.items = items + + +@pytest.fixture +def mock_dyn_client(): + """Create a mock DynamicClient""" + client = Mock() + return client + + +@pytest.fixture +def mock_env(): + """Create a mock Jinja2 Environment""" + env = Mock() + template = Mock() + template.render.return_value = """ +apiVersion: operators.coreos.com/v1alpha1 +kind: Subscription +metadata: + name: test-operator + namespace: test-namespace +spec: + channel: stable + name: test-operator + source: test-catalog + sourceNamespace: openshift-marketplace +""" + env.get_template.return_value = template + return env + + +def create_owner_ref(kind, name): + """Helper to create an owner reference""" + ref = Mock() + ref.kind = kind + ref.name = name + return ref + + +@patch('mas.devops.olm.createNamespace') +@patch('mas.devops.olm.ensureOperatorGroupExists') +@patch('mas.devops.olm.getPackageManifest') +@patch('mas.devops.olm.sleep') +def test_automatic_approval_uses_label_selector_only( + mock_sleep, mock_get_manifest, mock_ensure_og, mock_create_ns, mock_dyn_client, mock_env +): + """ + Test that automatic approval uses only the label selector (standard behavior). + Should NOT query all InstallPlans. + """ + # Setup mocks + mock_get_manifest.return_value = Mock( + status=Mock(defaultChannel="stable", catalogSource="test-catalog") + ) + + # Mock subscription API + sub_api = Mock() + sub_api.get.return_value = MockResourceList([]) # No existing subscription + sub_api.apply.return_value = Mock() + + # Mock InstallPlan API - label selector returns one InstallPlan + install_plan_api = Mock() + install_plan = MockResource( + name="install-plan-1", + labels={"operators.coreos.com/test-operator.test-namespace": ""}, + csv_names=["test-operator.v1.0.0"], + phase="Complete" + ) + install_plan_api.get.return_value = MockResourceList([install_plan]) + + # Setup resource API + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { + ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, + ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, + }.get((kwargs.get("api_version"), kwargs.get("kind"))) + + with patch('mas.devops.olm.Environment', return_value=mock_env): + # Call applySubscription with Automatic approval (default) + olm.applySubscription( + mock_dyn_client, + "test-namespace", + "test-operator", + packageChannel="stable", + installPlanApproval="Automatic" + ) + + # Verify InstallPlan API was called with label selector only + install_plan_calls = [c for c in install_plan_api.get.call_args_list] + + # Should only use label selector, never query all InstallPlans + for call_args in install_plan_calls: + args, kwargs = call_args + assert 'label_selector' in kwargs, "Should use label selector" + assert kwargs.get('namespace') == "test-namespace" + + +@patch('mas.devops.olm.createNamespace') +@patch('mas.devops.olm.ensureOperatorGroupExists') +@patch('mas.devops.olm.getPackageManifest') +@patch('mas.devops.olm.sleep') +def test_manual_approval_without_starting_csv_uses_label_selector_only( + mock_sleep, mock_get_manifest, mock_ensure_og, mock_create_ns, mock_dyn_client, mock_env +): + """ + Test that Manual approval WITHOUT startingCSV uses only label selector. + Should NOT query all InstallPlans. + """ + # Setup mocks + mock_get_manifest.return_value = Mock( + status=Mock(defaultChannel="stable", catalogSource="test-catalog") + ) + + # Mock subscription API + sub_api = Mock() + sub_api.get.return_value = MockResourceList([]) + sub_api.apply.return_value = Mock() + + # Mock InstallPlan API + install_plan_api = Mock() + install_plan = MockResource( + name="install-plan-1", + labels={"operators.coreos.com/test-operator.test-namespace": ""}, + csv_names=["test-operator.v1.0.0"], + phase="RequiresApproval" + ) + install_plan_api.get.return_value = MockResourceList([install_plan]) + install_plan_api.patch.return_value = Mock() + + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { + ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, + ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, + }.get((kwargs.get("api_version"), kwargs.get("kind"))) + + with patch('mas.devops.olm.Environment', return_value=mock_env): + # Call with Manual approval but NO startingCSV + olm.applySubscription( + mock_dyn_client, + "test-namespace", + "test-operator", + packageChannel="stable", + installPlanApproval="Manual" + ) + + # Verify only label selector was used + install_plan_calls = [c for c in install_plan_api.get.call_args_list] + for call_args in install_plan_calls: + args, kwargs = call_args + # Should only use label selector or get by name, never query all + assert 'label_selector' in kwargs or 'name' in kwargs + + +@patch('mas.devops.olm.createNamespace') +@patch('mas.devops.olm.ensureOperatorGroupExists') +@patch('mas.devops.olm.getPackageManifest') +@patch('mas.devops.olm.sleep') +def test_manual_approval_with_starting_csv_label_selector_finds_match( + mock_sleep, mock_get_manifest, mock_ensure_og, mock_create_ns, mock_dyn_client, mock_env +): + """ + Test Manual approval with startingCSV when label selector returns the correct InstallPlan. + Should use label selector result, NOT query all InstallPlans. + """ + # Setup mocks + mock_get_manifest.return_value = Mock( + status=Mock(defaultChannel="stable", catalogSource="test-catalog") + ) + + # Mock subscription API + sub_api = Mock() + sub_api.get.return_value = MockResourceList([]) + sub_api.apply.return_value = Mock() + + # Mock InstallPlan API - label selector returns matching InstallPlan + install_plan_api = Mock() + install_plan = MockResource( + name="install-plan-1", + labels={"operators.coreos.com/test-operator.test-namespace": ""}, + csv_names=["test-operator.v1.0.0"], # Matches startingCSV + phase="Complete" + ) + install_plan_api.get.return_value = MockResourceList([install_plan]) + + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { + ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, + ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, + }.get((kwargs.get("api_version"), kwargs.get("kind"))) + + with patch('mas.devops.olm.Environment', return_value=mock_env): + olm.applySubscription( + mock_dyn_client, + "test-namespace", + "test-operator", + packageChannel="stable", + installPlanApproval="Manual", + startingCSV="test-operator.v1.0.0" + ) + + # Verify we found the InstallPlan via label selector + # Should NOT have queried all InstallPlans (no call without label_selector) + install_plan_calls = [c for c in install_plan_api.get.call_args_list] + + # Check that we never queried without a label_selector or name + for call_args in install_plan_calls: + args, kwargs = call_args + assert 'label_selector' in kwargs or 'name' in kwargs, \ + "Should only use label selector or get by name, not query all" + + +@patch('mas.devops.olm.createNamespace') +@patch('mas.devops.olm.ensureOperatorGroupExists') +@patch('mas.devops.olm.getPackageManifest') +@patch('mas.devops.olm.sleep') +def test_manual_approval_with_starting_csv_fallback_to_ownership_search( + mock_sleep, mock_get_manifest, mock_ensure_og, mock_create_ns, mock_dyn_client, mock_env +): + """ + Test Manual approval with startingCSV when label selector misses the completed InstallPlan. + Should fall back to querying all InstallPlans and filter by subscription ownership. + This is the key scenario the bug fix addresses. + """ + # Setup mocks + mock_get_manifest.return_value = Mock( + status=Mock(defaultChannel="stable", catalogSource="test-catalog") + ) + + # Mock subscription API + sub_api = Mock() + sub_api.get.return_value = MockResourceList([]) + sub_api.apply.return_value = Mock() + + # Mock InstallPlan API + install_plan_api = Mock() + + # Label selector returns only the in-progress InstallPlan (wrong one) + wrong_install_plan = MockResource( + name="install-plan-2", + labels={"operators.coreos.com/test-operator.test-namespace": ""}, + csv_names=["test-operator.v2.0.0"], # Does NOT match startingCSV + phase="Installing" + ) + + # All InstallPlans query returns both (including the completed one) + correct_install_plan = MockResource( + name="install-plan-1", + labels={}, # Label might be removed from completed plan + owner_refs=[create_owner_ref("Subscription", "test-operator")], + csv_names=["test-operator.v1.0.0"], # Matches startingCSV + phase="Complete" + ) + + # Setup the mock to return different results based on parameters + def get_side_effect(*args, **kwargs): + if 'label_selector' in kwargs: + # Label selector query - returns only wrong InstallPlan + return MockResourceList([wrong_install_plan]) + elif 'name' in kwargs: + # Get by name - return the correct one + return correct_install_plan + else: + # Query all InstallPlans - returns both + return MockResourceList([correct_install_plan, wrong_install_plan]) + + install_plan_api.get.side_effect = get_side_effect + + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { + ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, + ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, + }.get((kwargs.get("api_version"), kwargs.get("kind"))) + + with patch('mas.devops.olm.Environment', return_value=mock_env): + olm.applySubscription( + mock_dyn_client, + "test-namespace", + "test-operator", + packageChannel="stable", + installPlanApproval="Manual", + startingCSV="test-operator.v1.0.0" + ) + + # Verify the fallback behavior occurred + install_plan_calls = [c for c in install_plan_api.get.call_args_list] + + # Should have: + # 1. Called with label_selector (initial query) + # 2. Called without label_selector (fallback to query all) + has_label_selector_call = any( + 'label_selector' in call_args[1] + for call_args in install_plan_calls + ) + has_all_query_call = any( + 'label_selector' not in call_args[1] and 'name' not in call_args[1] + for call_args in install_plan_calls + ) + + assert has_label_selector_call, "Should have tried label selector first" + assert has_all_query_call, "Should have fallen back to querying all InstallPlans" + + +@patch('mas.devops.olm.createNamespace') +@patch('mas.devops.olm.ensureOperatorGroupExists') +@patch('mas.devops.olm.getPackageManifest') +@patch('mas.devops.olm.sleep') +def test_manual_approval_filters_by_subscription_ownership( + mock_sleep, mock_get_manifest, mock_ensure_og, mock_create_ns, mock_dyn_client, mock_env +): + """ + Test that when querying all InstallPlans, we correctly filter by subscription ownership. + This ensures we don't accidentally use InstallPlans from other subscriptions. + """ + # Setup mocks + mock_get_manifest.return_value = Mock( + status=Mock(defaultChannel="stable", catalogSource="test-catalog") + ) + + # Mock subscription API + sub_api = Mock() + sub_api.get.return_value = MockResourceList([]) + sub_api.apply.return_value = Mock() + + # Mock InstallPlan API + install_plan_api = Mock() + + # Label selector returns wrong InstallPlan + wrong_install_plan = MockResource( + name="install-plan-wrong", + labels={"operators.coreos.com/test-operator.test-namespace": ""}, + csv_names=["test-operator.v2.0.0"], + phase="Installing" + ) + + # All InstallPlans includes: + # 1. Correct one owned by our subscription + correct_install_plan = MockResource( + name="install-plan-correct", + labels={}, + owner_refs=[create_owner_ref("Subscription", "test-operator")], + csv_names=["test-operator.v1.0.0"], + phase="Complete" + ) + + # 2. One owned by a different subscription (should be ignored) + other_subscription_plan = MockResource( + name="install-plan-other", + labels={}, + owner_refs=[create_owner_ref("Subscription", "other-operator")], + csv_names=["test-operator.v1.0.0"], # Same CSV but wrong subscription + phase="Complete" + ) + + def get_side_effect(*args, **kwargs): + if 'label_selector' in kwargs: + return MockResourceList([wrong_install_plan]) + elif 'name' in kwargs: + return correct_install_plan + else: + # Return all three InstallPlans + return MockResourceList([correct_install_plan, other_subscription_plan, wrong_install_plan]) + + install_plan_api.get.side_effect = get_side_effect + + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { + ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, + ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, + }.get((kwargs.get("api_version"), kwargs.get("kind"))) + + with patch('mas.devops.olm.Environment', return_value=mock_env): + olm.applySubscription( + mock_dyn_client, + "test-namespace", + "test-operator", + packageChannel="stable", + installPlanApproval="Manual", + startingCSV="test-operator.v1.0.0" + ) + + # The test passes if it completes without error + # The code should have found the correct InstallPlan by checking ownership + # and ignored the one from the other subscription + +# Made with Bob From 0a1b6cb392f10fe511ec16abf67984882867c9fe Mon Sep 17 00:00:00 2001 From: whitfiea Date: Fri, 6 Mar 2026 10:45:28 +0000 Subject: [PATCH 4/6] fix linting --- test/src/test_olm_installplan_selection.py | 84 +++++++++++----------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/test/src/test_olm_installplan_selection.py b/test/src/test_olm_installplan_selection.py index bd84b459..452432cb 100644 --- a/test/src/test_olm_installplan_selection.py +++ b/test/src/test_olm_installplan_selection.py @@ -23,21 +23,23 @@ class MockResource: """Mock Kubernetes resource object""" + def __init__(self, name, labels=None, owner_refs=None, csv_names=None, phase="Complete"): self.metadata = Mock() self.metadata.name = name self.metadata.labels = labels or {} self.metadata.ownerReferences = owner_refs or [] - + self.spec = Mock() self.spec.clusterServiceVersionNames = csv_names or [] - + self.status = Mock() self.status.phase = phase class MockResourceList: """Mock Kubernetes resource list""" + def __init__(self, items): self.items = items @@ -93,12 +95,12 @@ def test_automatic_approval_uses_label_selector_only( mock_get_manifest.return_value = Mock( status=Mock(defaultChannel="stable", catalogSource="test-catalog") ) - + # Mock subscription API sub_api = Mock() sub_api.get.return_value = MockResourceList([]) # No existing subscription sub_api.apply.return_value = Mock() - + # Mock InstallPlan API - label selector returns one InstallPlan install_plan_api = Mock() install_plan = MockResource( @@ -108,13 +110,13 @@ def test_automatic_approval_uses_label_selector_only( phase="Complete" ) install_plan_api.get.return_value = MockResourceList([install_plan]) - + # Setup resource API mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, }.get((kwargs.get("api_version"), kwargs.get("kind"))) - + with patch('mas.devops.olm.Environment', return_value=mock_env): # Call applySubscription with Automatic approval (default) olm.applySubscription( @@ -124,10 +126,10 @@ def test_automatic_approval_uses_label_selector_only( packageChannel="stable", installPlanApproval="Automatic" ) - + # Verify InstallPlan API was called with label selector only install_plan_calls = [c for c in install_plan_api.get.call_args_list] - + # Should only use label selector, never query all InstallPlans for call_args in install_plan_calls: args, kwargs = call_args @@ -150,12 +152,12 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( mock_get_manifest.return_value = Mock( status=Mock(defaultChannel="stable", catalogSource="test-catalog") ) - + # Mock subscription API sub_api = Mock() sub_api.get.return_value = MockResourceList([]) sub_api.apply.return_value = Mock() - + # Mock InstallPlan API install_plan_api = Mock() install_plan = MockResource( @@ -166,12 +168,12 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( ) install_plan_api.get.return_value = MockResourceList([install_plan]) install_plan_api.patch.return_value = Mock() - + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, }.get((kwargs.get("api_version"), kwargs.get("kind"))) - + with patch('mas.devops.olm.Environment', return_value=mock_env): # Call with Manual approval but NO startingCSV olm.applySubscription( @@ -181,7 +183,7 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( packageChannel="stable", installPlanApproval="Manual" ) - + # Verify only label selector was used install_plan_calls = [c for c in install_plan_api.get.call_args_list] for call_args in install_plan_calls: @@ -205,12 +207,12 @@ def test_manual_approval_with_starting_csv_label_selector_finds_match( mock_get_manifest.return_value = Mock( status=Mock(defaultChannel="stable", catalogSource="test-catalog") ) - + # Mock subscription API sub_api = Mock() sub_api.get.return_value = MockResourceList([]) sub_api.apply.return_value = Mock() - + # Mock InstallPlan API - label selector returns matching InstallPlan install_plan_api = Mock() install_plan = MockResource( @@ -220,12 +222,12 @@ def test_manual_approval_with_starting_csv_label_selector_finds_match( phase="Complete" ) install_plan_api.get.return_value = MockResourceList([install_plan]) - + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, }.get((kwargs.get("api_version"), kwargs.get("kind"))) - + with patch('mas.devops.olm.Environment', return_value=mock_env): olm.applySubscription( mock_dyn_client, @@ -235,11 +237,11 @@ def test_manual_approval_with_starting_csv_label_selector_finds_match( installPlanApproval="Manual", startingCSV="test-operator.v1.0.0" ) - + # Verify we found the InstallPlan via label selector # Should NOT have queried all InstallPlans (no call without label_selector) install_plan_calls = [c for c in install_plan_api.get.call_args_list] - + # Check that we never queried without a label_selector or name for call_args in install_plan_calls: args, kwargs = call_args @@ -263,15 +265,15 @@ def test_manual_approval_with_starting_csv_fallback_to_ownership_search( mock_get_manifest.return_value = Mock( status=Mock(defaultChannel="stable", catalogSource="test-catalog") ) - + # Mock subscription API sub_api = Mock() sub_api.get.return_value = MockResourceList([]) sub_api.apply.return_value = Mock() - + # Mock InstallPlan API install_plan_api = Mock() - + # Label selector returns only the in-progress InstallPlan (wrong one) wrong_install_plan = MockResource( name="install-plan-2", @@ -279,7 +281,7 @@ def test_manual_approval_with_starting_csv_fallback_to_ownership_search( csv_names=["test-operator.v2.0.0"], # Does NOT match startingCSV phase="Installing" ) - + # All InstallPlans query returns both (including the completed one) correct_install_plan = MockResource( name="install-plan-1", @@ -288,7 +290,7 @@ def test_manual_approval_with_starting_csv_fallback_to_ownership_search( csv_names=["test-operator.v1.0.0"], # Matches startingCSV phase="Complete" ) - + # Setup the mock to return different results based on parameters def get_side_effect(*args, **kwargs): if 'label_selector' in kwargs: @@ -300,14 +302,14 @@ def get_side_effect(*args, **kwargs): else: # Query all InstallPlans - returns both return MockResourceList([correct_install_plan, wrong_install_plan]) - + install_plan_api.get.side_effect = get_side_effect - + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, }.get((kwargs.get("api_version"), kwargs.get("kind"))) - + with patch('mas.devops.olm.Environment', return_value=mock_env): olm.applySubscription( mock_dyn_client, @@ -317,22 +319,22 @@ def get_side_effect(*args, **kwargs): installPlanApproval="Manual", startingCSV="test-operator.v1.0.0" ) - + # Verify the fallback behavior occurred install_plan_calls = [c for c in install_plan_api.get.call_args_list] - + # Should have: # 1. Called with label_selector (initial query) # 2. Called without label_selector (fallback to query all) has_label_selector_call = any( - 'label_selector' in call_args[1] + 'label_selector' in call_args[1] for call_args in install_plan_calls ) has_all_query_call = any( 'label_selector' not in call_args[1] and 'name' not in call_args[1] for call_args in install_plan_calls ) - + assert has_label_selector_call, "Should have tried label selector first" assert has_all_query_call, "Should have fallen back to querying all InstallPlans" @@ -352,15 +354,15 @@ def test_manual_approval_filters_by_subscription_ownership( mock_get_manifest.return_value = Mock( status=Mock(defaultChannel="stable", catalogSource="test-catalog") ) - + # Mock subscription API sub_api = Mock() sub_api.get.return_value = MockResourceList([]) sub_api.apply.return_value = Mock() - + # Mock InstallPlan API install_plan_api = Mock() - + # Label selector returns wrong InstallPlan wrong_install_plan = MockResource( name="install-plan-wrong", @@ -368,7 +370,7 @@ def test_manual_approval_filters_by_subscription_ownership( csv_names=["test-operator.v2.0.0"], phase="Installing" ) - + # All InstallPlans includes: # 1. Correct one owned by our subscription correct_install_plan = MockResource( @@ -378,7 +380,7 @@ def test_manual_approval_filters_by_subscription_ownership( csv_names=["test-operator.v1.0.0"], phase="Complete" ) - + # 2. One owned by a different subscription (should be ignored) other_subscription_plan = MockResource( name="install-plan-other", @@ -387,7 +389,7 @@ def test_manual_approval_filters_by_subscription_ownership( csv_names=["test-operator.v1.0.0"], # Same CSV but wrong subscription phase="Complete" ) - + def get_side_effect(*args, **kwargs): if 'label_selector' in kwargs: return MockResourceList([wrong_install_plan]) @@ -396,14 +398,14 @@ def get_side_effect(*args, **kwargs): else: # Return all three InstallPlans return MockResourceList([correct_install_plan, other_subscription_plan, wrong_install_plan]) - + install_plan_api.get.side_effect = get_side_effect - + mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, ("operators.coreos.com/v1alpha1", "InstallPlan"): install_plan_api, }.get((kwargs.get("api_version"), kwargs.get("kind"))) - + with patch('mas.devops.olm.Environment', return_value=mock_env): olm.applySubscription( mock_dyn_client, @@ -413,7 +415,7 @@ def get_side_effect(*args, **kwargs): installPlanApproval="Manual", startingCSV="test-operator.v1.0.0" ) - + # The test passes if it completes without error # The code should have found the correct InstallPlan by checking ownership # and ignored the one from the other subscription From b5a52dd31a2c538caeb99d240877b51b855e8516 Mon Sep 17 00:00:00 2001 From: whitfiea Date: Fri, 6 Mar 2026 11:34:45 +0000 Subject: [PATCH 5/6] fixes for test --- src/mas/devops/olm.py | 7 +- test/src/test_olm.py | 36 ++--- test/src/test_olm_installplan_selection.py | 156 ++++++++++++++++++--- 3 files changed, 159 insertions(+), 40 deletions(-) diff --git a/src/mas/devops/olm.py b/src/mas/devops/olm.py index 1153aca6..63f5f0d4 100644 --- a/src/mas/devops/olm.py +++ b/src/mas/devops/olm.py @@ -140,15 +140,18 @@ def applySubscription(dynClient: DynamicClient, namespace: str, packageName: str installMode (str, optional): Install mode for the OperatorGroup. Defaults to "OwnNamespace". installPlanApproval (str, optional): Install plan approval mode ("Automatic" or "Manual"). Defaults to None. startingCSV (str, optional): The specific CSV version to install. When combined with Manual approval, - the first InstallPlan to this CSV will be automatically approved. Defaults to None. + the first InstallPlan to this CSV will be automatically approved. Required when installPlanApproval is "Manual". Defaults to None. Returns: Subscription: The created or updated subscription resource Raises: - OLMException: If the package is not available in any catalog + OLMException: If the package is not available in any catalog, or if installPlanApproval is "Manual" without a startingCSV NotFoundError: If resources cannot be created """ + # Validate that startingCSV is provided when installPlanApproval is Manual + if installPlanApproval == "Manual" and startingCSV is None: + raise OLMException("When installPlanApproval is 'Manual', a startingCSV must be provided") if catalogSourceNamespace is None: catalogSourceNamespace = "openshift-marketplace" diff --git a/test/src/test_olm.py b/test/src/test_olm.py index 0fbade94..ec4540b7 100644 --- a/test/src/test_olm.py +++ b/test/src/test_olm.py @@ -82,23 +82,27 @@ def test_crud_with_config(): def test_crud_with_manual_approval(): + """ + Test that when installPlanApproval is Manual without a startingCSV, + an OLMException is raised. + """ namespace = "cli-fvt-3" - subscription = olm.applySubscription( - dynClient, - namespace, - "ibm-sls", - packageChannel="3.x", - installPlanApproval="Manual" - ) - assert subscription.metadata.name == "ibm-sls" - assert subscription.metadata.namespace == namespace - assert subscription.spec.installPlanApproval == "Manual" - - # When we install the ibm-sls subscription OLM will automatically create the ibm-truststore-mgr - # subscription, but when we delete the subscription, OLM will not automatically remove the latter - olm.deleteSubscription(dynClient, namespace, "ibm-sls") - olm.deleteSubscription(dynClient, namespace, "ibm-truststore-mgr") - ocp.deleteNamespace(dynClient, namespace) + + # This should raise an OLMException because Manual approval requires a startingCSV + try: + olm.applySubscription( + dynClient, + namespace, + "ibm-sls", + packageChannel="3.x", + installPlanApproval="Manual" + ) + # If we get here, the test should fail + assert False, "Expected OLMException to be raised when installPlanApproval is Manual without startingCSV" + except olm.OLMException as e: + # Verify the error message is correct + assert "When installPlanApproval is 'Manual', a startingCSV must be provided" in str(e) + # Test passed - exception was raised as expected def test_crud_with_starting_csv(): diff --git a/test/src/test_olm_installplan_selection.py b/test/src/test_olm_installplan_selection.py index 452432cb..e390dfb3 100644 --- a/test/src/test_olm_installplan_selection.py +++ b/test/src/test_olm_installplan_selection.py @@ -11,9 +11,9 @@ """ Unit tests for InstallPlan selection logic in applySubscription. -These tests verify the fix for the bug where completed InstallPlans -might not be returned by label selector queries, causing infinite loops -when using Manual approval with startingCSV. +These tests mock up the resources rather than use real resources as the +OLM processing on a cluster is quite slow and it would take time to +exercise all these scenerios """ import pytest @@ -42,6 +42,8 @@ class MockResourceList: def __init__(self, items): self.items = items + # Add status attribute to match real ResourceList behavior + self.status = Mock() @pytest.fixture @@ -98,7 +100,19 @@ def test_automatic_approval_uses_label_selector_only( # Mock subscription API sub_api = Mock() - sub_api.get.return_value = MockResourceList([]) # No existing subscription + + # Mock subscription resource with proper status + mock_subscription = Mock() + mock_subscription.metadata.name = "test-operator" + mock_subscription.status = Mock() + mock_subscription.status.state = "AtLatestKnown" + mock_subscription.status.installedCSV = "test-operator.v1.0.0" + + # First call returns empty list (no existing subscription), subsequent calls return the subscription + sub_api.get.side_effect = [ + MockResourceList([]), # Initial check for existing subscription + mock_subscription # Subsequent calls when waiting for subscription to complete + ] sub_api.apply.return_value = Mock() # Mock InstallPlan API - label selector returns one InstallPlan @@ -145,7 +159,7 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( mock_sleep, mock_get_manifest, mock_ensure_og, mock_create_ns, mock_dyn_client, mock_env ): """ - Test that Manual approval WITHOUT startingCSV uses only label selector. + Test that Manual approval with startingCSV uses only label selector when it finds a match. Should NOT query all InstallPlans. """ # Setup mocks @@ -155,18 +169,46 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( # Mock subscription API sub_api = Mock() - sub_api.get.return_value = MockResourceList([]) + + # Mock subscription resource with proper status + mock_subscription = Mock() + mock_subscription.metadata.name = "test-operator" + mock_subscription.status = Mock() + mock_subscription.status.state = "UpgradePending" + mock_subscription.status.installedCSV = "test-operator.v1.0.0" + + # First call returns empty list (no existing subscription), subsequent calls return the subscription + sub_api.get.side_effect = [ + MockResourceList([]), # Initial check for existing subscription + mock_subscription # Subsequent calls when waiting for subscription to complete + ] sub_api.apply.return_value = Mock() # Mock InstallPlan API install_plan_api = Mock() - install_plan = MockResource( + install_plan_requires_approval = MockResource( name="install-plan-1", labels={"operators.coreos.com/test-operator.test-namespace": ""}, csv_names=["test-operator.v1.0.0"], phase="RequiresApproval" ) - install_plan_api.get.return_value = MockResourceList([install_plan]) + install_plan_complete = MockResource( + name="install-plan-1", + labels={"operators.coreos.com/test-operator.test-namespace": ""}, + csv_names=["test-operator.v1.0.0"], + phase="Complete" + ) + + # Simulate phase transition: first returns RequiresApproval, then Complete after patch + def get_install_plan_side_effect(*args, **kwargs): + if 'name' in kwargs: + # After patch is called, return Complete phase + return install_plan_complete + else: + # Initial query with label selector + return MockResourceList([install_plan_requires_approval]) + + install_plan_api.get.side_effect = get_install_plan_side_effect install_plan_api.patch.return_value = Mock() mock_dyn_client.resources.get.side_effect = lambda **kwargs: { @@ -175,13 +217,14 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( }.get((kwargs.get("api_version"), kwargs.get("kind"))) with patch('mas.devops.olm.Environment', return_value=mock_env): - # Call with Manual approval but NO startingCSV + # Call with Manual approval with startingCSV olm.applySubscription( mock_dyn_client, "test-namespace", "test-operator", packageChannel="stable", - installPlanApproval="Manual" + installPlanApproval="Manual", + startingCSV="test-operator.v1.0.0" ) # Verify only label selector was used @@ -210,18 +253,45 @@ def test_manual_approval_with_starting_csv_label_selector_finds_match( # Mock subscription API sub_api = Mock() - sub_api.get.return_value = MockResourceList([]) + + # Mock subscription resource with proper status + mock_subscription = Mock() + mock_subscription.metadata.name = "test-operator" + mock_subscription.status = Mock() + mock_subscription.status.state = "UpgradePending" + mock_subscription.status.installedCSV = "test-operator.v1.0.0" + + # First call returns empty list (no existing subscription), subsequent calls return the subscription + sub_api.get.side_effect = [ + MockResourceList([]), # Initial check for existing subscription + mock_subscription # Subsequent calls when waiting for subscription to complete + ] sub_api.apply.return_value = Mock() # Mock InstallPlan API - label selector returns matching InstallPlan install_plan_api = Mock() - install_plan = MockResource( + install_plan_requires_approval = MockResource( name="install-plan-1", labels={"operators.coreos.com/test-operator.test-namespace": ""}, csv_names=["test-operator.v1.0.0"], # Matches startingCSV + phase="RequiresApproval" + ) + install_plan_complete = MockResource( + name="install-plan-1", + labels={"operators.coreos.com/test-operator.test-namespace": ""}, + csv_names=["test-operator.v1.0.0"], phase="Complete" ) - install_plan_api.get.return_value = MockResourceList([install_plan]) + + # Simulate phase transition + def get_install_plan_side_effect(*args, **kwargs): + if 'name' in kwargs: + return install_plan_complete + else: + return MockResourceList([install_plan_requires_approval]) + + install_plan_api.get.side_effect = get_install_plan_side_effect + install_plan_api.patch.return_value = Mock() mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, @@ -268,7 +338,19 @@ def test_manual_approval_with_starting_csv_fallback_to_ownership_search( # Mock subscription API sub_api = Mock() - sub_api.get.return_value = MockResourceList([]) + + # Mock subscription resource with proper status + mock_subscription = Mock() + mock_subscription.metadata.name = "test-operator" + mock_subscription.status = Mock() + mock_subscription.status.state = "UpgradePending" + mock_subscription.status.installedCSV = "test-operator.v1.0.0" + + # First call returns empty list (no existing subscription), subsequent calls return the subscription + sub_api.get.side_effect = [ + MockResourceList([]), # Initial check for existing subscription + mock_subscription # Subsequent calls when waiting for subscription to complete + ] sub_api.apply.return_value = Mock() # Mock InstallPlan API @@ -283,11 +365,19 @@ def test_manual_approval_with_starting_csv_fallback_to_ownership_search( ) # All InstallPlans query returns both (including the completed one) - correct_install_plan = MockResource( + correct_install_plan_requires_approval = MockResource( name="install-plan-1", labels={}, # Label might be removed from completed plan owner_refs=[create_owner_ref("Subscription", "test-operator")], csv_names=["test-operator.v1.0.0"], # Matches startingCSV + phase="RequiresApproval" + ) + + correct_install_plan_complete = MockResource( + name="install-plan-1", + labels={}, + owner_refs=[create_owner_ref("Subscription", "test-operator")], + csv_names=["test-operator.v1.0.0"], phase="Complete" ) @@ -297,13 +387,14 @@ def get_side_effect(*args, **kwargs): # Label selector query - returns only wrong InstallPlan return MockResourceList([wrong_install_plan]) elif 'name' in kwargs: - # Get by name - return the correct one - return correct_install_plan + # Get by name - return the correct one (Complete after patch) + return correct_install_plan_complete else: # Query all InstallPlans - returns both - return MockResourceList([correct_install_plan, wrong_install_plan]) + return MockResourceList([correct_install_plan_requires_approval, wrong_install_plan]) install_plan_api.get.side_effect = get_side_effect + install_plan_api.patch.return_value = Mock() mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, @@ -357,7 +448,19 @@ def test_manual_approval_filters_by_subscription_ownership( # Mock subscription API sub_api = Mock() - sub_api.get.return_value = MockResourceList([]) + + # Mock subscription resource with proper status + mock_subscription = Mock() + mock_subscription.metadata.name = "test-operator" + mock_subscription.status = Mock() + mock_subscription.status.state = "UpgradePending" + mock_subscription.status.installedCSV = "test-operator.v1.0.0" + + # First call returns empty list (no existing subscription), subsequent calls return the subscription + sub_api.get.side_effect = [ + MockResourceList([]), # Initial check for existing subscription + mock_subscription # Subsequent calls when waiting for subscription to complete + ] sub_api.apply.return_value = Mock() # Mock InstallPlan API @@ -373,7 +476,15 @@ def test_manual_approval_filters_by_subscription_ownership( # All InstallPlans includes: # 1. Correct one owned by our subscription - correct_install_plan = MockResource( + correct_install_plan_requires_approval = MockResource( + name="install-plan-correct", + labels={}, + owner_refs=[create_owner_ref("Subscription", "test-operator")], + csv_names=["test-operator.v1.0.0"], + phase="RequiresApproval" + ) + + correct_install_plan_complete = MockResource( name="install-plan-correct", labels={}, owner_refs=[create_owner_ref("Subscription", "test-operator")], @@ -394,12 +505,13 @@ def get_side_effect(*args, **kwargs): if 'label_selector' in kwargs: return MockResourceList([wrong_install_plan]) elif 'name' in kwargs: - return correct_install_plan + return correct_install_plan_complete else: # Return all three InstallPlans - return MockResourceList([correct_install_plan, other_subscription_plan, wrong_install_plan]) + return MockResourceList([correct_install_plan_requires_approval, other_subscription_plan, wrong_install_plan]) install_plan_api.get.side_effect = get_side_effect + install_plan_api.patch.return_value = Mock() mock_dyn_client.resources.get.side_effect = lambda **kwargs: { ("operators.coreos.com/v1alpha1", "Subscription"): sub_api, From e608a0a59fd95371386b04d425e88f7d3b9bc535 Mon Sep 17 00:00:00 2001 From: whitfiea Date: Fri, 6 Mar 2026 11:35:32 +0000 Subject: [PATCH 6/6] linting --- test/src/test_olm.py | 2 +- test/src/test_olm_installplan_selection.py | 34 +++++++++++----------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/test/src/test_olm.py b/test/src/test_olm.py index ec4540b7..354a9e2a 100644 --- a/test/src/test_olm.py +++ b/test/src/test_olm.py @@ -87,7 +87,7 @@ def test_crud_with_manual_approval(): an OLMException is raised. """ namespace = "cli-fvt-3" - + # This should raise an OLMException because Manual approval requires a startingCSV try: olm.applySubscription( diff --git a/test/src/test_olm_installplan_selection.py b/test/src/test_olm_installplan_selection.py index e390dfb3..1101742c 100644 --- a/test/src/test_olm_installplan_selection.py +++ b/test/src/test_olm_installplan_selection.py @@ -12,7 +12,7 @@ Unit tests for InstallPlan selection logic in applySubscription. These tests mock up the resources rather than use real resources as the -OLM processing on a cluster is quite slow and it would take time to +OLM processing on a cluster is quite slow and it would take time to exercise all these scenerios """ @@ -100,14 +100,14 @@ def test_automatic_approval_uses_label_selector_only( # Mock subscription API sub_api = Mock() - + # Mock subscription resource with proper status mock_subscription = Mock() mock_subscription.metadata.name = "test-operator" mock_subscription.status = Mock() mock_subscription.status.state = "AtLatestKnown" mock_subscription.status.installedCSV = "test-operator.v1.0.0" - + # First call returns empty list (no existing subscription), subsequent calls return the subscription sub_api.get.side_effect = [ MockResourceList([]), # Initial check for existing subscription @@ -169,14 +169,14 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( # Mock subscription API sub_api = Mock() - + # Mock subscription resource with proper status mock_subscription = Mock() mock_subscription.metadata.name = "test-operator" mock_subscription.status = Mock() mock_subscription.status.state = "UpgradePending" mock_subscription.status.installedCSV = "test-operator.v1.0.0" - + # First call returns empty list (no existing subscription), subsequent calls return the subscription sub_api.get.side_effect = [ MockResourceList([]), # Initial check for existing subscription @@ -198,7 +198,7 @@ def test_manual_approval_without_starting_csv_uses_label_selector_only( csv_names=["test-operator.v1.0.0"], phase="Complete" ) - + # Simulate phase transition: first returns RequiresApproval, then Complete after patch def get_install_plan_side_effect(*args, **kwargs): if 'name' in kwargs: @@ -207,7 +207,7 @@ def get_install_plan_side_effect(*args, **kwargs): else: # Initial query with label selector return MockResourceList([install_plan_requires_approval]) - + install_plan_api.get.side_effect = get_install_plan_side_effect install_plan_api.patch.return_value = Mock() @@ -253,14 +253,14 @@ def test_manual_approval_with_starting_csv_label_selector_finds_match( # Mock subscription API sub_api = Mock() - + # Mock subscription resource with proper status mock_subscription = Mock() mock_subscription.metadata.name = "test-operator" mock_subscription.status = Mock() mock_subscription.status.state = "UpgradePending" mock_subscription.status.installedCSV = "test-operator.v1.0.0" - + # First call returns empty list (no existing subscription), subsequent calls return the subscription sub_api.get.side_effect = [ MockResourceList([]), # Initial check for existing subscription @@ -282,14 +282,14 @@ def test_manual_approval_with_starting_csv_label_selector_finds_match( csv_names=["test-operator.v1.0.0"], phase="Complete" ) - + # Simulate phase transition def get_install_plan_side_effect(*args, **kwargs): if 'name' in kwargs: return install_plan_complete else: return MockResourceList([install_plan_requires_approval]) - + install_plan_api.get.side_effect = get_install_plan_side_effect install_plan_api.patch.return_value = Mock() @@ -338,14 +338,14 @@ def test_manual_approval_with_starting_csv_fallback_to_ownership_search( # Mock subscription API sub_api = Mock() - + # Mock subscription resource with proper status mock_subscription = Mock() mock_subscription.metadata.name = "test-operator" mock_subscription.status = Mock() mock_subscription.status.state = "UpgradePending" mock_subscription.status.installedCSV = "test-operator.v1.0.0" - + # First call returns empty list (no existing subscription), subsequent calls return the subscription sub_api.get.side_effect = [ MockResourceList([]), # Initial check for existing subscription @@ -372,7 +372,7 @@ def test_manual_approval_with_starting_csv_fallback_to_ownership_search( csv_names=["test-operator.v1.0.0"], # Matches startingCSV phase="RequiresApproval" ) - + correct_install_plan_complete = MockResource( name="install-plan-1", labels={}, @@ -448,14 +448,14 @@ def test_manual_approval_filters_by_subscription_ownership( # Mock subscription API sub_api = Mock() - + # Mock subscription resource with proper status mock_subscription = Mock() mock_subscription.metadata.name = "test-operator" mock_subscription.status = Mock() mock_subscription.status.state = "UpgradePending" mock_subscription.status.installedCSV = "test-operator.v1.0.0" - + # First call returns empty list (no existing subscription), subsequent calls return the subscription sub_api.get.side_effect = [ MockResourceList([]), # Initial check for existing subscription @@ -483,7 +483,7 @@ def test_manual_approval_filters_by_subscription_ownership( csv_names=["test-operator.v1.0.0"], phase="RequiresApproval" ) - + correct_install_plan_complete = MockResource( name="install-plan-correct", labels={},