fix: move maas component_health under maas_billing#1310
Merged
sheltoncyril merged 2 commits intoopendatahub-io:mainfrom Mar 26, 2026
Merged
fix: move maas component_health under maas_billing#1310sheltoncyril merged 2 commits intoopendatahub-io:mainfrom
sheltoncyril merged 2 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/verified', '/wip', '/lgtm', '/hold', '/cherry-pick'} |
Contributor
📝 WalkthroughWalkthroughAdded two MaaS component-health test modules and reorganized MaaS fixtures: moved PostgreSQL, namespace, controller-enablement, and free-tier provisioning fixtures into the parent conftest, removed duplicate fixtures from the subscription conftest, and introduced several new session- and class-scoped fixtures for MaaS setup. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/conftest.py (1)
1086-1092: Encode the DSC prerequisite in the root free-stack fixture.
maas_inference_service_tinyllama_freecan be requested directly withoutmaas_subscription_controller_enabled_latest, and then every downstream free-stack fixture inherits a race againstmodelsAsServicestill being disabled. Make the dependency explicit here once instead of relying on each consumer to remember a class-levelusefixtures.Proposed fix
def maas_inference_service_tinyllama_free( admin_client: DynamicClient, maas_unprivileged_model_namespace: Namespace, maas_model_service_account: ServiceAccount, + maas_subscription_controller_enabled_latest: None, maas_gateway_api: None, ) -> Generator[LLMInferenceService, Any, Any]:As per coding guidelines, "Code reuse, test parameterization, proper test dependency should be also encouraged".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/conftest.py` around lines 1086 - 1092, The fixture maas_inference_service_tinyllama_free must explicitly depend on the DSC prerequisite so downstream free-stack consumers don't race; add the maas_subscription_controller_enabled_latest fixture as an argument (or annotate with pytest.mark.usefixtures("maas_subscription_controller_enabled_latest")) to the maas_inference_service_tinyllama_free fixture definition so the DSC prerequisite is always applied at the root free-stack fixture rather than left to consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/model_serving/maas_billing/component_health/test_maas_api_health_check.py`:
- Around line 58-66: The test test_maas_api_pods_health currently waits for all
pods in applications_namespace; restrict it to only maas-api pods by selecting
them first (e.g., read the maas-api Deployment or use its label selector like
"app=maas-api") via admin_client, then pass that selector or the filtered pod
list into wait_for_pods_running (or call wait_for_pods_running with a
label_selector argument if supported); update the test to derive the selector
from the maas-api Deployment/labels and use that selector when invoking
wait_for_pods_running so only maas-api rollout readiness is asserted.
In
`@tests/model_serving/maas_billing/component_health/test_maas_controller_health_check.py`:
- Around line 71-78: The test_maas_controller_pods_health currently calls
wait_for_pods_running(namespace_name=applications_namespace) which checks the
whole namespace; change it to only target pods owned by the maas-controller
Deployment: use the admin_client to fetch the maas-controller Deployment
(Deployment name "maas-controller") in the same namespace, extract its pod
selector labels or ownerReferences, list pods matching that selector, and then
call wait_for_pods_running for only those pods (or pass a filtered
namespace+label selector if wait_for_pods_running supports it). Update
test_maas_controller_pods_health to query the Deployment selector and scope the
wait_for_pods_running invocation to that selector/filtered pod list so unrelated
namespace pods cannot affect the result.
- Around line 30-50: The test currently only verifies CRD existence via
CustomResourceDefinition(..., ensure_exists=True) and may pass while CRDs are
not ready; update test_maas_controller_crds_exist to also assert the CRD
readiness by checking the CRD status conditions (e.g., look up
CustomResourceDefinition.obj['status']['conditions'] or use any provided
wait/ready helper on CustomResourceDefinition) and require a condition with type
"Established" and status "True" (optionally also check "NamesAccepted" if
desired), retrying/polling with a short timeout per CRD before failing so
partial installs are not considered healthy.
In `@tests/model_serving/maas_billing/conftest.py`:
- Around line 1039-1053: The maas_subscription_namespace fixture currently
reuses MAAS_SUBSCRIPTION_NAMESPACE which can contain stale MaaSAuthPolicy or
MaaSSubscription objects; modify maas_subscription_namespace to either always
create an isolated namespace via create_ns (rather than yielding existing_ns
when Namespace(...).exists is true) or, if reusing the namespace, explicitly
scrub known resources before yielding by deleting any MaaSAuthPolicy and
MaaSSubscription objects found using admin_client (or unprivileged_client) APIs;
locate the fixture maas_subscription_namespace and update the branch that
handles existing_ns to perform the cleanup of MaaSAuthPolicy/MaaSSubscription
(or switch to creating a fresh namespace with create_ns) so tests run against a
deterministic, clean state.
---
Nitpick comments:
In `@tests/model_serving/maas_billing/conftest.py`:
- Around line 1086-1092: The fixture maas_inference_service_tinyllama_free must
explicitly depend on the DSC prerequisite so downstream free-stack consumers
don't race; add the maas_subscription_controller_enabled_latest fixture as an
argument (or annotate with
pytest.mark.usefixtures("maas_subscription_controller_enabled_latest")) to the
maas_inference_service_tinyllama_free fixture definition so the DSC prerequisite
is always applied at the root free-stack fixture rather than left to consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5c969a37-bba1-43cf-bf41-c6e1bb069295
📒 Files selected for processing (5)
tests/model_serving/maas_billing/component_health/__init__.pytests/model_serving/maas_billing/component_health/test_maas_api_health_check.pytests/model_serving/maas_billing/component_health/test_maas_controller_health_check.pytests/model_serving/maas_billing/conftest.pytests/model_serving/maas_billing/maas_subscription/conftest.py
tests/model_serving/maas_billing/component_health/test_maas_api_health_check.py
Show resolved
Hide resolved
tests/model_serving/maas_billing/component_health/test_maas_controller_health_check.py
Show resolved
Hide resolved
tests/model_serving/maas_billing/component_health/test_maas_controller_health_check.py
Show resolved
Hide resolved
SB159
force-pushed
the
fix/move-maas-component-health
branch
from
cd31d97 to
7f403b2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/model_serving/maas_billing/component_health/test_maas_controller_health_check.py (2)
71-78:⚠️ Potential issue | 🟠 MajorScope pod health check to
maas-controllerpods only.
wait_for_pods_running(namespace_name=applications_namespace)checks all pods in the namespace. Unrelated pod failures or delays will affect this test result. Filter pods by themaas-controllerDeployment's selector labels.def test_maas_controller_pods_health( self, admin_client: DynamicClient, ) -> None: """Verify maas-controller pods are Running.""" applications_namespace = py_config["applications_namespace"] + controller_deployment = Deployment( + client=admin_client, + name="maas-controller", + namespace=applications_namespace, + ensure_exists=True, + ) + selector = controller_deployment.instance.spec.selector.matchLabels LOGGER.info(f"Testing Pods in namespace {applications_namespace} for MaaS Controller health") - wait_for_pods_running(admin_client=admin_client, namespace_name=applications_namespace) + wait_for_pods_running( + admin_client=admin_client, + namespace_name=applications_namespace, + label_selector=",".join(f"{k}={v}" for k, v in selector.items()), + )Verify
wait_for_pods_runningsupports alabel_selectorparameter:#!/bin/bash # Check wait_for_pods_running signature ast-grep --pattern 'def wait_for_pods_running($$$)'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/component_health/test_maas_controller_health_check.py` around lines 71 - 78, The test test_maas_controller_pods_health currently checks all pods in the namespace; limit it to maas-controller pods by reading the maas-controller Deployment's selector labels (fetch the Deployment named "maas-controller" using admin_client in applications_namespace, read spec.selector.matchLabels and convert to a label selector string like "key1=value1,key2=value2") and pass that string as the label_selector argument to wait_for_pods_running(admin_client=..., namespace_name=applications_namespace, label_selector=label_selector). Ensure wait_for_pods_running supports a label_selector parameter (if it does not, add support to accept and forward the selector to the API call that lists pods).
30-50:⚠️ Potential issue | 🟠 MajorCheck CRD
Establishedcondition, not just existence.
CustomResourceDefinition(..., ensure_exists=True)and.existsdo not guarantee the CRD is usable. A CRD can exist withEstablished=False. For a component-health gate, verify theEstablishedcondition isTrue.for crd_name in expected_crd_names: crd_resource = CustomResourceDefinition( client=admin_client, name=crd_name, ensure_exists=True, ) if not crd_resource.exists: missing_crds.append(crd_name) + else: + crd_resource.wait_for_condition( + condition="Established", + status="True", + timeout=60, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/component_health/test_maas_controller_health_check.py` around lines 30 - 50, The test test_maas_controller_crds_exist currently only checks CustomResourceDefinition(..., ensure_exists=True) and .exists; update it to also verify each CRD's status.conditions contains an entry with type "Established" and status "True" (e.g., inspect the CRD object from CustomResourceDefinition or via the underlying admin_client resource and check resource.status.conditions for type == "Established"). If the Established condition is missing or not "True", treat that CRD as failing and append its name to missing_crds; update the final assertion message to indicate CRDs missing or not Established.tests/model_serving/maas_billing/conftest.py (1)
1039-1053:⚠️ Potential issue | 🟠 MajorClean stale MaaS resources when reusing namespace.
When
MAAS_SUBSCRIPTION_NAMESPACEalready exists, this fixture yields it without clearing previous MaaS resources. The free-tier fixtures create deterministic names (tinyllama-free-access,tinyllama-free-subscription), so a failed previous run can leave stale objects causingAlreadyExistserrors or tests passing against old state.Either create a fresh namespace each session or scrub known MaaS resources before yielding:
existing_ns = Namespace(client=admin_client, name=MAAS_SUBSCRIPTION_NAMESPACE) if existing_ns.exists: LOGGER.info(f"Namespace {MAAS_SUBSCRIPTION_NAMESPACE} already exists, reusing it") + # Scrub stale MaaS resources + for auth_policy in MaaSAuthPolicy.get(client=admin_client, namespace=MAAS_SUBSCRIPTION_NAMESPACE): + auth_policy.delete(wait=True) + for subscription in MaaSSubscription.get(client=admin_client, namespace=MAAS_SUBSCRIPTION_NAMESPACE): + subscription.delete(wait=True) yield existing_ns🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/conftest.py` around lines 1039 - 1053, The maas_subscription_namespace fixture currently reuses an existing Namespace (Namespace and MAAS_SUBSCRIPTION_NAMESPACE) without cleaning up stale MaaS objects, causing AlreadyExists or cross-test contamination; update maas_subscription_namespace so that when existing_ns.exists is true it scrubs known deterministic MaaS resources (e.g., resource names tinyllama-free-access, tinyllama-free-subscription and any MaaS CRs) using the admin_client/DynamicClient (or delete+recreate the namespace via the same create_ns flow) before yielding existing_ns; ensure the cleanup targets the specific resource kinds used by the free-tier fixtures and handles not-found errors gracefully.
🧹 Nitpick comments (2)
tests/model_serving/maas_billing/conftest.py (2)
1056-1083: Consider extracting common DSC patching logic.
maas_subscription_controller_enabled_latestduplicates the DSC patching logic frommaas_controller_enabled_latest(lines 720-750). Extract a helper or parameterize the fixture to reduce maintenance burden.♻️ Potential refactor
def _enable_maas_controller(dsc_resource: DataScienceCluster) -> Generator[DataScienceCluster, Any, Any]: """Common logic for enabling MaaS controller.""" component_patch = { DscComponents.KSERVE: {"modelsAsService": {"managementState": DscComponents.ManagementState.MANAGED}} } with ResourceEditor(patches={dsc_resource: {"spec": {"components": component_patch}}}): dsc_resource.wait_for_condition(condition="ModelsAsServiceReady", status="True", timeout=900) dsc_resource.wait_for_condition(condition="Ready", status="True", timeout=600) yield dsc_resource dsc_resource.wait_for_condition(condition="Ready", status="True", timeout=600) `@pytest.fixture`(scope="session") def maas_subscription_controller_enabled_latest( dsc_resource: DataScienceCluster, maas_postgres_prereqs: None, maas_gateway_api: None, maas_subscription_namespace: Namespace, ) -> Generator[DataScienceCluster, Any, Any]: yield from _enable_maas_controller(dsc_resource)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/conftest.py` around lines 1056 - 1083, The DSC patching logic in maas_subscription_controller_enabled_latest duplicates maas_controller_enabled_latest; extract a shared helper (e.g., _enable_maas_controller) that contains the ResourceEditor patching using DscComponents.KSERVE -> modelsAsService.managementState = DscComponents.ManagementState.MANAGED, the two wait_for_condition calls ("ModelsAsServiceReady" timeout=900 and "Ready" timeout=600) and the final post-context wait_for_condition, then replace both fixtures (maas_subscription_controller_enabled_latest and maas_controller_enabled_latest) to simply yield from _enable_maas_controller(dsc_resource) to remove duplication and centralize the logic.
846-849: Verify annotation persistence intent on existing gateway.When reusing an existing gateway,
ResourceEditorwill revert theauthorino-tls-bootstrapannotation on teardown. If this annotation should persist for production use, this could cause issues. Confirm this is the intended behavior for shared test clusters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/conftest.py` around lines 846 - 849, The test edits an existing gateway (gw) with ResourceEditor to add the "security.opendatahub.io/authorino-tls-bootstrap" annotation but ResourceEditor will revert that annotation on teardown; confirm whether the annotation must persist for production/shared clusters—if it must, change the test to either apply the annotation to a dedicated gateway resource instead of mutating a shared gw, or configure ResourceEditor to not revert (use its non-teardown/persist option) so the annotation remains; update conftest.py where ResourceEditor is invoked and reference gw and the exact annotation key when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tests/model_serving/maas_billing/component_health/test_maas_controller_health_check.py`:
- Around line 71-78: The test test_maas_controller_pods_health currently checks
all pods in the namespace; limit it to maas-controller pods by reading the
maas-controller Deployment's selector labels (fetch the Deployment named
"maas-controller" using admin_client in applications_namespace, read
spec.selector.matchLabels and convert to a label selector string like
"key1=value1,key2=value2") and pass that string as the label_selector argument
to wait_for_pods_running(admin_client=...,
namespace_name=applications_namespace, label_selector=label_selector). Ensure
wait_for_pods_running supports a label_selector parameter (if it does not, add
support to accept and forward the selector to the API call that lists pods).
- Around line 30-50: The test test_maas_controller_crds_exist currently only
checks CustomResourceDefinition(..., ensure_exists=True) and .exists; update it
to also verify each CRD's status.conditions contains an entry with type
"Established" and status "True" (e.g., inspect the CRD object from
CustomResourceDefinition or via the underlying admin_client resource and check
resource.status.conditions for type == "Established"). If the Established
condition is missing or not "True", treat that CRD as failing and append its
name to missing_crds; update the final assertion message to indicate CRDs
missing or not Established.
In `@tests/model_serving/maas_billing/conftest.py`:
- Around line 1039-1053: The maas_subscription_namespace fixture currently
reuses an existing Namespace (Namespace and MAAS_SUBSCRIPTION_NAMESPACE) without
cleaning up stale MaaS objects, causing AlreadyExists or cross-test
contamination; update maas_subscription_namespace so that when
existing_ns.exists is true it scrubs known deterministic MaaS resources (e.g.,
resource names tinyllama-free-access, tinyllama-free-subscription and any MaaS
CRs) using the admin_client/DynamicClient (or delete+recreate the namespace via
the same create_ns flow) before yielding existing_ns; ensure the cleanup targets
the specific resource kinds used by the free-tier fixtures and handles not-found
errors gracefully.
---
Nitpick comments:
In `@tests/model_serving/maas_billing/conftest.py`:
- Around line 1056-1083: The DSC patching logic in
maas_subscription_controller_enabled_latest duplicates
maas_controller_enabled_latest; extract a shared helper (e.g.,
_enable_maas_controller) that contains the ResourceEditor patching using
DscComponents.KSERVE -> modelsAsService.managementState =
DscComponents.ManagementState.MANAGED, the two wait_for_condition calls
("ModelsAsServiceReady" timeout=900 and "Ready" timeout=600) and the final
post-context wait_for_condition, then replace both fixtures
(maas_subscription_controller_enabled_latest and maas_controller_enabled_latest)
to simply yield from _enable_maas_controller(dsc_resource) to remove duplication
and centralize the logic.
- Around line 846-849: The test edits an existing gateway (gw) with
ResourceEditor to add the "security.opendatahub.io/authorino-tls-bootstrap"
annotation but ResourceEditor will revert that annotation on teardown; confirm
whether the annotation must persist for production/shared clusters—if it must,
change the test to either apply the annotation to a dedicated gateway resource
instead of mutating a shared gw, or configure ResourceEditor to not revert (use
its non-teardown/persist option) so the annotation remains; update conftest.py
where ResourceEditor is invoked and reference gw and the exact annotation key
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 19db25ae-a345-4848-b4be-d10148803946
📒 Files selected for processing (5)
tests/model_serving/maas_billing/component_health/__init__.pytests/model_serving/maas_billing/component_health/test_maas_api_health_check.pytests/model_serving/maas_billing/component_health/test_maas_controller_health_check.pytests/model_serving/maas_billing/conftest.pytests/model_serving/maas_billing/maas_subscription/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_serving/maas_billing/component_health/test_maas_api_health_check.py
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
SB159
force-pushed
the
fix/move-maas-component-health
branch
from
7f403b2 to
2f4963f
Compare
dbasunag
approved these changes
Mar 26, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/model_serving/maas_billing/maas_subscription/conftest.py (3)
354-358:⚠️ Potential issue | 🟠 MajorGive these function-scoped subscriptions unique names.
Both fixtures create resources in a shared namespace but hard-code
subscription_name. With xdist, reruns, or a stale leftover from a failed run, the next test will race or fail withAlreadyExists.Suggested change
- subscription_name="e2e-second-free-subscription", + subscription_name=f"e2e-second-free-subscription-{generate_random_name()}", @@ - subscription_name="e2e-free-actor-premium-sub", + subscription_name=f"e2e-free-actor-premium-sub-{generate_random_name()}",Also applies to: 385-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 354 - 358, The fixtures call create_maas_subscription with a hard-coded subscription_name which leads to collisions under xdist or reruns; update the fixtures that call create_maas_subscription (the ones passing "e2e-second-free-subscription" and the other hard-coded name at the other location) to generate a unique name per test run—e.g., derive the name from the pytest request node (request.node.name) or append a short uuid/timestamp—to pass as subscription_name so each invocation of create_maas_subscription produces a distinct resource and avoids AlreadyExists races.
185-195:⚠️ Potential issue | 🟠 MajorYield and revoke
maas_api_key_for_actor.Severity: major. CWE-613. This creates a live credential and returns it without any teardown. In a shared test cluster, any leaked
sk-*value remains usable after the class finishes, and repeated runs keep accumulating orphaned keys until quota or cleanup behavior starts breaking later tests. Switch this to ayieldfixture and update the annotation toGenerator[str, Any, Any].Suggested change
_, body = create_api_key( base_url=base_url, ocp_user_token=ocp_token_for_actor, request_session_http=request_session_http, api_key_name=api_key_name, request_timeout_seconds=60, ) - - return body["key"] + key_id = body["id"] + try: + yield body["key"] + finally: + revoke_resp, _ = revoke_api_key( + request_session_http=request_session_http, + base_url=base_url, + key_id=key_id, + ocp_user_token=ocp_token_for_actor, + ) + if revoke_resp.status_code not in (200, 404): + raise AssertionError(f"Unexpected teardown status for key id={key_id}: {revoke_resp.status_code}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 185 - 195, The fixture maas_api_key_for_actor currently returns a live API key without teardown; change it to a yield-style fixture (annotate as Generator[str, Any, Any]) so the test receives the key, and after yielding call the appropriate revocation helper (e.g., revoke_api_key or the code path that deletes keys) to remove body["key"]; keep the create_api_key call and api_key_name generation as-is, yield body["key"], and ensure the teardown uses the same base_url and ocp_token_for_actor to revoke the created key so no credentials are leaked.
240-243:⚠️ Potential issue | 🟠 MajorMake this cascade-deletion teardown idempotent.
Line 241 says this fixture is used for cascade deletion tests, but Lines 272-273 always delete the subscription again. If the test already removed it through the cascade path, teardown becomes the failure instead of the assertion under test.
Suggested change
- LOGGER.info(f"Fixture teardown: ensuring subscription {temporary_subscription.name} is removed") - temporary_subscription.clean_up(wait=True) + if temporary_subscription.exists: + LOGGER.info(f"Fixture teardown: ensuring subscription {temporary_subscription.name} is removed") + temporary_subscription.clean_up(wait=True)Also applies to: 272-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 240 - 243, The fixture that "Creates a temporary subscription owned by system:authenticated" currently always calls the teardown deletion (so a test that already cascaded-deleted it will fail); make the teardown idempotent by either checking that the created subscription still exists (e.g., via a get/exists call on the subscription_id) before calling delete, or by wrapping the delete call in a try/except that catches the NotFound/404 error and ignores it; update the teardown block that references the subscription variable (and the unconditional delete call) to use this existence check or exception handling so repeated deletes are safe.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/conftest.py (1)
170-177: Remove the hidden controller setup from this API-key fixture.This fixture only creates a key through the MaaS API, but Line 174 still forces
maas_subscription_controller_enabled_latestfor every caller. That keeps the child conftest coupled to the cluster-wide managed-mode setup and makes simple API-key tests pay for unrelated state changes. If a test genuinely needs managed mode, declare that fixture on the test/class instead of hiding it here.As per coding guidelines, "Code reuse, test parameterization, proper test dependency should be also encouraged".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 170 - 177, The fixture maas_api_key_for_actor currently forces the managed-mode controller by including the maas_subscription_controller_enabled_latest parameter; remove that hidden dependency by deleting maas_subscription_controller_enabled_latest from the maas_api_key_for_actor fixture signature and body (and any direct references to it) so the fixture simply creates an API key via the MaaS API using request_session_http/base_url/ocp_token_for_actor; if tests need managed-mode they should declare maas_subscription_controller_enabled_latest explicitly in their test or class instead of relying on this fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 354-358: The fixtures call create_maas_subscription with a
hard-coded subscription_name which leads to collisions under xdist or reruns;
update the fixtures that call create_maas_subscription (the ones passing
"e2e-second-free-subscription" and the other hard-coded name at the other
location) to generate a unique name per test run—e.g., derive the name from the
pytest request node (request.node.name) or append a short uuid/timestamp—to pass
as subscription_name so each invocation of create_maas_subscription produces a
distinct resource and avoids AlreadyExists races.
- Around line 185-195: The fixture maas_api_key_for_actor currently returns a
live API key without teardown; change it to a yield-style fixture (annotate as
Generator[str, Any, Any]) so the test receives the key, and after yielding call
the appropriate revocation helper (e.g., revoke_api_key or the code path that
deletes keys) to remove body["key"]; keep the create_api_key call and
api_key_name generation as-is, yield body["key"], and ensure the teardown uses
the same base_url and ocp_token_for_actor to revoke the created key so no
credentials are leaked.
- Around line 240-243: The fixture that "Creates a temporary subscription owned
by system:authenticated" currently always calls the teardown deletion (so a test
that already cascaded-deleted it will fail); make the teardown idempotent by
either checking that the created subscription still exists (e.g., via a
get/exists call on the subscription_id) before calling delete, or by wrapping
the delete call in a try/except that catches the NotFound/404 error and ignores
it; update the teardown block that references the subscription variable (and the
unconditional delete call) to use this existence check or exception handling so
repeated deletes are safe.
---
Nitpick comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 170-177: The fixture maas_api_key_for_actor currently forces the
managed-mode controller by including the
maas_subscription_controller_enabled_latest parameter; remove that hidden
dependency by deleting maas_subscription_controller_enabled_latest from the
maas_api_key_for_actor fixture signature and body (and any direct references to
it) so the fixture simply creates an API key via the MaaS API using
request_session_http/base_url/ocp_token_for_actor; if tests need managed-mode
they should declare maas_subscription_controller_enabled_latest explicitly in
their test or class instead of relying on this fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a71f1eef-aa52-4289-8e95-24b8b59990e6
📒 Files selected for processing (5)
tests/model_serving/maas_billing/component_health/__init__.pytests/model_serving/maas_billing/component_health/test_maas_api_health_check.pytests/model_serving/maas_billing/component_health/test_maas_controller_health_check.pytests/model_serving/maas_billing/conftest.pytests/model_serving/maas_billing/maas_subscription/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_serving/maas_billing/conftest.py
|
Status of building tag latest: success. |
Contributor
Author
|
/cherry-pick 3.4ea2 |
rhods-ci-bot
pushed a commit
that referenced
this pull request
Mar 27, 2026
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> Co-authored-by: Shelton Cyril <sheltoncyril@gmail.com>
Contributor
|
Cherry pick action created PR #1315 successfully 🎉! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Pull Request
Summary
move maas component_health under maas_billing
Related Issues
Please review and indicate how it has been tested
Additional Requirements
Summary by CodeRabbit