test(maas): update subscription tests with new API key changes#1177
Merged
threcc merged 11 commits intoopendatahub-io:mainfrom Mar 9, 2026
Merged
test(maas): update subscription tests with new API key changes#1177threcc merged 11 commits intoopendatahub-io:mainfrom
threcc merged 11 commits intoopendatahub-io:mainfrom
Conversation
SB159
requested review from
Raghul-M,
brettmthompson,
mwaykole and
threcc
as code owners
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/build-push-pr-image', '/hold', '/lgtm', '/wip', '/verified'} |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds API-key based auth for MaaS gateway tests: new fixtures create plaintext MaaS API keys from OpenShift tokens and provide header dicts; a helper POST function to create API keys is added; tests switch from token-based header construction to the new API-key header fixture and adjust some test markers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security findings (actionable)
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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: 1
🧹 Nitpick comments (3)
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py (1)
102-102: Remove commented-out dead code.- # ocp_token_for_actor: str,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py` at line 102, Remove the dead commented-out parameter line "ocp_token_for_actor: str," from the test file (found in tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py) — locate the commented line near the test function(s) that reference multiple subscriptions and simply delete the commented code to clean up the test; no behavior changes are needed.tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py (1)
1-1: Remove or clarify vague# temporarycomment.This comment provides no context. Either remove it or explain what is temporary and why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py` at line 1, The lone vague comment "# temporary" in conftest.py should be removed or replaced with a clear explanatory TODO: either delete it if nothing is temporary, or replace it with a descriptive comment explaining what is temporary, why, expected lifespan, and next action (e.g., "TODO: temporary stub for X until Y is implemented — remove after PR `#123`; owner: `@username`; created: YYYY-MM-DD"). Ensure the updated comment appears near the related test/setup code in conftest.py so future readers understand context.tests/model_serving/model_server/maas_billing/maas_subscription/utils.py (1)
210-214: UseLOGGER.exceptionto preserve traceback on JSON decode failure.
LOGGER.errordiscards the exception context. Switch toLOGGER.exceptionso the traceback is logged, aiding debugging when the API returns malformed responses.Proposed fix
try: parsed_body: dict[str, Any] = json.loads(response.text) except json.JSONDecodeError: - LOGGER.error(f"Unable to parse API key response: {response.text}") + LOGGER.exception(f"Unable to parse API key response: {response.text}") parsed_body = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/utils.py` around lines 210 - 214, The JSON decode failure handler in the try/except around json.loads(response.text) currently calls LOGGER.error and loses exception context; change LOGGER.error(f"Unable to parse API key response: {response.text}") to LOGGER.exception("Unable to parse API key response", exc_info=True) or simply LOGGER.exception(f"Unable to parse API key response: {response.text}") in the except block so the traceback is preserved when parsing in this function (the block that sets parsed_body: dict[str, Any] = json.loads(response.text)); leave parsed_body = {} behavior intact.
🤖 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/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py`:
- Line 145: Replace the `@pytest.mark.smoke` marker used on the MaaS subscription
access-control tests in this test class with `@pytest.mark.sanity` because these
tests validate specific subscription behavior (service account authorization)
rather than core build-stability functionality; locate the pytest decorators in
the test class and change each `@pytest.mark.smoke` to `@pytest.mark.sanity` for the
tests that verify unauthorized subscription usage (and the other similar tests
in the same class).
---
Nitpick comments:
In `@tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py`:
- Line 1: The lone vague comment "# temporary" in conftest.py should be removed
or replaced with a clear explanatory TODO: either delete it if nothing is
temporary, or replace it with a descriptive comment explaining what is
temporary, why, expected lifespan, and next action (e.g., "TODO: temporary stub
for X until Y is implemented — remove after PR `#123`; owner: `@username`; created:
YYYY-MM-DD"). Ensure the updated comment appears near the related test/setup
code in conftest.py so future readers understand context.
In
`@tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py`:
- Line 102: Remove the dead commented-out parameter line "ocp_token_for_actor:
str," from the test file (found in
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.py)
— locate the commented line near the test function(s) that reference multiple
subscriptions and simply delete the commented code to clean up the test; no
behavior changes are needed.
In `@tests/model_serving/model_server/maas_billing/maas_subscription/utils.py`:
- Around line 210-214: The JSON decode failure handler in the try/except around
json.loads(response.text) currently calls LOGGER.error and loses exception
context; change LOGGER.error(f"Unable to parse API key response:
{response.text}") to LOGGER.exception("Unable to parse API key response",
exc_info=True) or simply LOGGER.exception(f"Unable to parse API key response:
{response.text}") in the except block so the traceback is preserved when parsing
in this function (the block that sets parsed_body: dict[str, Any] =
json.loads(response.text)); leave parsed_body = {} behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 19640a29-bc47-4c0a-a5b8-88c3bfee54ff
📒 Files selected for processing (3)
tests/model_serving/model_server/maas_billing/maas_subscription/conftest.pytests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.pytests/model_serving/model_server/maas_billing/maas_subscription/utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py (1)
1-15:⚠️ Potential issue | 🔴 CriticalCritical: Import
AnyandGeneratorfromtypingbefore pytest imports this module.Annotations at lines 35, 63, 89, 111, 134, 159, 181, 210, and 315 use
GeneratorandAnyfrom thetypingmodule, but neither is imported. Python evaluates type annotations during module import, causing test collection to fail withNameError: name 'Generator' is not defined.Fix
+from typing import Any, Generator + import pytest import requests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py` around lines 1 - 15, The module-level type annotations use Generator and Any but those names are not imported, causing import-time NameError during pytest collection; fix by adding an import for them (e.g. from typing import Any, Generator) at the top of conftest.py before any pytest-related imports or, alternatively, enable postponed evaluation of annotations (from __future__ import annotations) — ensure the import appears before fixtures/functions that reference Generator/Any so annotations resolve during module import (look for the fixture and function annotations in this file where Generator and Any are used).
🤖 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/model_server/maas_billing/maas_subscription/conftest.py`:
- Around line 284-298: The fixture leaks plaintext credentials by logging
response.text, printing the full response body on assertion failure, and
emitting an 8-char API key prefix; update the code around LOGGER.info, the
assertion that prints body, and the api_key handling so that secrets are never
logged: stop using response.text or body in logs or assertion messages, validate
api_key with isinstance(api_key, str) and api_key.startswith("sk-") but on
failure only include non-sensitive metadata (e.g., response.status_code and safe
indicators like body present/absent or key length), and change the final
LOGGER.info to redact the key (e.g., log only "created api-key
name={api_key_name} key_redacted" or a fixed-length masked string) while keeping
the variable names api_key, body, response, LOGGER, and api_key_name for
locating the changes.
---
Outside diff comments:
In `@tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py`:
- Around line 1-15: The module-level type annotations use Generator and Any but
those names are not imported, causing import-time NameError during pytest
collection; fix by adding an import for them (e.g. from typing import Any,
Generator) at the top of conftest.py before any pytest-related imports or,
alternatively, enable postponed evaluation of annotations (from __future__
import annotations) — ensure the import appears before fixtures/functions that
reference Generator/Any so annotations resolve during module import (look for
the fixture and function annotations in this file where Generator and Any are
used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: aaf2117f-b1a4-4e8c-8ef4-43a1f8633e63
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/model_server/maas_billing/maas_subscription/conftest.py`:
- Around line 279-285: The create_api_key() function currently logs the full
response.text (including plaintext API keys) on success and error; update the
logging in create_api_key() to avoid leaking credentials by logging only the
HTTP status code and a redacted body—parse response.json() (if JSON), replace
the api key field (e.g., keys named "key" or values matching /^sk-/) with a
fixed token like "<REDACTED>", and use LOGGER.info/LOGGER.error to output the
status and the redacted JSON instead of response.text; apply the same redaction
logic for both the success path and the error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f2b83775-631a-47f6-9309-802934bb36cd
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/maas_subscription/conftest.py
threcc
previously approved these changes
Mar 6, 2026
dbasunag
requested changes
Mar 7, 2026
auto-merge was automatically disabled
March 9, 2026 15:40
Head branch was pushed to by a user without write access
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/model_serving/model_server/maas_billing/maas_subscription/utils.py (1)
201-205: UseLOGGER.exception()to capture traceback in error handler.When catching an exception,
exception()automatically includes the stack trace which aids debugging JSON parse failures.Proposed fix
try: parsed_body: dict[str, Any] = json.loads(response.text) except json.JSONDecodeError: - LOGGER.error(f"Unable to parse API key response: {response.text}") + LOGGER.exception("Unable to parse API key response") parsed_body = {}Note: Also avoid logging
response.texthere as it may contain sensitive data on partial success scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/model_server/maas_billing/maas_subscription/utils.py` around lines 201 - 205, The JSON parse error handler in utils.py uses LOGGER.error and logs response.text; change this to call LOGGER.exception("Unable to parse API key response") so the traceback is captured, and remove logging of response.text to avoid exposing sensitive data; keep the except block setting parsed_body = {} and reference the json.loads(response.text) call and the except json.JSONDecodeError handler to locate the change.
🤖 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/model_server/maas_billing/maas_subscription/utils.py`:
- Line 199: The current LOGGER.info in create_api_key logs the full
response.text (which contains the plaintext API key) exposing credentials;
change the logging in the create_api_key function to never include response.text
or any raw credential, instead log only non-sensitive metadata (e.g.,
api_keys_url and response.status_code) and if you must surface a key, log a
masked form (e.g., show only last 4 chars) or an existence boolean; reference
LOGGER, create_api_key, api_keys_url and response when making this change.
---
Nitpick comments:
In `@tests/model_serving/model_server/maas_billing/maas_subscription/utils.py`:
- Around line 201-205: The JSON parse error handler in utils.py uses
LOGGER.error and logs response.text; change this to call
LOGGER.exception("Unable to parse API key response") so the traceback is
captured, and remove logging of response.text to avoid exposing sensitive data;
keep the except block setting parsed_body = {} and reference the
json.loads(response.text) call and the except json.JSONDecodeError handler to
locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 87383e10-b3a3-4657-92c0-9174efdb995a
📒 Files selected for processing (2)
tests/model_serving/model_server/maas_billing/maas_subscription/test_multiple_subscriptions_per_model.pytests/model_serving/model_server/maas_billing/maas_subscription/utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/model_server/maas_billing/maas_subscription/utils.py`:
- Around line 189-207: The current code swallows non-JSON or error responses and
returns (response, {}) which lets fixtures proceed without a valid API key;
update the post-call handling (around the POST to api_keys_url and the
parsed_body/result return) to: check response.status_code for success (e.g. 2xx)
and attempt to parse JSON, and if parsing fails or parsed_body does not contain
a usable parsed_body["key"], log an error with response.status_code and
response.text and raise an exception (or return an explicit failure) instead of
returning (response, {}); reference the variables and logger in this block
(request_session_http.post, api_keys_url, ocp_user_token, api_key_name, LOGGER,
parsed_body, response) so callers fail fast when key creation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 65c01054-29b9-4168-acfa-de704f299542
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/maas_subscription/utils.py
dbasunag
requested changes
Mar 9, 2026
for more information, see https://pre-commit.ci
dbasunag
approved these changes
Mar 9, 2026
threcc
approved these changes
Mar 9, 2026
|
Status of building tag latest: success. |
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
Updates maas multiple subscription tests to use the new API key based authentication flow.
Changes include:
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit