RHOAIENG-32026: remove duplication for mysql commands#606
RHOAIENG-32026: remove duplication for mysql commands#606dbasunag merged 3 commits intoopendatahub-io:mainfrom
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaces direct MySQL CLI invocations in negative tests with two new helper utilities: one that writes a MySQL credentials file onto the DB Pod and one that runs SQL via that credentials file; conftest now uses these helpers to read the latest migration version and mark it dirty. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✨ Finishing touches
🧪 Generate unit tests
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{'/cherry-pick', '/lgtm', '/verified', '/hold', '/wip', '/build-push-pr-image'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/model_registry/negative_tests/conftest.py (3)
28-28: Fix typo in imported helper name.It should be create_mysql_credentials_file for clarity and consistency.
-from tests.model_registry.negative_tests.utils import execute_mysql_command, crate_mysql_credentials_file +from tests.model_registry.negative_tests.utils import execute_mysql_command, create_mysql_credentials_file
143-148: Make result parsing deterministic (use mysql -N -B) and simplify cast.With -N -B (proposed in utils comment), the output will be a single line number. Parse it directly.
- crate_mysql_credentials_file(model_registry_db_instance_pod=model_registry_db_instance_pod) - output = execute_mysql_command( + create_mysql_credentials_file(model_registry_db_instance_pod=model_registry_db_instance_pod) + output = execute_mysql_command( sql_query="SELECT version FROM schema_migrations ORDER BY version DESC LIMIT 1;", model_registry_db_instance_pod=model_registry_db_instance_pod, ) - latest_migration_version = int(output.strip().split()[1]) + latest_migration_version = int(output.strip())
149-152: Address Ruff S608 (string-built SQL) and harden.The value is already int-sanitized; annotate to silence S608 and avoid false positives.
- execute_mysql_command( - sql_query=f"UPDATE schema_migrations SET dirty = 1 WHERE version = {latest_migration_version};", + execute_mysql_command( + sql_query=f"UPDATE schema_migrations SET dirty = 1 WHERE version = {latest_migration_version};", # noqa: S608 - latest_migration_version is int-sanitized model_registry_db_instance_pod=model_registry_db_instance_pod, )Optionally, add a quick guard to fail fast if the SELECT returned empty output to avoid ValueError on int().
tests/model_registry/negative_tests/utils.py (3)
6-6: Rename helper to ‘create_mysql_credentials_file’.Fix the typo to improve readability; update imports/callers accordingly (see conftest diff).
-def crate_mysql_credentials_file(model_registry_db_instance_pod: Pod) -> None: +def create_mysql_credentials_file(model_registry_db_instance_pod: Pod) -> None:
16-18: Set restrictive permissions on the credentials file.Ensure the file isn’t world-readable. Use umask 077 during creation.
- model_registry_db_instance_pod.execute( - command=["bash", "-c", f"echo '{b64_content}' | base64 --decode > /tmp/.my.cnf"] - ) + model_registry_db_instance_pod.execute( + command=["bash", "-c", f"umask 077 && echo '{b64_content}' | base64 -d > /tmp/.my.cnf"] + )Optional: add a small helper to delete /tmp/.my.cnf in a test finalizer to reduce credential residue.
1-33: Minor cleanup: centralize the credentials path.Define a module-level constant (e.g., CREDENTIALS_PATH = "/tmp/.my.cnf") and reuse it in both helpers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/negative_tests/conftest.py(2 hunks)tests/model_registry/negative_tests/utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-09T08:50:22.172Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Applied to files:
tests/model_registry/negative_tests/conftest.py
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/negative_tests/conftest.py
🧬 Code graph analysis (2)
tests/model_registry/negative_tests/conftest.py (1)
tests/model_registry/negative_tests/utils.py (2)
execute_mysql_command(21-33)crate_mysql_credentials_file(6-18)
tests/model_registry/negative_tests/utils.py (1)
tests/model_registry/negative_tests/conftest.py (1)
model_registry_db_instance_pod(157-164)
🪛 Ruff (0.12.2)
tests/model_registry/negative_tests/conftest.py
150-150: Possible SQL injection vector through string-based query construction
(S608)
| import base64 | ||
|
|
||
|
|
||
| def crate_mysql_credentials_file(model_registry_db_instance_pod: Pod) -> None: |
There was a problem hiding this comment.
Hey, is this a typo? ("crate" instead of "create")
There was a problem hiding this comment.
Thank you for catching it. @fege this looks like a typo.
| import base64 | ||
|
|
||
|
|
||
| def crate_mysql_credentials_file(model_registry_db_instance_pod: Pod) -> None: |
There was a problem hiding this comment.
Thank you for catching it. @fege this looks like a typo.
|
Status of building tag latest: success. |
…#606) * change: RHOAIENG-32026 remove duplication for mysql commands * Update conftest.py * Update utils.py --------- Co-authored-by: Shelton Cyril <sheltoncyril@gmail.com>
4 participants
Description
Address comments of previous PRs
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit