Add tests for postgres db#1098
Conversation
|
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:
📝 WalkthroughWalkthroughGeneralizes model-registry tests and helpers from MySQL-only to multi-backend external DB support (mysql/mariadb and postgres): adds Postgres port, renames/refactors DB helpers and fixtures (e.g., Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/wip', '/hold', '/cherry-pick', '/verified', '/build-push-pr-image'} |
|
/build-push-pr-image |
|
Status of building tag pr-1098: success. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/model_registry/model_registry/rest_api/test_model_registry_secure_db.py (2)
32-46:⚠️ Potential issue | 🟡 Minor
ca_mount_path: "/etc/mysql/ssl"is MySQL-specific but will be used for PostgreSQL tests too.Since the class is now parameterized over both
mysqlandpostgresbackends, using a hardcoded/etc/mysql/sslmount path for the CA in both test methods (lines 38 and 87) will produce a misleading MySQL-centric path inside a PostgreSQL container. Consider parameterizing the mount path based on the backend or using a backend-neutral path like/etc/db/ssl.
103-130:⚠️ Potential issue | 🟡 MinorMissing docstring on
test_register_model_with_valid_ca.As per coding guidelines, every test must have a docstring explaining what it tests, preferably in Given-When-Then format.
Suggested docstring
def test_register_model_with_valid_ca( self: Self, admin_client: DynamicClient, model_registry_namespace: str, model_registry_rest_headers: dict[str, str], local_ca_bundle: str, deploy_secure_db_mr: ModelRegistry, model_data_for_test: dict[str, Any], db_backend_under_test: str, ): + """ + Test that model registration succeeds when the Model Registry is deployed + with a valid CA certificate for the secure database backend. + + Given: A Model Registry deployed with a valid SSL CA for the db_backend_under_test. + When: A model is registered via the REST API using the valid CA bundle. + Then: The registration succeeds and the returned model attributes match the expected data. + """ service = get_mr_service_by_label(As per coding guidelines:
tests/**/*.py: Every test MUST have a docstring explaining what it tests.tests/model_registry/utils.py (1)
525-563:⚠️ Potential issue | 🟡 MinorSimplify annotation template to match codebase pattern.
The f-string is unnecessarily complex for a template string with only literal characters. Other instances in the codebase (conftest.py:75, rbac/multiple_instance_utils.py:43) use a plain raw string:
r"mysql://{.spec.clusterIP}:{.spec.ports[?(.name==\mysql\)].port}". Change line 536 to use the same pattern but parameterized for the dynamicservice_port_name:service_uri = rf"{service_port_name}://{{{.spec.clusterIP}}}:{{{.spec.ports[?(.name==\{service_port_name}\)].port}}}"Or refactor to a plain raw string with format substitution:
service_uri = f"{service_port_name}://{{.spec.clusterIP}}:{{.spec.ports[?(.name==\\{service_port_name}\\)].port}}"Note: The backslash escapes in the JSONPath filter (e.g.,
\{,\)) are consistent with existing usages in the codebase, though they should be verified against OpenShift template syntax specifications.
🤖 Fix all issues with AI agents
In `@tests/model_registry/model_registry/rest_api/conftest.py`:
- Around line 135-173: external_db_template_with_ca currently builds
postgres_ssl_args and calls
db_deployment_template["spec"]["containers"][0].get("args", []).extend(...),
which mutates a temporary list and never writes it back; fix by ensuring the
container has an "args" list before extending it (e.g., use container =
db_deployment_template["spec"]["containers"][0]; container.setdefault("args",
[]) and then extend with postgres_ssl_args) so the args are persisted in the
template returned by external_db_template_with_ca (refer to
get_model_registry_deployment_template_dict, CA_FILE_PATH, CA_MOUNT_PATH,
CA_CONFIGMAP_NAME).
- Around line 286-289: The code uses next(container for container in
spec["containers"] if container["name"] == db_backend_under_test) which will
raise StopIteration before the subsequent assert can run; change to use
next((container for container in spec["containers"] if container["name"] ==
db_backend_under_test), None) so db_container may be None and the assert
db_container is not None, f"{db_backend_under_test} container not found" will
produce the intended error message; update the reference where db_container is
defined (in the block handling
model_registry_db_deployments[0].instance.to_dict()) accordingly.
- Around line 148-163: The PostgreSQL SSL file paths in the fixture
(postgres_ssl_args in tests/model_registry/model_registry/rest_api/conftest.py)
differ from those used in apply_mysql_args_and_volume_mounts in
tests/model_registry/utils.py; update one side so both use the same paths (e.g.,
set postgres_ssl_args entries ssl_cert_file, ssl_key_file, and ssl_ca_file to
the same /etc/ssl-certs, /etc/ssl-keys, /etc/ssl-ca paths used by
apply_mysql_args_and_volume_mounts or vice versa) and ensure the final values
are applied to db_deployment_template["spec"]["containers"][0]["args"]
consistently.
In `@tests/model_registry/utils.py`:
- Around line 42-45: The POSTGRES_DB_IMAGE constant currently uses "postgres:16"
instead of the commented RHEL image; restore the RHEL image value by setting
POSTGRES_DB_IMAGE to the original RHEL URI (the removed sha256 pinned string)
or, if that image fails in CI, add an explicit fallback mechanism and a clear
TODO referencing an issue/bug ID: e.g., set POSTGRES_DB_IMAGE to the RHEL URI
from the commented line, or set it from an environment variable with the RHEL
URI as the default, and add a short comment that documents the CI failure and
links the issue number so the RHEL image replacement is tracked (refer to
POSTGRES_DB_IMAGE to locate and change the constant).
🧹 Nitpick comments (3)
tests/model_registry/utils.py (1)
391-466: Function nameapply_mysql_args_and_volume_mountsis misleading now that it handles both MySQL and PostgreSQL.The function body branches on
db_backendto support both MySQL and PostgreSQL, but the name still says "mysql". This should be renamed for consistency with the other renamed functions (e.g.,add_db_certs_volumes_to_deployment,get_external_db_config).Suggested rename
-def apply_mysql_args_and_volume_mounts( +def apply_db_args_and_volume_mounts( db_container: dict[str, Any], ca_configmap_name: str, ca_mount_path: str, db_backend: str, ) -> dict[str, Any]: """ - Applies the database args and volume mounts to the database container. + Applies backend-specific SSL args and volume mounts to the database container.Note: This rename must also be applied to the import in
conftest.py(line 17) and any other callers.tests/model_registry/model_registry/rest_api/conftest.py (2)
321-362: SSL secrets use manualcreate()/delete()instead of context managers for lifecycle management.Per coding guidelines, Kubernetes resource lifecycle should use context managers to ensure cleanup. The
create_k8s_secretutility calls.create()directly and the teardown relies on manual.delete()calls. If an error occurs between creation and teardown, secrets may leak.Consider wrapping in context managers or using the
withpattern. Ifcreate_k8s_secretcan't be changed, this is a deferred improvement.As per coding guidelines:
tests/**/conftest.py: Use context managers for resource lifecycle management in fixtures.
268-307: Fixture namepatch_external_deployment_with_ssl_causes a verb — guideline recommends nouns.Similarly,
deploy_secure_db_mr(line 177) uses a verb. Consider noun-based names likeexternal_deployment_with_ssl_caandsecure_db_mrrespectively.As per coding guidelines:
tests/**/conftest.py: Fixture names MUST be nouns:storage_secretnotcreate_secret.
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 (4)
tests/model_registry/utils.py (1)
570-570:⚠️ Potential issue | 🟡 MinorIncorrect return type annotation:
list[Service]should belist[ConfigMap].
get_mr_configmap_objectsreturns a list ofConfigMapobjects, notService.Proposed fix
-) -> list[Service]: +) -> list[ConfigMap]:tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py (1)
152-169:⚠️ Potential issue | 🟡 MinorMissing docstring on
test_default_postgres_db_pod_log.Per coding guidelines, every test must have a docstring explaining what it tests. This new test validates the default Postgres pod log contains the expected connection string but lacks a docstring.
Proposed fix
def test_default_postgres_db_pod_log( self: Self, skip_if_not_default_db: None, admin_client: DynamicClient, model_registry_namespace: str, model_registry_default_postgres_deployment_match_label: dict[str, str], ): + """ + Test that the default postgres pod log contains the expected connection string, + confirming the database is accepting connections. + """ label_selector = ",".join([tests/model_registry/model_registry/rest_api/test_model_registry_secure_db.py (2)
103-130:⚠️ Potential issue | 🟡 MinorMissing docstring on
test_register_model_with_valid_ca.Per coding guidelines, every test must have a docstring. This test validates successful model registration with a valid CA certificate over a secure DB connection.
35-43:⚠️ Potential issue | 🟡 MinorUse a backend-neutral mount path instead of
/etc/mysql/ssl.The parametrization applies to both MySQL and PostgreSQL backends (via class-level
db_backend_under_test), but the mount path is hardcoded with "mysql". Theadd_db_container_certificates()function inutils.pyuses the sameca_mount_pathfor both backends without differentiating, and volumeMounts apply identically regardless of backend type. While the path itself is functionally correct for both, the naming is misleading. Consider/etc/db/sslor/etc/sslto reflect its backend-agnostic usage.
🤖 Fix all issues with AI agents
In `@tests/model_registry/model_registry/rest_api/conftest.py`:
- Around line 283-284: The local variables CA_CONFIGMAP_NAME and CA_MOUNT_PATH
in the fixture are shadowing the module-level constants imported from
tests.model_registry.constants; rename the local variables to lowercase (e.g.,
ca_configmap_name and ca_mount_path) in the fixture that reads request.param,
and update all subsequent references (the uses of CA_CONFIGMAP_NAME and
CA_MOUNT_PATH later in that function) to the new lowercase names so the
module-level imports remain unshadowed and intent is clear.
🧹 Nitpick comments (3)
tests/model_registry/utils.py (1)
388-393: Function nameapply_mysql_args_and_volume_mountsis misleading — it now handles Postgres too.The function was generalized to support both MySQL and PostgreSQL backends via the
db_backendparameter, but the name still says "mysql". Consider renaming toapply_db_args_and_volume_mountsfor consistency with the rest of the refactor (e.g.,add_db_certs_volumes_to_deployment).tests/model_registry/model_registry/rest_api/conftest.py (2)
177-183: Fixture namedeploy_secure_db_mris verb-based.As per coding guidelines, fixture names should be nouns (e.g.,
secure_db_model_registry), not verbs/actions. Similarly,patch_external_deployment_with_ssl_ca(line 268) is verb-based. As per coding guidelines: "Fixture names MUST be nouns:storage_secretnotcreate_secret".
321-362:external_db_ssl_secretsmanually deletes secrets instead of using a context manager.The fixture creates secrets via
create_k8s_secretand manually deletes them in teardown. Per coding guidelines, resource lifecycle should use context managers to ensure cleanup. Ifcreate_k8s_secretdoesn't support the context manager protocol, this may be a limitation, but it's worth noting.
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 (1)
tests/model_registry/utils.py (1)
565-572:⚠️ Potential issue | 🟡 MinorIncorrect return type annotation:
list[Service]should belist[ConfigMap].The function returns a list of
ConfigMapobjects, but the return type is annotated aslist[Service].Proposed fix
def get_mr_configmap_objects( base_name: str, namespace: str, client: DynamicClient, teardown_resources: bool, num: int, db_backend: str, -) -> list[Service]: +) -> list[ConfigMap]:
🧹 Nitpick comments (3)
tests/model_registry/utils.py (3)
390-395: Function nameapply_mysql_args_and_volume_mountsis misleading — it now handles Postgres too.The function was extended to support the
postgresbackend (line 432), but the name still says "mysql". Consider renaming to something backend-agnostic likeapply_db_args_and_volume_mountsfor consistency with the other refactored helpers (e.g.,add_db_certs_volumes_to_deployment).
88-115: New backend-dispatch helpers look good overall.The factored-out helpers (
get_database_volumes,get_database_volume_mounts,get_database_image,get_database_health_probes,get_database_env_vars) cleanly separate backend-specific concerns. One minor observation: thedb_backendstring is checked via raw string comparisons ("postgres","mariadb","mysql") throughout. If a typo or unexpected value is passed, these functions silently fall through to the MySQL default. Consider adding a guard or using an enum/constant set for the valid backend values to catch misconfiguration early.Also applies to: 118-142, 145-153, 156-209, 212-258
353-387:add_db_certs_volumes_to_deploymentsilently no-ops formariadbbackend.The function handles
"mysql"and"postgres"but has no branch for"mariadb". If MariaDB SSL tests are added later, this will silently return the original volumes without any cert mounts. A brief comment noting this is intentional (or aValueErrorfor unsupported backends) would prevent confusion.
|
/build-push-pr-image |
|
Status of building tag pr-1098: success. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/model_registry/model_registry/rest_api/conftest.py`:
- Line 284: The fixture currently sets a MySQL-specific default ca_mount_path
("/etc/mysql/ssl") which is incorrect for other backends; change the assignment
to inspect request.param.get("backend") and choose a backend-appropriate default
(e.g., use "/etc/mysql/ssl" for mysql, "/etc/postgres/ssl" for postgres) or fall
back to a neutral path like "/etc/ssl/certs" (or None) when backend is
unspecified, by replacing the direct default of ca_mount_path with logic that
derives it from request.param and assigning to ca_mount_path accordingly.
- Around line 288-289: The assertion is wrong because db_containers is a list
(never None); change the check to assert that the list is non-empty (e.g.,
assert db_containers or len(db_containers) > 0) and include a clear message
using db_backend_under_test so failures show which backend was not found; update
the code around the db_containers = [container for container in
spec["containers"] if container["name"] == db_backend_under_test] line and the
following assertion to validate emptiness instead of None and provide a helpful
error message.
🧹 Nitpick comments (1)
tests/model_registry/model_registry/rest_api/conftest.py (1)
330-362: Consider using context managers for secret lifecycle management.The secrets are created with
create_k8s_secretand manually deleted in teardown. Per coding guidelines, fixtures should "use context managers for resource lifecycle management." If the Secret resource supportswith(asConfigMapdoes elsewhere in this file), that would simplify cleanup and ensure it happens even on exceptions.
|
/build-push-pr-image |
|
Status of building tag pr-1098: success. |
|
/build-push-pr-image |
|
Status of building tag pr-1098: success. |
|
/build-push-pr-image |
|
Status of building tag pr-1098: success. |
|
Status of building tag latest: success. |
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Tests
Chores