test: add qdrant vector_io tests#1029
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/lgtm', '/cherry-pick', '/wip', '/verified', '/hold'} |
📝 WalkthroughWalkthroughAdds Qdrant vector-I/O support and moves multiple test deployments to secret-backed credentials: new QDRANT env constants, Qdrant Deployment/Service/Secret fixtures, provider config using secretKeyRef for credentials, Postgres secret integration, plus related test and README updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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
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 |
Bobbins228
force-pushed
the
qdrant-tests
branch
from
5d2eb18 to
33cdf59
Compare
📝 WalkthroughWalkthroughThe changes add Qdrant remote vector store integration support to test fixtures and test cases. New Kubernetes fixtures deploy Qdrant instances with associated secrets and services. A parameterized test case is added to verify the qdrant-remote provider functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/fixtures/vector_io.py (1)
83-115: Broaden env var typing to allowvalueFromentries.Line 108-115 introduces a nested dict (
valueFrom), but Line 83 definesenv_varsaslist[dict[str, str]]. This will fail mypy strict. Widen the type (and the factory return type) todict[str, Any].🔧 Suggested fix
- def _factory(provider_name: str) -> list[Dict[str, str]]: - env_vars: list[dict[str, str]] = [] + def _factory(provider_name: str) -> list[dict[str, Any]]: + env_vars: list[dict[str, Any]] = []tests/llama_stack/vector_io/test_vector_stores.py (1)
57-70: Add the component marker for Llama Stack tests.This file now includes a new qdrant-remote case but still lacks the required component marker. Please add
@pytest.mark.llama_stackat the class (or module) level to satisfy the marker guidelines.✅ Suggested change
`@pytest.mark.rag` +@pytest.mark.llama_stack class TestLlamaStackVectorStores:
🤖 Fix all issues with AI agents
In `@tests/fixtures/vector_io.py`:
- Around line 370-434: Update the three fixture docstrings (qdrant_deployment,
qdrant_service, qdrant_secret) to Google-style by adding an "Args:" section
listing each fixture parameter (unprivileged_client,
unprivileged_model_namespace, and qdrant_secret where applicable) and a
"Returns:" section describing the yielded object type (Deployment for
qdrant_deployment, Service for qdrant_service, Secret for qdrant_secret); keep
the brief one-line summary at the top and ensure indentation and blank lines
follow Google docstring conventions.
- Around line 371-417: Ruff flags unused fixture parameters in qdrant_deployment
and qdrant_service; silence ARG001 by explicitly marking those fixture args as
used. Inside qdrant_deployment add a no-op reference like "_ = qdrant_secret"
(or use "assert qdrant_secret is not None") and inside qdrant_service add "_ =
unprivileged_client" (or "assert unprivileged_client is not None") so the
fixture dependencies remain while Ruff stops reporting them as unused.
- Around line 442-466: The pod spec sets securityContext.runAsNonRoot = True
which is incompatible with the default Qdrant image (container "qdrant" using
QDRANT_IMAGE) that runs as root; fix by either switching to an unprivileged
Qdrant image variant (e.g., qdrant:*-unprivileged), or explicitly set
securityContext.runAsUser = 1000 for the qdrant container, or remove
runAsNonRoot: True—apply the chosen change within the qdrant container spec (the
"containers" entry for "qdrant") so the pod can start successfully.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/fixtures/vector_io.pytests/llama_stack/vector_io/test_vector_stores.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Every test MUST have a docstring explaining what it tests
Apply relevant markers frompytest.ini: tier (smoke,sanity,tier1,tier2), component (model_serving,model_registry,llama_stack), infrastructure (gpu,parallel,slow)
Use Given-When-Then format in test docstrings for behavioral clarity
Use openshift-python-wrapper for all Kubernetes API calls
Kubernetes resource lifecycle MUST use context managers to ensure cleanup
Add type annotations to test code and fixtures (mypy strict enforced)
Write Google-format docstrings for tests and fixtures
Files:
tests/llama_stack/vector_io/test_vector_stores.pytests/fixtures/vector_io.py
tests/**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Do not import heavy resources at module level; defer to fixture scope
Files:
tests/llama_stack/vector_io/test_vector_stores.py
🧠 Learnings (1)
📚 Learning: 2026-01-16T10:17:22.830Z
Learnt from: jgarciao
Repo: opendatahub-io/opendatahub-tests PR: 1024
File: tests/llama_stack/models/test_models.py:95-105
Timestamp: 2026-01-16T10:17:22.830Z
Learning: In llama_stack_client 0.4.x, tests that validate retrieval results should expect a ModelRetrieveResponse, not a Model. Update type expectations and assertions accordingly (e.g., check attributes like identifier and api_model_type on ModelRetrieveResponse). Ensure tests reference the correct type and fields to avoid type errors and misinterpretation of the API response.
Applied to files:
tests/llama_stack/vector_io/test_vector_stores.pytests/fixtures/vector_io.py
🪛 Ruff (0.14.11)
tests/fixtures/vector_io.py
374-374: Unused function argument: qdrant_secret
(ARG001)
396-396: Unused function argument: qdrant_deployment
(ARG001)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jgarciao
left a comment
jgarciao
left a comment
There was a problem hiding this comment.
I've added just a little nit. Thanks for this PR, @Bobbins228 !
Bobbins228
force-pushed
the
qdrant-tests
branch
from
2b11611 to
4e8d3b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/fixtures/vector_io.py`:
- Line 38: The comment on the string literal
"0425e3e03e7fd9b3dc95c4214546afe19de2eb2e28ca621441a56663ac6e1f46" contains an
extra trailing double-quote in the pragma (pragma: allowlist secret"), remove
the stray quote so the comment reads pragma: allowlist secret; update the line
in tests/fixtures/vector_io.py where that hex string appears to fix the typo.
♻️ Duplicate comments (4)
tests/fixtures/vector_io.py (4)
379-398: Silence Ruff ARG001 by marking the fixture dependency as intentionally used.The
qdrant_secretparameter is a fixture dependency to ensure the secret is created before the deployment, but Ruff flags it as unused. Mark it as intentionally used.Also, per coding guidelines, update the docstring to Google format with
Args:andYields:sections.🔧 Suggested fix
`@pytest.fixture`(scope="class") def qdrant_deployment( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, qdrant_secret: Secret, ) -> Generator[Deployment, Any, Any]: - """Deploy a Qdrant instance for vector I/O provider testing.""" + """Deploy a Qdrant instance for vector I/O provider testing. + + Args: + unprivileged_client: Kubernetes API client with unprivileged access. + unprivileged_model_namespace: Namespace for deploying resources. + qdrant_secret: Secret containing the Qdrant API key (dependency). + + Yields: + Deployment: The Qdrant deployment resource. + """ + _ = qdrant_secret # Fixture dependency for ordering with Deployment(
401-427: Silence Ruff ARG001 and update docstring to Google format.The
qdrant_deploymentparameter ensures deployment readiness before creating the service. Mark it as used and add proper docstring sections.🔧 Suggested fix
`@pytest.fixture`(scope="class") def qdrant_service( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, qdrant_deployment: Deployment, ) -> Generator[Service, Any, Any]: - """Create a service for the Qdrant deployment.""" + """Create a service for the Qdrant deployment. + + Args: + unprivileged_client: Kubernetes API client with unprivileged access. + unprivileged_model_namespace: Namespace for deploying resources. + qdrant_deployment: Qdrant deployment (dependency to ensure readiness). + + Yields: + Service: The Qdrant service resource. + """ + _ = qdrant_deployment # Fixture dependency for ordering with Service(
430-443: Update docstring to Google format.Per coding guidelines, fixtures require Google-format docstrings with
Args:andYields:sections.🔧 Suggested fix
`@pytest.fixture`(scope="class") def qdrant_secret( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, ) -> Generator[Secret, Any, Any]: - """Return a Kubernetes Secret for Qdrant""" + """Create a Kubernetes Secret for Qdrant API key. + + Args: + unprivileged_client: Kubernetes API client with unprivileged access. + unprivileged_model_namespace: Namespace for deploying resources. + + Yields: + Secret: The Qdrant secret containing the API key. + """ with Secret(
446-489: FixrunAsNonRootincompatibility with the standard Qdrant image.The standard
qdrant/qdrantDocker image runs as root (UID 0) by default. SettingrunAsNonRoot: Trueat the pod level without specifyingrunAsUserwill cause the pod to fail at startup with a "container has runAsNonRoot and image will run as root" error.Options to fix:
- Recommended: Add
runAsUser: 1000to the container'ssecurityContext(Qdrant supports running as non-root with this UID)- Use the unprivileged image variant (
qdrant:*-unprivileged) which defaults to UID 1000- Remove
runAsNonRoot: True(less secure)🔧 Suggested fix (Option 1)
"containers": [ { "name": "qdrant", "image": QDRANT_IMAGE, + "securityContext": { + "runAsUser": 1000, + "allowPrivilegeEscalation": False, + "capabilities": {"drop": ["ALL"]}, + }, "ports": [
Bobbins228
force-pushed
the
qdrant-tests
branch
from
4e8d3b1 to
c8ab22a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/fixtures/vector_io.py`:
- Around line 37-39: The pragma comment on the Qdrant image string literal is
using a different comment format and may not be detected; locate the string
starting with "docker.io/qdrant/qdrant@sha256:" and update its trailing comment
so the pragma follows the same pattern used earlier (i.e., separate the
human-readable comment and the pragma with the same comment marker sequence used
on line 27), e.g., adjust the trailing comment so it matches the file's
established "comment then pragma" format (two '#' markers) to ensure secret
detection recognizes the pragma.
♻️ Duplicate comments (3)
tests/fixtures/vector_io.py (3)
379-398: Mark fixture dependency as intentionally used to silence Ruff ARG001.The
qdrant_secretparameter is required for dependency ordering but flagged as unused. Add an explicit reference to mark it as intentionally used.🔧 Suggested fix
def qdrant_deployment( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, qdrant_secret: Secret, ) -> Generator[Deployment, Any, Any]: """Deploy a Qdrant instance for vector I/O provider testing.""" + _ = qdrant_secret # Ensure secret exists before deployment with Deployment(
401-443: Mark fixture dependency as intentionally used and update docstrings.Similar to
qdrant_deployment, theqdrant_deploymentparameter inqdrant_serviceis required for ordering but flagged as unused. Additionally, per coding guidelines, fixtures should use Google-format docstrings withArgs:andYields:sections (note: "Yields" not "Returns" for generator fixtures).🔧 Suggested fix for qdrant_service
def qdrant_service( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, qdrant_deployment: Deployment, ) -> Generator[Service, Any, Any]: - """Create a service for the Qdrant deployment.""" + """Create a service for the Qdrant deployment. + + Args: + unprivileged_client: Kubernetes API client. + unprivileged_model_namespace: Target namespace. + qdrant_deployment: Deployment dependency for ordering. + + Yields: + Service: The Qdrant Kubernetes Service. + """ + _ = qdrant_deployment # Ensure deployment exists before service with Service(
446-489: FixrunAsNonRootincompatibility with standard Qdrant image.The standard Qdrant Docker image runs as root (UID 0) by default. Setting
runAsNonRoot: Truewithout specifyingrunAsUserwill cause the pod to fail at startup with a security context violation.Options to resolve:
- Use the unprivileged image variant (
qdrant:*-unprivileged) and setrunAsUser: 1000- Add
runAsUser: 1000to the container's securityContext- Remove
runAsNonRoot: True(less secure)🔧 Suggested fix (option 2 - add runAsUser)
return { "metadata": {"labels": {"app": "qdrant"}}, "spec": { "securityContext": {"runAsNonRoot": True, "seccompProfile": {"type": "RuntimeDefault"}}, "containers": [ { "name": "qdrant", "image": QDRANT_IMAGE, + "securityContext": { + "runAsUser": 1000, + "allowPrivilegeEscalation": False, + "capabilities": {"drop": ["ALL"]}, + }, "ports": [
Bobbins228
force-pushed
the
qdrant-tests
branch
from
c8ab22a to
f762bf8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/vector_io.py (1)
47-124: Fix env-var type hints forvalueFromusage.
QDRANT_API_KEYnow usesvalueFrom, solist[Dict[str, str]]is no longer accurate and will fail mypy strict. Widen the typing (or introduce a TypedDict) to reflect bothvalueandvalueFrom.🔧 Proposed fix
-def vector_io_provider_deployment_config_factory( - request: FixtureRequest, -) -> Callable[[str], list[Dict[str, str]]]: +def vector_io_provider_deployment_config_factory( + request: FixtureRequest, +) -> Callable[[str], list[Dict[str, Any]]]: @@ - def _factory(provider_name: str) -> list[Dict[str, str]]: - env_vars: list[dict[str, str]] = [] + def _factory(provider_name: str) -> list[Dict[str, Any]]: + env_vars: list[dict[str, Any]] = []
♻️ Duplicate comments (3)
tests/fixtures/vector_io.py (3)
379-443: Use Google-style docstrings for new fixtures.
qdrant_deployment,qdrant_service, andqdrant_secretneedArgs:/Returns:sections (Google format) per the test guidelines. As per coding guidelines, please update these docstrings accordingly.
379-426: Silence Ruff ARG001 for fixture dependencies.
qdrant_secretandqdrant_deploymentare intentionally unused, but Ruff still flags them. Add a no-op reference to keep the dependency while satisfying lint.🔧 Proposed fix
def qdrant_deployment( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, qdrant_secret: Secret, ) -> Generator[Deployment, Any, Any]: """Deploy a Qdrant instance for vector I/O provider testing.""" + _ = qdrant_secret @@ def qdrant_service( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, qdrant_deployment: Deployment, ) -> Generator[Service, Any, Any]: """Create a service for the Qdrant deployment.""" + _ = qdrant_deployment
446-455:runAsNonRootlikely breaks the default Qdrant image.The default
qdrant/qdrantimage runs as root; settingrunAsNonRoot: Truewithout an explicit non-root user will prevent the pod from starting. Either switch to an unprivileged image variant or setrunAsUserexplicitly. Please verify against the Qdrant image docs.🔧 One possible fix (set non-root UID)
def get_qdrant_deployment_template() -> Dict[str, Any]: """Return a Kubernetes deployment for Qdrant""" return { "metadata": {"labels": {"app": "qdrant"}}, "spec": { - "securityContext": {"runAsNonRoot": True, "seccompProfile": {"type": "RuntimeDefault"}}, + "securityContext": { + "runAsNonRoot": True, + "runAsUser": 1000, + "seccompProfile": {"type": "RuntimeDefault"}, + },
|
/hold I am going to make some minor upgrades to this PR |
Bobbins228
force-pushed
the
qdrant-tests
branch
from
f762bf8 to
d5f1039
Compare
|
/unhold Added generic secrets for llama stack distribution and vector stores |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/fixtures/vector_io.py (2)
47-88: Update fixture docstring for new args and Dict[str, Any] return.The Args/Returns sections still describe the old signature and return typing.
✍️ Suggested docstring update
@@ - Args: - request: Pytest fixture request object for accessing other fixtures + Args: + unprivileged_client: Unprivileged Kubernetes client + unprivileged_model_namespace: Namespace for deploying provider resources + vector_io_secret: Secret containing provider credentials + request: Pytest fixture request object for accessing other fixtures @@ - Returns: - Callable[[str], list[Dict[str, str]]]: Factory function that takes a provider name - and returns a list of environment variable dictionaries + Returns: + Callable[[str], list[Dict[str, Any]]]: Factory function that takes a provider name + and returns a list of environment variable dictionariesAs per coding guidelines, ...
99-146: Fix env_vars typing to match valueFrom entries (mypy strict).
env_varsis annotated aslist[dict[str, str]]but now includes nested dicts viavalueFrom, which will trip strict typing.🧩 Proposed fix
def _factory(provider_name: str) -> list[Dict[str, Any]]: - env_vars: list[dict[str, str]] = [] + env_vars: list[dict[str, Any]] = []As per coding guidelines, ...
🤖 Fix all issues with AI agents
In `@tests/fixtures/vector_io.py`:
- Around line 153-170: Add a Google-style docstring to the vector_io_secret
pytest fixture: update the docstring for the function named vector_io_secret
(the fixture that creates the Secret) to include an Args section documenting
unprivileged_client (DynamicClient) and unprivileged_model_namespace (Namespace)
and a Returns section describing the yielded Secret generator; keep it concise,
include purpose ("Create a secret for the vector I/O providers"), parameter
types, and that it yields a Secret for use by tests.
In `@tests/llama_stack/conftest.py`:
- Around line 294-305: The admin namespace is missing the
llamastack-distribution-secret used by POSTGRES_USER/POSTGRES_PASSWORD; add a
fixture that creates the same secret scoped to the admin/model_namespace and
ensure the admin deployment fixtures reference it (make the admin secret fixture
used by postgres_deployment and/or llama_stack_distribution); update the test
fixtures so env_vars for POSTGRES_USER and POSTGRES_PASSWORD can read from the
admin-scoped secret (same name/key as in the diff) and wire the new fixture into
the postgres_deployment/llama_stack_distribution fixture dependencies.
- Around line 339-354: Add a Google-style docstring to the fixture function
llama_stack_distribution_secret describing what the fixture provides,
documenting the parameters unprivileged_client (DynamicClient) and
unprivileged_model_namespace (Namespace) under an Args section, and documenting
the return value under a Returns section as a Generator that yields a Secret
used for Postgres credentials; place the docstring immediately below the def
line and ensure it follows Google-format with brief descriptions for each arg
and the returned Secret.
🧹 Nitpick comments (1)
tests/fixtures/vector_io.py (1)
428-446: Make qdrant_deployment explicitly depend on vector_io_secret.The deployment template uses a secretRef; adding the dependency keeps the fixture self-contained and avoids ordering/race issues if used directly.
♻️ Suggested tweak
def qdrant_deployment( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, + vector_io_secret: Secret, ) -> Generator[Deployment, Any, Any]: """Deploy a Qdrant instance for vector I/O provider testing.""" + _ = vector_io_secret with Deployment(As per coding guidelines, ...
Bobbins228
force-pushed
the
qdrant-tests
branch
from
d5f1039 to
8cd8dd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/fixtures/vector_io.py (1)
54-101: Fix env_vars typing and docstring to match secretRef entries.
env_varsnow includesvalueFromdicts, solist[dict[str, str]]is invalid and will likely break mypy strict. Also, the fixture docstring should include the newly added fixture args and the updated return type.As per coding guidelines, ...🛠️ Proposed fix
- Args: - request: Pytest fixture request object for accessing other fixtures + Args: + unprivileged_client: Unprivileged Kubernetes client + unprivileged_model_namespace: Namespace for vector I/O deployments + vector_io_secret: Secret containing provider credentials + request: Pytest fixture request object for accessing other fixtures @@ - Returns: - Callable[[str], list[Dict[str, str]]]: Factory function that takes a provider name + Returns: + Callable[[str], list[Dict[str, Any]]]: Factory function that takes a provider name and returns a list of environment variable dictionaries @@ - env_vars: list[dict[str, str]] = [] + env_vars: list[Dict[str, Any]] = []tests/llama_stack/conftest.py (2)
357-383: Silence Ruff ARG001 for unusedllama_stack_distribution_secret.The fixture parameter is unused in both distributions; add a no-op reference to keep the dependency without lint noise.
🧹 Suggested fix
def unprivileged_llama_stack_distribution( @@ llama_stack_distribution_secret: Secret, @@ ) -> Generator[LlamaStackDistribution, None, None]: + _ = llama_stack_distribution_secret @@ def llama_stack_distribution( @@ llama_stack_distribution_secret: Secret, @@ ) -> Generator[LlamaStackDistribution, None, None]: + _ = llama_stack_distribution_secretAlso applies to: 405-430
815-835: Fix NameError: missingllama_stack_distribution_secretin fixture signatures.Both postgres deployment fixtures reference
llama_stack_distribution_secretbut don’t accept it, causing a runtime failure. Add the parameter to the function signatures.🐛 Proposed fix
def unprivileged_postgres_deployment( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, + llama_stack_distribution_secret: Secret, ) -> Generator[Deployment, Any, Any]: """Deploy a Postgres instance for vector I/O provider testing with unprivileged client.""" _ = llama_stack_distribution_secret @@ def postgres_deployment( admin_client: DynamicClient, model_namespace: Namespace, + llama_stack_distribution_secret: Secret, ) -> Generator[Deployment, Any, Any]: """Deploy a Postgres instance for vector I/O provider testing.""" _ = llama_stack_distribution_secretAlso applies to: 862-882
♻️ Duplicate comments (3)
tests/fixtures/vector_io.py (2)
153-171: Google-style docstring still missing forvector_io_secret.This was previously flagged; the fixture docstring still lacks Google-format Args/Returns.
As per coding guidelines, ...
428-477: Qdrant fixture docstrings still not Google-format.This was already raised earlier for the new fixtures; still outstanding.
As per coding guidelines, ...
tests/llama_stack/conftest.py (1)
339-354: Admin namespace still missingllamastack-distribution-secret.The secret is created only in
unprivileged_model_namespace, but admin flows use the same secret name inmodel_namespace. This will break admin deployments unless a secret is also provisioned there (or the fixture is adjusted to create it in both namespaces). Also, the fixture docstring still lacks Google-format Args/Returns.As per coding guidelines, ...
Bobbins228
force-pushed
the
qdrant-tests
branch
from
8cd8dd3 to
8da9dfb
Compare
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/fixtures/vector_io.py (1)
405-419: Credentials exposed in lifecycle command despite secret-based env vars.The
POSTGRES_USERandPOSTGRES_PASSWORDenv vars now correctly usesecretKeyRef, but thepostStartlifecycle command still embeds credentials via Python f-strings (lines 413-414). This means credentials are visible in the deployment spec.Consider using the container's environment variables within the lifecycle command instead:
🔧 Suggested fix
"lifecycle": { "postStart": { "exec": { "command": [ "/bin/sh", "-c", - ( - "sleep 5\n" - f"PGPASSWORD={PGVECTOR_PASSWORD} psql -h localhost -U {PGVECTOR_USER} " - '-d pgvector -c "CREATE EXTENSION IF NOT EXISTS vector;" || true' - ), + "sleep 5\n" + 'PGPASSWORD=$POSTGRES_PASSWORD psql -h localhost -U $POSTGRES_USER ' + '-d pgvector -c "CREATE EXTENSION IF NOT EXISTS vector;" || true', ] } } },
🤖 Fix all issues with AI agents
In `@tests/fixtures/vector_io.py`:
- Around line 428-446: The qdrant_deployment fixture fails when created without
its required secret; update the qdrant_deployment fixture signature to accept
the vector_io_secret fixture (add vector_io_secret: Secret or the correct
fixture name/type as a parameter) so the secret is created before Deployment is
applied, and ensure get_qdrant_deployment_template() continues to reference the
"vector-io-secret" via secretKeyRef; this guarantees the secret exists prior to
calling Deployment in qdrant_deployment.
♻️ Duplicate comments (3)
tests/fixtures/vector_io.py (2)
153-171: Add Google-format docstring forvector_io_secretfixture.As per coding guidelines, fixture docstrings must include
Args:andReturns:sections.✏️ Suggested docstring
def vector_io_secret( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, ) -> Generator[Secret, Any, Any]: - """Create a secret for the vector I/O providers""" + """Create a secret for the vector I/O providers. + + Args: + unprivileged_client: Unprivileged Kubernetes client. + unprivileged_model_namespace: Namespace for the secret. + + Yields: + Secret containing credentials for vector I/O providers. + """
449-477: Add Google-format docstrings for Qdrant fixtures.As per coding guidelines, fixture docstrings should include
Args:andYields:sections forqdrant_deployment(lines 428-446) andqdrant_service(lines 449-477).tests/llama_stack/conftest.py (1)
344-371: Add Google-format docstrings for secret fixtures.As per coding guidelines,
llama_stack_distribution_secretandunprivileged_llama_stack_distribution_secretfixtures should includeArgs:andYields:sections.
🧹 Nitpick comments (3)
tests/fixtures/vector_io.py (1)
64-66: Docstring return type is inconsistent with actual signature.The docstring at line 65 states
list[Dict[str, str]], but the function signature at line 53 correctly returnslist[Dict[str, Any]]to accommodatevalueFromsecretKeyRef structures.✏️ Suggested fix
Returns: - Callable[[str], list[Dict[str, str]]]: Factory function that takes a provider name + Callable[[str], list[Dict[str, Any]]]: Factory function that takes a provider name and returns a list of environment variable dictionariestests/llama_stack/conftest.py (2)
374-400: Mark secret fixture dependency as intentionally used to silence ARG001.The
unprivileged_llama_stack_distribution_secretparameter (line 386) is a dependency to ensure the secret exists, but it's flagged as unused by Ruff. Apply the same pattern used invector_io.py.🔧 Suggested fix
def unprivileged_llama_stack_distribution( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, enabled_llama_stack_operator: DataScienceCluster, request: FixtureRequest, llama_stack_server_config: Dict[str, Any], ci_s3_bucket_name: str, ci_s3_bucket_endpoint: str, ci_s3_bucket_region: str, aws_access_key_id: str, aws_secret_access_key: str, unprivileged_llama_stack_distribution_secret: Secret, unprivileged_postgres_deployment: Deployment, unprivileged_postgres_service: Service, ) -> Generator[LlamaStackDistribution, None, None]: + _ = unprivileged_llama_stack_distribution_secret # Distribution name needs a random substring due to bug RHAIENG-999 / RHAIENG-1139
422-447: Mark secret fixture dependency as intentionally used to silence ARG001.The
llama_stack_distribution_secretparameter (line 434) is flagged as unused. Apply the same_ =pattern.🔧 Suggested fix
def llama_stack_distribution( admin_client: DynamicClient, model_namespace: Namespace, enabled_llama_stack_operator: DataScienceCluster, request: FixtureRequest, llama_stack_server_config: Dict[str, Any], ci_s3_bucket_name: str, ci_s3_bucket_endpoint: str, ci_s3_bucket_region: str, aws_access_key_id: str, aws_secret_access_key: str, llama_stack_distribution_secret: Secret, postgres_deployment: Deployment, postgres_service: Service, ) -> Generator[LlamaStackDistribution, None, None]: + _ = llama_stack_distribution_secret # Distribution name needs a random substring due to bug RHAIENG-999 / RHAIENG-1139
Head branch was pushed to by a user without write access
Bobbins228
force-pushed
the
qdrant-tests
branch
from
bef5070 to
70296f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/vector_io.py (1)
417-431: Lifecycle command exposes credentials in plaintext.The
postStartlifecycle command embedsPGVECTOR_PASSWORDandPGVECTOR_USERdirectly in the command string, which will be visible in the pod spec. This is inconsistent with thesecretKeyRefpattern used for the container's environment variables (lines 407-414).Consider referencing the environment variables set from the secret instead:
🔧 Suggested fix
"lifecycle": { "postStart": { "exec": { "command": [ "/bin/sh", "-c", ( - "sleep 5\n" - f"PGPASSWORD={PGVECTOR_PASSWORD} psql -h localhost -U {PGVECTOR_USER} " - '-d pgvector -c "CREATE EXTENSION IF NOT EXISTS vector;" || true' + "sleep 5\n" + 'PGPASSWORD="$POSTGRES_PASSWORD" psql -h localhost -U "$POSTGRES_USER" ' + '-d pgvector -c "CREATE EXTENSION IF NOT EXISTS vector;" || true' ), ] } } },Note: The env var names in the container are
POSTGRES_USERandPOSTGRES_PASSWORD(from the secretKeyRef), so the lifecycle command should reference those.
♻️ Duplicate comments (4)
tests/fixtures/vector_io.py (3)
153-171: Add Google-format docstring with Args/Returns sections.The
vector_io_secretfixture docstring is missing the requiredArgs:andReturns:sections per coding guidelines.✍️ Suggested docstring
def vector_io_secret( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, ) -> Generator[Secret, Any, Any]: - """Create a secret for the vector I/O providers""" + """Create a secret for the vector I/O providers. + + Args: + unprivileged_client: Kubernetes client for unprivileged operations. + unprivileged_model_namespace: Namespace for the secret. + + Yields: + Secret containing credentials for vector I/O providers. + """ with Secret(As per coding guidelines, ...
440-461: Add Google-format docstring with Args/Yields sections.The
qdrant_deploymentfixture docstring is missing required sections per coding guidelines.✍️ Suggested docstring
def qdrant_deployment( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, vector_io_secret: Secret, ) -> Generator[Deployment, Any, Any]: - """Deploy a Qdrant instance for vector I/O provider testing.""" + """Deploy a Qdrant instance for vector I/O provider testing. + + Args: + unprivileged_client: Kubernetes client for unprivileged operations. + unprivileged_model_namespace: Namespace for the deployment. + vector_io_secret: Secret containing Qdrant API key. + + Yields: + Deployment resource for the Qdrant instance. + """ _ = vector_io_secretAs per coding guidelines, ...
464-492: Add Google-format docstring with Args/Yields sections.The
qdrant_servicefixture docstring is missing required sections per coding guidelines.✍️ Suggested docstring
def qdrant_service( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, qdrant_deployment: Deployment, ) -> Generator[Service, Any, Any]: - """Create a service for the Qdrant deployment.""" + """Create a service for the Qdrant deployment. + + Args: + unprivileged_client: Kubernetes client for unprivileged operations. + unprivileged_model_namespace: Namespace for the service. + qdrant_deployment: Deployment resource to expose. + + Yields: + Service exposing Qdrant HTTP and gRPC ports. + """ _ = qdrant_deploymentAs per coding guidelines, ...
tests/llama_stack/conftest.py (1)
344-356: Add Google-format docstring with Args/Yields sections.The
llama_stack_distribution_secretfixture is missing a docstring per coding guidelines.✍️ Suggested docstring
`@pytest.fixture`(scope="class") def llama_stack_distribution_secret( admin_client: DynamicClient, model_namespace: Namespace, ) -> Generator[Secret, Any, Any]: + """Create a secret for LlamaStack Postgres credentials. + + Args: + admin_client: Kubernetes client with admin privileges. + model_namespace: Namespace for the secret. + + Yields: + Secret containing Postgres user and password. + """ with Secret(As per coding guidelines, ...
🧹 Nitpick comments (3)
tests/llama_stack/conftest.py (3)
359-371: Add Google-format docstring with Args/Yields sections.The
unprivileged_llama_stack_distribution_secretfixture is missing a docstring per coding guidelines.✍️ Suggested docstring
`@pytest.fixture`(scope="class") def unprivileged_llama_stack_distribution_secret( unprivileged_client: DynamicClient, unprivileged_model_namespace: Namespace, ) -> Generator[Secret, Any, Any]: + """Create a secret for LlamaStack Postgres credentials in unprivileged namespace. + + Args: + unprivileged_client: Kubernetes client for unprivileged operations. + unprivileged_model_namespace: Namespace for the secret. + + Yields: + Secret containing Postgres user and password. + """ with Secret(As per coding guidelines, ...
374-401: Silence Ruff ARG001 by marking fixture dependencies as used.The
unprivileged_llama_stack_distribution_secret,unprivileged_postgres_deployment, andunprivileged_postgres_serviceparameters are intentional dependencies but Ruff flags them as unused.🔧 Suggested fix
unprivileged_llama_stack_distribution_secret: Secret, unprivileged_postgres_deployment: Deployment, unprivileged_postgres_service: Service, ) -> Generator[LlamaStackDistribution, None, None]: + _ = unprivileged_llama_stack_distribution_secret + _ = unprivileged_postgres_deployment + _ = unprivileged_postgres_service # Distribution name needs a random substring due to bug RHAIENG-999 / RHAIENG-1139
422-447: Silence Ruff ARG001 by marking fixture dependencies as used.The
llama_stack_distribution_secret,postgres_deployment, andpostgres_serviceparameters are intentional dependencies but Ruff flags them as unused.🔧 Suggested fix
llama_stack_distribution_secret: Secret, postgres_deployment: Deployment, postgres_service: Service, ) -> Generator[LlamaStackDistribution, None, None]: + _ = llama_stack_distribution_secret + _ = postgres_deployment + _ = postgres_service # Distribution name needs a random substring due to bug RHAIENG-999 / RHAIENG-1139
|
Status of building tag latest: success. |
* test: add qdrant vector_io tests * test: update env vars for qdrant and readme added generic secrets --------- Co-authored-by: Jorge <jgarciao@users.noreply.github.com>
5 participants
Description
Added Qdrant Vector Store tests
How Has This Been Tested?
Set up your testing environment .env file
Ran tests and passed successfully:
Merge criteria:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.