MGMT-21406 Switch from Gemini to Vertex AI #126
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughReplaces Gemini/Llama direct wiring with Vertex credential plumbing and a default ASSISTED_CHAT_DEFAULT_MODEL; parameterizes JWT claims and DB SSL modes; mounts Vertex service-account secret into the deployment; updates generator and deploy scripts to handle Vertex credentials and in-cluster secret creation; moves many test stores to SQLite. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Lightspeed as LightspeedStack
participant Llama as LlamaStackClient
participant Vertex as VertexAI
Note over Lightspeed,Llama: Config defaults to ASSISTED_CHAT_DEFAULT_MODEL\nand default_provider="gemini"
User->>Lightspeed: send chat request
Lightspeed->>Llama: forward inference request
rect rgb(232,245,233)
Note right of Llama: LlamaStack uses mounted\nGOOGLE_APPLICATION_CREDENTIALS (service-account JSON)
end
Llama->>Vertex: authenticated inference call (via service account)
Vertex-->>Llama: inference response
Llama-->>Lightspeed: result
Lightspeed-->>User: reply
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
template.yaml (1)
274-279: Broken template references: undefined provider_id/provider_type parametersThese lines still reference ${LLAMA_STACK_INFERENCE_PROVIDER} and ${LLAMA_STACK_INFERENCE_PROVIDER_TYPE}, but those parameters were removed. This will leave unresolved placeholders in the rendered ConfigMap and likely break startup.
Replace the inference providers block with an empty list to match the “auto-discovery” intent:
providers: - inference: - - provider_id: ${LLAMA_STACK_INFERENCE_PROVIDER} - provider_type: ${LLAMA_STACK_INFERENCE_PROVIDER_TYPE} - config: - api_key: dummy-to-stop-llama-stack-from-complaining-even-though-we-use-vertex-and-not-gemini-directly + inference: []If you still need an explicit provider, hard-code the correct provider_id/provider_type instead of param references, or reintroduce those parameters.
🧹 Nitpick comments (1)
template.yaml (1)
110-112: Quote the default model value for consistency and YAML safetyMinor consistency nit. Most string parameter values are quoted; quoting here avoids accidental type coercion by YAML parsers.
Apply this diff:
- value: gemini-2.0-flash + value: "gemini-2.0-flash"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
template-params.dev.env(1 hunks)template.yaml(8 hunks)
🔇 Additional comments (4)
template-params.dev.env (1)
7-7: Verify model ID format consistency between dev and cluster configsDev uses "gemini/gemini-2.0-flash" while template default uses "gemini-2.0-flash". Confirm that Lightspeed accepts both with default_provider=gemini (no duplication or mismatch), or standardize to one format across environments.
template.yaml (3)
47-49: Confirm Vertex service-account secret content and key namingGood addition. Ensure the secret contains a single JSON key for the service account and that its filename matches the mount/subPath you intend to use below (see comments on Lines 440-442 and 499-501). Otherwise, the SDK won’t find creds via GOOGLE_APPLICATION_CREDENTIALS.
158-160: Validate provider/model alignment with Vertex-backed GeminiYou’re hard-coding default_provider: gemini and letting models be auto-discovered. That’s fine if Lightspeed treats Vertex-backed Gemini as provider “gemini.” Please confirm that:
- “gemini” is the correct provider id for Vertex, and
- default_model accepts both “gemini-2.0-flash” and “gemini/gemini-2.0-flash” when default_provider is set.
333-333: Models list emptied as intendedSetting models: [] aligns with the goal to let Lightspeed/Llama auto-discover Vertex AI models.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
template.yaml (1)
391-397: Remove unused GEMINI_API_KEY env and rely solely on Vertex credentials
The env var still references a secret that may not exist anymore, contradicting the PR goal and potentially breaking deployment. The dummy key in the client config suffices.Apply this diff:
- - name: GEMINI_API_KEY - valueFrom: - secretKeyRef: - name: ${GEMINI_API_SECRET_NAME} - key: api_key - name: GOOGLE_APPLICATION_CREDENTIALS value: /app-root/google-vertex-service-account.json
♻️ Duplicate comments (2)
template.yaml (2)
452-455: Mount the secret as a single file with predictable name and read-only
Current subPath is “service_account”, which assumes the secret key is named exactly that. Pin the filename and mark the mount read-only.Apply this diff (adjust key name if your secret uses a different one):
- name: google-vertex-service-account - mountPath: /app-root/google-vertex-service-account.json - subPath: service_account + mountPath: /app-root/google-vertex-service-account.json + subPath: google-vertex-service-account.json + readOnly: true
512-515: Define secret items to ensure the expected filename exists inside the volume
Without items, subPath must match an existing secret key name. Map the secret key to the desired filename to make this robust.Apply this diff (replace the key if your actual secret uses a different one):
- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: google-vertex-service-account.json + path: google-vertex-service-account.json
🧹 Nitpick comments (1)
template.yaml (1)
50-52: Document the expected secret key name for the Vertex SA JSONPlease clarify in the parameter description which key name the secret must contain (so volumes/subPath work predictably). Suggest mentioning the exact filename you intend to mount (see related mount comments below).
Proposed description tweak:
- description: "Name of the Kubernetes secret containing the Vertex service account credentials" + description: "Name of the Kubernetes secret containing the Vertex service account credentials (must include key: google-vertex-service-account.json)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
template-params.dev.env(1 hunks)template.yaml(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template-params.dev.env
🔇 Additional comments (3)
template.yaml (3)
161-163: Confirm default_model matches a declared model_id
After updating the parameter above, this will correctly resolve to a declared model_id. If you intentionally want to allow unqualified names, confirm lightspeed-stack resolves them at runtime.
278-282: Using a dummy Gemini API key in client config is acceptable here
This aligns with the plan to rely on Vertex while satisfying llama-stack’s provider validation.
337-340: Models list correctly uses provider-qualified ID
The model entry is consistent with the chosen provider and expected format.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
template.yaml (1)
391-395: Remove legacy GEMINI_API_KEY env and secret param from all files (multiple references remain)rg found live references — removing only template.yaml is not enough. Clean or replace these occurrences to avoid stale secret dependencies.
- template.yaml (lines shown by rg: 47, 391–395): remove the GEMINI_API_KEY env block and the GEMINI_API_SECRET_NAME parameter.
- test/prow/template.yaml (lines ~36, 40, 76): remove the GEMINI_API_KEY env block and the GEMINI_API_SECRET_NAME parameter.
- assisted-chat-pod.yaml (lines 10–11 and 80–81): remove GEMINI_API_KEY env entries (value: ${GEMINI_API_KEY}) or switch to Vertex creds.
- scripts/generate.sh (lines ~19–20, ~39): stop prompting for/writing GEMINI_API_KEY to .env and remove the dummy fallback.
- config/llama_stack_client_config.yaml (line ~19): remove or replace api_key: ${env.GEMINI_API_KEY:=} to use Vertex configuration or make optional.
- test/evals/README.md (line ~14): update docs that mention GEMINI_API_KEY.
Apply this diff to template.yaml (at the snippet in the original comment):
- - name: GEMINI_API_KEY - valueFrom: - secretKeyRef: - name: ${GEMINI_API_SECRET_NAME} - key: api_keyAfter making changes, re-run:
rg -n --no-heading $'GEMINI_API_KEY|GEMINI_API_SECRET_NAME'
♻️ Duplicate comments (3)
template.yaml (3)
47-49: Remove obsolete GEMINI_API_SECRET_NAME parameterThis parameter is no longer used per the PR objectives and leaves dead/confusing surface in the template.
Apply this diff to drop it:
- - name: GEMINI_API_SECRET_NAME - value: "assisted-chat-gemini-secret" - description: "Name of the Kubernetes secret containing the Gemini API key"
113-115: Fully-qualify ASSISTED_CHAT_DEFAULT_MODEL to prevent lookup mismatchConfigured models and provider selection expect a provider-qualified model id. Use gemini/gemini-2.0-flash.
Apply this diff:
- - name: ASSISTED_CHAT_DEFAULT_MODEL - value: gemini-2.0-flash + - name: ASSISTED_CHAT_DEFAULT_MODEL + value: "gemini/gemini-2.0-flash"
396-397: Align secret mount with GOOGLE_APPLICATION_CREDENTIALS; make mount read-only and pin filenameGOOGLE_APPLICATION_CREDENTIALS points to /app-root/google-vertex-service-account.json (template.yaml:397), but the volumeMount uses subPath: service_account and the secret has no pinned items. Update the mount and secret so the file exists at the exact path and is read-only.
Files to update:
- template.yaml:397 (env var)
- template.yaml:452-454 (volumeMount)
- template.yaml:512-514 (secret)
Apply these diffs:
- Make the single-file mount read-only and use a stable subPath that matches the mounted filename:
- name: google-vertex-service-account mountPath: /app-root/google-vertex-service-account.json - subPath: service_account + subPath: google-vertex-service-account.json + readOnly: true
- Pin the secret key to the expected filename via items (replace the key if your secret uses a different key name):
- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: google-vertex-service-account.jsonQuick verification: confirm the actual key name inside the secret before applying (e.g. kubectl -n get secret ${VERTEX_API_SECRET_NAME} -o yaml) and replace "service_account" if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
template-params.dev.env(1 hunks)template.yaml(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template-params.dev.env
🔇 Additional comments (3)
template.yaml (3)
50-52: LGTM: Vertex credentials parameter addedVERTEX_API_SECRET_NAME addition and description look correct and aligned with the PR goal.
161-163: LGTM: default provider/model wiring into lightspeed configdefault_model via parameter and default_provider: gemini are consistent with the Vertex switch. Will be correct after the parameter is fully qualified.
278-282: LGTM: Provider set to gemini with dummy api_keyMatches the intent to route via Vertex while satisfying llama-stack validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
template.yaml (1)
399-403: Remove obsolete GEMINI_API_KEY env (prevents deployment failures if secret is absent)This deployment still requires the Gemini secret even though the PR switches to Vertex and the client config uses a dummy key. If the secret isn’t present in the cluster, the pod will fail to schedule. Remove this env block.
- - name: GEMINI_API_KEY - valueFrom: - secretKeyRef: - name: ${GEMINI_API_SECRET_NAME} - key: api_key
♻️ Duplicate comments (3)
template.yaml (3)
113-115: Fully-qualify the default model to match model IDs used elsewhereTo avoid lookup mismatches with the models list and provider, set the default to a fully-qualified model ID and quote it.
- name: ASSISTED_CHAT_DEFAULT_MODEL - value: gemini-2.0-flash + name: ASSISTED_CHAT_DEFAULT_MODEL + value: "gemini/gemini-2.0-flash"
345-348: Remove hard-coded model; rely on auto-discoveryPer PR objectives, models should be discovered automatically by lightspeed-stack. Keeping a hard-coded model risks drift and conflicts.
- models: - - model_id: gemini/gemini-2.0-flash - provider_id: gemini - provider_model_id: gemini/gemini-2.0-flash - model_type: llm + models: []
47-49: Remove obsolete GEMINI_API_SECRET_NAME parameterThis parameter is no longer used with the Vertex switch and retaining it is misleading.
- - name: GEMINI_API_SECRET_NAME - value: "assisted-chat-gemini-secret" - description: "Name of the Kubernetes secret containing the Gemini API key"
🧹 Nitpick comments (1)
template.yaml (1)
404-406: Ensure mounted secret key name matches subPath and mark mount read-onlyThe env var points to a file path which is fine with subPath. However, the subPath refers to "service_account" and the volume lacks items. Unless the secret actually has a key named "service_account", the mount will fail. Also, secret mounts should be read-only.
- Verify the secret’s data key is "service_account" (or update subPath/items accordingly).
- Mark the mount read-only.
- Pin the secret key-to-filename mapping via items for predictability.
Apply:
- name: google-vertex-service-account mountPath: /app-root/google-vertex-service-account.json - subPath: service_account + subPath: service_account + readOnly: trueAnd:
- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: service_accountIf your actual secret uses a different key (e.g., "google-vertex-service-account.json"), change both items.key and the volumeMount subPath to that exact key.
Also applies to: 460-463, 520-522
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
template-params.dev.env(1 hunks)template.yaml(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template-params.dev.env
🔇 Additional comments (3)
template.yaml (3)
50-52: Vertex secret parameter addition looks goodParameter aligns with PR goals and description is clear.
161-163: Inference defaults align with the Vertex/Gemini switchReferencing ${ASSISTED_CHAT_DEFAULT_MODEL} and setting default_provider to gemini is consistent with the new setup.
286-290: Intentional dummy API key is acceptable given Vertex usageUsing a dummy api_key to satisfy llama-stack while actually using Vertex credentials matches the PR rationale.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
scripts/generate.sh (1)
56-66: Harden secret handling: set strict file permissions after copyAfter copying the service account JSON, explicitly set its permissions to 600 to avoid accidental exposure if the source file had permissive bits.
cp "$VERTEX_AI_SERVICE_ACCOUNT_CREDENTIALS_PATH" "$PROJECT_ROOT/config/vertex-credentials.json" + chmod 600 "$PROJECT_ROOT/config/vertex-credentials.json" # We must set the GEMINI_API_KEY to a dummy value, as # lightspeed-stack wrongly expects it to be set for all Gemini # providers, even if we are using Vertex AI service account # authentication. echo GEMINI_API_KEY="dummy" >"$PROJECT_ROOT/.env" chmod 600 "$PROJECT_ROOT/.env" echo "Vertex credentials successfully configured."Optional alternative (copies with desired mode in a single step):
- cp "$VERTEX_AI_SERVICE_ACCOUNT_CREDENTIALS_PATH" "$PROJECT_ROOT/config/vertex-credentials.json" - chmod 600 "$PROJECT_ROOT/config/vertex-credentials.json" + install -m 600 "$VERTEX_AI_SERVICE_ACCOUNT_CREDENTIALS_PATH" "$PROJECT_ROOT/config/vertex-credentials.json"
🧹 Nitpick comments (1)
scripts/generate.sh (1)
27-27: Nit: fix wording in the user-facing messageMinor grammar polish.
- echo 'Also creating a dummy Vertex AI service account credentials file at config/vertex-credentials.json. If you want to use to be able to use both, modify config/vertex-credentials.json manually.' + echo 'Also creating a dummy Vertex AI service account credentials file at config/vertex-credentials.json. If you want to be able to use both, modify config/vertex-credentials.json manually.'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
scripts/generate.sh(3 hunks)template-params.dev.env(1 hunks)template.yaml(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- template-params.dev.env
- template.yaml
|
/hold wait for eval-test results |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
template.yaml (2)
453-456: Mount secret as a single file and mark read-onlyAdd readOnly: true and align subPath to a deterministic filename that exists in the secret. Right now subPath: service_account requires the secret key to be named “service_account”.
Apply this diff (matches the volumes items suggestion in the next comment):
- - name: google-vertex-service-account - mountPath: /app-root/google-vertex-service-account.json - subPath: service_account + - name: google-vertex-service-account + mountPath: /app-root/google-vertex-service-account.json + subPath: google-vertex-service-account.json + readOnly: true
486-489: Pin secret filename using items to match GOOGLE_APPLICATION_CREDENTIALSWithout items, the file name depends on the secret key name. Pin it to a stable filename and match the volumeMount subPath.
If your secret key is “service_account”, map it like this:
- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: google-vertex-service-account.jsonIf the actual key name differs, change key: accordingly, keeping path: google-vertex-service-account.json to match the subPath and env path.
🧹 Nitpick comments (4)
template.yaml (4)
110-112: Default model likely mismatched (unqualified ID)Your models list is empty and provider IDs elsewhere are “gemini/...”. An unqualified default like “gemini-2.0-flash” may not resolve.
Consider fully-qualifying to match provider naming or update to whatever the runtime expects.
Apply one of the following diffs (pick the one that matches your runtime):
- value: gemini-2.0-flash + value: "gemini/gemini-2.0-flash"or, if you intend to use Vertex-qualified IDs:
- value: gemini-2.0-flash + value: "vertex/gemini-2.0-flash"
118-120: Typo in parameter name: LIGHTSSPEED_...Parameter name has an extra “S” (LIGHTSSPEED). This propagates everywhere it’s referenced.
If you want this fixed for clarity (and to avoid future drift), rename consistently in the template and in any scripts that render it.
Proposed local changes (remember to update scripts/deploy_template.sh, template-params.dev.env, and test/prow/template.yaml accordingly):
- name: LIGHTSSPEED_STACK_POSTGRES_SSL_MODE + name: LIGHTSPEED_STACK_POSTGRES_SSL_MODE value: "verify-full" description: "SSL mode for the PostgreSQL database connection used by lightspeed-stack"and later (Line 176):
- ssl_mode: ${LIGHTSSPEED_STACK_POSTGRES_SSL_MODE} + ssl_mode: ${LIGHTSPEED_STACK_POSTGRES_SSL_MODE}
167-169: Confirm default_provider vs. “Switch to Vertex” intentiondefault_provider is set to “gemini” while the PR title states a switch to Vertex. If the plan is to route via Vertex using the Gemini provider wiring (and GOOGLE_APPLICATION_CREDENTIALS), that’s fine, but please confirm this is intentional to avoid confusion.
If you expect a Vertex-named provider, align provider IDs across lightspeed-stack and llama client config.
Would you like me to propose a follow-up patch that documents this in a comment next to default_provider to make intent explicit?
294-298: Remote Gemini provider with dummy API key — ensure it’s never used at runtimeIf llama-stack ever routes inference to this provider, the dummy key will cause runtime failures. If this is only to satisfy config validation, consider:
- gating usage via a non-default provider_id, or
- adding a clarifying comment,
- or using an “inline::noop”/disabled flag if supported.
Want me to check llama-stack docs and suggest a provider config that validates without needing a dummy API key?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Makefile(2 hunks)scripts/deploy_template.sh(1 hunks)scripts/generate.sh(3 hunks)template-params.dev.env(1 hunks)template.yaml(9 hunks)test/prow/template.yaml(0 hunks)test/prow/template_patch.diff(0 hunks)
💤 Files with no reviewable changes (2)
- test/prow/template.yaml
- test/prow/template_patch.diff
🚧 Files skipped from review as they are similar to previous changes (4)
- template-params.dev.env
- scripts/generate.sh
- Makefile
- scripts/deploy_template.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
🔇 Additional comments (5)
template.yaml (5)
47-49: Good addition: Vertex credentials parameter introducedParameterizing the Vertex service-account secret name is the right direction for different envs (dev/CI/prod). No functional concerns here.
150-151: JWT claim parameterization LGTMBinding user_id_claim and username_claim to parameters improves flexibility for different IdPs. Looks correct.
176-176: verify-full requires a CA (or system trust) — double-check deploymentWith ssl_mode=verify-full and db-ca-cert optional, TLS will fail unless the DB cert chains to a trusted CA available in /etc/pki/ca-trust (or you provide db.ca_cert). Either:
- make db.ca_cert non-optional, or
- document reliance on cluster trust bundle.
I can provide a small guard to switch ssl_mode to “require” when db.ca_cert is absent if desired.
352-352: Models discovery: empty list looks goodLeaving models empty to rely on discovery/remote provider config avoids drift. Good call.
403-405: GOOGLE_APPLICATION_CREDENTIALS path is fine — ensure the mount exposes a single file at that pathThis aligns with the mount path below. See next comments to make the mount read-only and to pin the filename via items/subPath to avoid accidental directory mounts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
template.yaml (2)
110-112: Default model should be fully qualified to match model IDsYour models are provider-qualified (e.g., gemini/...). Keep the default the same format to avoid mismatches at lookup time.
- value: gemini-2.0-flash + value: "gemini/gemini-2.0-flash"
495-497: Pin secret items to match subPath and fix potential key mismatchWithout items, the file name inside the mount is whatever the secret key name is. Pin it to service_account so it matches the subPath. Also align the deploy script to create the secret with key service_account.
- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: service_accountFollow-up required in scripts/deploy_template.sh:
- Create the secret with
--from-file=service_account="$SECRETS_BASE_PATH/vertex/service_account"(underscore) to match this key.- Ensure the file path exists (see my Makefile comment).
I can provide the exact script diff to standardize the key/path.
🧹 Nitpick comments (1)
template.yaml (1)
462-465: Mount Vertex secret as read-only and ensure key/subPath existMinor hardening: mark the secret mount read-only. Also, this subPath requires a secret key named service_account — add items on the secret volume (see below).
- name: google-vertex-service-account mountPath: /app-root/google-vertex-service-account.json subPath: service_account + readOnly: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Makefile(2 hunks)scripts/deploy_template.sh(1 hunks)scripts/generate.sh(3 hunks)template-params.dev.env(1 hunks)template.yaml(11 hunks)test/prow/template.yaml(0 hunks)test/prow/template_patch.diff(0 hunks)
💤 Files with no reviewable changes (2)
- test/prow/template.yaml
- test/prow/template_patch.diff
🚧 Files skipped from review as they are similar to previous changes (3)
- template-params.dev.env
- scripts/generate.sh
- scripts/deploy_template.sh
🔇 Additional comments (12)
Makefile (3)
8-8: PHONY list updated correctlyNew targets are properly added to .PHONY. No issues.
10-10: AI summary inconsistency:alltarget doesn’t include new targetsThe summary claims the new targets are prerequisites of
all, butallstill only shows help. If that was intended, ignore this. If not, wire them up.Would you like
allto run any of these targets (e.g., deploy-template)? If so, I can propose the exact Makefile wiring.
36-37: CI deploy target is fineTarget reads well and delegates to the script as expected.
template.yaml (9)
47-49: Vertex secret parameter looks goodClear parameterization of the secret name; aligns with the new volume in the Deployment.
153-155: JWT claim parameterization looks correctGood move making claims configurable; matches the auth module expectations.
170-172: Default provider set to gemini — confirm intended with Vertex-backed flowGiven the Vertex credentials, keeping default_provider as gemini is fine if llama-stack routes via a gemini remote provider. Just confirm this is the path you expect in production.
Would you like me to scan the config to ensure no direct Gemini API key usage paths are still live?
179-179: SSL mode wired to template param — OK, but ensure the same param naming is used everywhereThis references LIGHTSSPEED_STACK_POSTGRES_SSL_MODE; ensure deploy scripts and dev overrides pass the same spelling (see earlier comment).
I can generate a quick check against scripts/deploy_template.sh if helpful.
297-301: Gemini provider with dummy key — safe placeholderUsing a dummy key avoids startup errors without exposing secrets. Looks good.
315-316: Postgres SSL mode parameterization is consistentThese sections correctly reference LLAMA_STACK_POSTGRES_SSL_MODE; no issues.
Also applies to: 324-325, 359-360
361-361: Models list empty — aligns with discovery approachKeeping models: [] fits the intent to avoid hard-coded entries.
412-414: GOOGLE_APPLICATION_CREDENTIALS env set — ensure the mounted file path existsPath matches the volumeMount. Once the secret key/subPath alignment is fixed (see below), this will work.
118-123: LIGHTSSPEED_STACK_POSTGRES_SSL_MODE spelling is consistent across the repo
All occurrences of this parameter use the “LIGHTSSPEED” spelling, and the expected “LIGHTSPEED_STACK_POSTGRES_SSL_MODE” variant does not appear in any files. To avoid breaking existing deployment tooling, leave the name as-is in this PR. If you’d like to standardize the spelling, please open a follow-up to rename the parameter in template.yaml, scripts/deploy_template.sh, and any downstream consumers.
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)
template.yaml (1)
393-393: Fix OpenShift template parameter syntax (${{...}} → ${...})OpenShift templates require ${PARAM}. Using ${{...}} prevents substitution, likely producing invalid manifests (e.g., non-integer port/replica values).
Apply this diff:
- replicas: ${{REPLICAS_COUNT}} + replicas: ${REPLICAS_COUNT} @@ - containerPort: ${{SERVICE_PORT}} + containerPort: ${SERVICE_PORT} @@ - port: ${{SERVICE_PORT}} + port: ${SERVICE_PORT} @@ - port: ${{SERVICE_PORT}} + port: ${SERVICE_PORT} @@ - port: ${{SERVICE_PORT}} - targetPort: ${{SERVICE_PORT}} + port: ${SERVICE_PORT} + targetPort: ${SERVICE_PORT}Also applies to: 409-409, 473-473, 481-481, 518-519
♻️ Duplicate comments (1)
scripts/deploy_template.sh (1)
18-21: Secret key now matches template subPath (good)Key name service_account aligns with the template’s subPath: service_account. This unblocks GOOGLE_APPLICATION_CREDENTIALS resolution.
🧹 Nitpick comments (2)
template.yaml (2)
462-465: Mount credentials read-only and keep subPath stableRecommend marking the mounted credentials file read-only. The subPath already matches the secret key; keep that stable.
Apply this diff:
- name: google-vertex-service-account mountPath: /app-root/google-vertex-service-account.json subPath: service_account + readOnly: true
495-498: Pin secret item key to ensure predictable filename in the volumeWithout items, the mounted file depends on the secret’s keys. Pin the expected key (service_account) to avoid surprises.
Apply this diff:
- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: service_account
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Makefile(2 hunks)scripts/deploy_template.sh(1 hunks)scripts/generate.sh(3 hunks)template-params.dev.env(1 hunks)template.yaml(11 hunks)test/prow/template.yaml(0 hunks)test/prow/template_patch.diff(0 hunks)
💤 Files with no reviewable changes (2)
- test/prow/template_patch.diff
- test/prow/template.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- template-params.dev.env
- Makefile
- scripts/generate.sh
🔇 Additional comments (3)
scripts/deploy_template.sh (1)
73-75: Good: LLAMA Postgres SSL mode explicitly disabled for local devThis complements the LIGHTSSPEED override and avoids connection failures against the local plaintext Postgres.
template.yaml (2)
110-117: Confirm default model identifier formatDefault provider is set to gemini, but default model is unqualified (gemini-2.0-flash). If your code expects fully-qualified IDs, this may cause lookup mismatches.
If a qualified ID is required, update:
- value: gemini-2.0-flash + value: "gemini/gemini-2.0-flash"If unqualified names are supported alongside default_provider, keep as-is.
170-172: Verify provider choice vs. Vertex objectivedefault_provider: gemini is set while the PR objective is to switch to Vertex AI. If lightspeed handles Vertex directly and Llama’s gemini entry is a placeholder, this is fine; otherwise, you may be routing inference to gemini inadvertently.
Please confirm the runtime path for inference:
- Lightspeed uses Vertex via GOOGLE_APPLICATION_CREDENTIALS, ignoring Llama’s gemini provider, or
- Llama’s provider is actually used (and should be Vertex-aligned).
| IMAGE=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f1) | ||
| TAG=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f2) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make image parsing robust for registries with ports and digest references
Splitting on the first colon breaks when the registry includes a port (e.g., quay.io:443/…), and it’s unnecessary for digest references. Split on the last colon instead.
Apply this diff:
-IMAGE=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f1)
-TAG=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f2)
+IMAGE="${ASSISTED_CHAT_IMG%:*}"
+TAG="${ASSISTED_CHAT_IMG##*:}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IMAGE=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f1) | |
| TAG=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f2) | |
| IMAGE="${ASSISTED_CHAT_IMG%:*}" | |
| TAG="${ASSISTED_CHAT_IMG##*:}" |
🤖 Prompt for AI Agents
In scripts/deploy_template.sh around lines 15-16, the current split on the first
colon breaks for registries with ports and digest references; update the parsing
to split on the last colon and handle digest (@) cases: if the image string
contains an '@' treat it as a digest and set IMAGE to the substring before '@'
and TAG empty; otherwise obtain TAG as the substring after the last colon and
IMAGE as the substring before that last colon (use bash parameter expansion or
equivalent to extract the last-colon split rather than cut -d ":" -f1/f2).
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)
scripts/generate.sh (1)
56-66: Harden permissions of copied Vertex credentialsAfter copying the credentials, set restrictive permissions on the destination file to avoid accidental group/world read.
Apply this diff:
cp "$VERTEX_AI_SERVICE_ACCOUNT_CREDENTIALS_PATH" "$PROJECT_ROOT/config/vertex-credentials.json" + chmod 600 "$PROJECT_ROOT/config/vertex-credentials.json" @@ - echo "Vertex credentials successfully configured." + echo "Vertex credentials successfully configured."
♻️ Duplicate comments (5)
scripts/generate.sh (2)
25-31: Create config directory before writing dummy Vertex credentials (and fix grammar)On a clean checkout, config/ may not exist; writing the dummy credentials will fail. Also minor grammar nit in the message.
Apply this diff:
# Create a dummy Vertex AI service account credentials file if [[ ! -f "$PROJECT_ROOT/config/vertex-credentials.json" ]]; then - echo 'Also creating a dummy Vertex AI service account credentials file at config/vertex-credentials.json. If you want to use to be able to use both, modify config/vertex-credentials.json manually.' + # Ensure target directory exists + mkdir -p "$PROJECT_ROOT/config" + echo 'Also creating a dummy Vertex AI service account credentials file at config/vertex-credentials.json. If you want to be able to use both, modify config/vertex-credentials.json manually.' echo '{}' >"$PROJECT_ROOT/config/vertex-credentials.json" chmod 600 "$PROJECT_ROOT/config/vertex-credentials.json" fi
40-47: Ensure config directory exists before overwrite check/copy pathThe overwrite prompt/copy will fail if config/ doesn’t exist on a fresh workspace.
Apply this diff:
- if [[ -f "$PROJECT_ROOT/config/vertex-credentials.json" ]]; then + # Ensure target directory exists + mkdir -p "$PROJECT_ROOT/config" + if [[ -f "$PROJECT_ROOT/config/vertex-credentials.json" ]]; then echo "File $PROJECT_ROOT/config/vertex-credentials.json already exists. Do you want to overwrite it? (y/n)" read -r overwrite if [[ "$overwrite" != "y" && "$overwrite" != "Y" ]]; then echo "Exiting without copying." exit 1 fi fitemplate.yaml (3)
110-112: Default model should be fully-qualified to match models list formatValue is currently unqualified and may not resolve. Align with provider-qualified IDs (e.g., gemini/gemini-2.0-flash).
Apply this diff:
- name: ASSISTED_CHAT_DEFAULT_MODEL - value: gemini-2.0-flash + value: "gemini/gemini-2.0-flash"
462-465: Mount secret as a single file (keep read-only) to match GOOGLE_APPLICATION_CREDENTIALSAdd readOnly for the mount. SubPath is fine provided the secret key matches (see next comment).
Apply this diff:
- name: google-vertex-service-account mountPath: /app-root/google-vertex-service-account.json subPath: service_account + readOnly: true
495-498: Pin secret item key to ensure subPath works predictablyWithout items, filenames depend on secret keys. Pin the key so subPath: service_account resolves consistently.
Apply this diff:
- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: service_account
🧹 Nitpick comments (2)
scripts/generate.sh (1)
5-6: Improve portability: avoid readlink -f (not available on macOS)Use a POSIX-compatible approach to resolve SCRIPT_DIR/PROJECT_ROOT.
Apply this diff:
-SCRIPT_DIR=$(dirname "$(readlink -f "$0")") -PROJECT_ROOT=$(dirname "$SCRIPT_DIR") +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"template.yaml (1)
118-121: Rename parameter for consistency: use LIGHTSPEED_STACK_POSTGRES_SSL_MODEThe parameter name currently uses “LIGHTSSPEED” (double “S”), which doesn’t match the “lightspeed” spelling elsewhere and can confuse operators or automation.
Please update all occurrences:
In template.yaml
- Line 118:
- name: LIGHTSSPEED_STACK_POSTGRES_SSL_MODE→- name: LIGHTSPEED_STACK_POSTGRES_SSL_MODE- Line 179:
ssl_mode: ${LIGHTSSPEED_STACK_POSTGRES_SSL_MODE}→ssl_mode: ${LIGHTSPEED_STACK_POSTGRES_SSL_MODE}In scripts/deploy_template.sh
- Line 76:
-p LIGHTSSPEED_STACK_POSTGRES_SSL_MODE=disable→-p LIGHTSPEED_STACK_POSTGRES_SSL_MODE=disableAlso update the description (if it mentions the old spelling) and ensure any downstream consumers (CI/CD, env files, docs) reference the new key—or provide an alias for backward compatibility if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Makefile(2 hunks)scripts/deploy_template.sh(1 hunks)scripts/generate.sh(3 hunks)template-params.dev.env(1 hunks)template.yaml(11 hunks)test/prow/template.yaml(0 hunks)test/prow/template_patch.diff(0 hunks)
💤 Files with no reviewable changes (2)
- test/prow/template.yaml
- test/prow/template_patch.diff
🚧 Files skipped from review as they are similar to previous changes (3)
- template-params.dev.env
- scripts/deploy_template.sh
- Makefile
Having it in openshift/release is inconvenient, as it often needs to be modifies. It should just rely on the Makefile target like the install step. For now that target doesn't exist, but it will be added in. Doesn't matter because the CI doesn't work anyway. rh-ecosystem-edge/assisted-chat#126
|
/lgtm |
Having it in openshift/release is inconvenient, as it often needs to be modifies. It should just rely on the Makefile target like the install step. For now that target doesn't exist, but it will be added in. Doesn't matter because the CI doesn't work anyway. rh-ecosystem-edge/assisted-chat#126
We're moving all of our environments from Gemini to Gemini on Vertex AI. This commit updates the assisted-chat template to use Vertex AI instead of Gemini. - Changed template default default model to `gemini-2.0-flash`, which is the Vertex AI Gemini model - `ASSISTED_CHAT_DEFAULT_MODEL` controls the template's default model, which will default to `gemini/gemini-2.0-flash` for the local dev env, which is the normal Gemini model. - Removed `GEMINI_API_SECRET_NAME` and replaced it with `VERTEX_API_SECRET_NAME`, which is the name of the Kubernetes secret containing the Vertex service account credentials. - Removed all hard-coded models from the llama-stack config, along with their associated parameters. Vertex AI models are automatically discovered by lightspeed-stack - Removed GEMINI_API_KEY and hardcoded it to a dummy value in the llama-stack config, since without it llama-stack will complain about not having a Gemini API key, even though we are using Vertex AI and not Gemini - Modified scripts/deploy_template.sh to work well with minikube (for local experimentation), to use Vertex AI secret and use its own instance of postgres, to remove the need for a template patch to remove postgres. Also removed sed hacks and used USER_ID_CLAIM vars in the template instead. - Added `deploy-template-local` target to the Makefile, which deploys the template on whatever cluster `oc` is currently logged in to
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
template.yaml (3)
462-465: Mount secret file with readOnly and ensure subPath matches secret keyYou’re correctly using
subPath: service_account. Mark the mount read-only.- - name: google-vertex-service-account - mountPath: /app-root/google-vertex-service-account.json - subPath: service_account + - name: google-vertex-service-account + mountPath: /app-root/google-vertex-service-account.json + subPath: service_account + readOnly: true
495-497: Pin secret item key-to-filename mapping for predictable mountWithout items, key name must match
service_account. Pin it explicitly to avoid drift or misalignment with secret creation.- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: service_account
110-116: Default model should be fully-qualified to match provider IDsUsing an unqualified
gemini-2.0-flashcan cause mismatches when providers expect fully-qualified IDs.- - name: ASSISTED_CHAT_DEFAULT_MODEL - value: gemini-2.0-flash + - name: ASSISTED_CHAT_DEFAULT_MODEL + value: "gemini/gemini-2.0-flash"scripts/deploy_template.sh (1)
14-16: Make image parsing robust (ports, multiple colons, and digests)Splitting on the first colon mis-parses registries with ports and digest references. Prefer last-colon split and handle @digest form.
-echo "$ASSISTED_CHAT_IMG" -IMAGE=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f1) -TAG=$(echo "$ASSISTED_CHAT_IMG" | cut -d ":" -f2) +echo "$ASSISTED_CHAT_IMG" +if [[ "$ASSISTED_CHAT_IMG" == *"@"* ]]; then + # repo/image@sha256:<digest> — template expects IMAGE and IMAGE_TAG; consider updating template to accept full image. + IMAGE="${ASSISTED_CHAT_IMG%@*}" # repo/image + TAG="${ASSISTED_CHAT_IMG##*:}" # <digest> +else + IMAGE="${ASSISTED_CHAT_IMG%:*}" # before last colon + TAG="${ASSISTED_CHAT_IMG##*:}" # after last colon +fiUntil the template supports digests directly, verify the resolved image fields render a valid pull spec in the manifest:
#!/bin/bash set -euo pipefail echo "IMAGE=$IMAGE" echo "TAG=$TAG" rg -nPA3 'image:\s*\$\{IMAGE\}:\$\{IMAGE_TAG\}' template.yaml || true
🧹 Nitpick comments (5)
test/prow/entrypoint.sh (1)
18-18: Consider enabling the eval runner or removing the dead codeThe eval runner is commented out. If this is intentional for prow, add a short comment explaining why; otherwise, enable it.
scripts/ci_test.sh (1)
30-39: “Completed” is redundant; check Succeeded only (or use Ready for running tests)Pod phase is typically “Succeeded” for a completed run. “Completed” isn’t a pod phase. You can drop that branch.
- if [[ "$CURRENT_STATUS" == "Completed" ]]; then - echo "Pod ${POD_NAME} is successfully completed, exiting" - oc logs -n "$NAMESPACE" "$POD_NAME" - exit 0 - fiAlternatively, if this is a Job, consider watching the Job condition instead of the Pod phase.
template.yaml (1)
118-123: Typo/inconsistency: LIGHTSSPEED vs LIGHTSPEEDParameter/refs use
LIGHTSSPEED_*(double “SS”), which is inconsistent with other LIGHTSPEED names and easy to mistype in scripts.If possible, standardize to
LIGHTSPEED_STACK_POSTGRES_SSL_MODEacross template and scripts (deploy/CI) to reduce confusion. I can provide a follow-up diff if you want to do this renaming consistently.Also applies to: 179-181, 315-316, 324-325, 359-360
scripts/deploy_template.sh (2)
69-80: Handle clusters without Route API more defensivelyThe jq filter assumes
oc processreturns a Template/List with items. If it doesn’t, the filter will fail. Consider guarding for both cases.-oc process \ +oc process \ -p IMAGE="$IMAGE" \ -p IMAGE_TAG="$TAG" \ -p VERTEX_API_SECRET_NAME=vertex-service-account \ -p ASSISTED_CHAT_DB_SECRET_NAME=llama-stack-db \ -p USER_ID_CLAIM=client_id \ -p USERNAME_CLAIM=clientHost \ -p LIGHTSSPEED_STACK_POSTGRES_SSL_MODE=disable \ -p LLAMA_STACK_POSTGRES_SSL_MODE=disable \ -f template.yaml --local | - jq '. as $root | $root.items = [$root.items[] | '"$FILTER"']' | + jq 'if has("items") then .items = [.items[] | '"$FILTER"'] else . end' | oc apply -n "$NAMESPACE" -f -
21-24: Pin secret volume items in template.yamlTo prevent unintended keys from being mounted, explicitly map the
service_accountkey in your secret volume:• File:
template.yaml(around lines 495–498)- name: google-vertex-service-account secret: secretName: ${VERTEX_API_SECRET_NAME} + items: + - key: service_account + path: service_accountThis ensures the volume only exposes the
service_accountfile, matching thesubPathin your deploy script.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Makefile(2 hunks)scripts/ci_test.sh(1 hunks)scripts/deploy_template.sh(1 hunks)scripts/generate.sh(3 hunks)template-params.dev.env(1 hunks)template.yaml(11 hunks)test/prow/entrypoint.sh(1 hunks)test/prow/template.yaml(0 hunks)test/prow/template_patch.diff(0 hunks)
💤 Files with no reviewable changes (2)
- test/prow/template_patch.diff
- test/prow/template.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/generate.sh
- template-params.dev.env
- Makefile
🔇 Additional comments (1)
test/prow/entrypoint.sh (1)
14-14: Ensure raw token is written to fileThe current invocation on line 12 uses
jq '.access_token', which emits a JSON-quoted string. As a result,
echo "$OCM_TOKEN" > test/evals/ocm_token.txtwill write surrounding"characters and can break downstream consumers.Apply one of the following:
• Minimal patch (strip quotes at write time):
--- a/test/prow/entrypoint.sh +++ b/test/prow/entrypoint.sh @@ -14,1 +14,1 @@ -echo "$OCM_TOKEN" > test/evals/ocm_token.txt +echo "$OCM_TOKEN" | tr -d '"' > test/evals/ocm_token.txt• Preferred fix (emit a raw token at source):
--- a/test/prow/entrypoint.sh +++ b/test/prow/entrypoint.sh @@ -10,1 +10,1 @@ - -d "client_secret=$CLIENT_SECRET" | jq '.access_token') + -d "client_secret=$CLIENT_SECRET" | jq -r '.access_token')Please manually verify after running the entrypoint that
test/evals/ocm_token.txtcontains no leading or trailing quotes:set -euo pipefail test/prow/entrypoint.sh if grep -q '^"' test/evals/ocm_token.txt; then echo "⚠️ Found surrounding quotes in token file" >&2 exit 1 fi echo "✅ Token file looks unquoted"
| oc create secret generic -n "$NAMESPACE" assisted-chat-ssl-ci --from-file=client_id=/var/run/secrets/sso-ci/client_id \ | ||
| --from-file=client_secret=/var/run/secrets/sso-ci/client_secret | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make secret creation idempotent and use SECRETS_BASE_PATH consistently
- Re-running this script will fail because
oc create secretis unconditional. - You introduced
SECRETS_BASE_PATHbut hardcoded/var/run/secrets/....
Use apply-style creation and the base path:
-oc create secret generic -n "$NAMESPACE" assisted-chat-ssl-ci --from-file=client_id=/var/run/secrets/sso-ci/client_id \
- --from-file=client_secret=/var/run/secrets/sso-ci/client_secret
+oc create secret generic -n "$NAMESPACE" assisted-chat-ssl-ci \
+ --from-file=client_id="${SECRETS_BASE_PATH}/sso-ci/client_id" \
+ --from-file=client_secret="${SECRETS_BASE_PATH}/sso-ci/client_secret" \
+ 2>/dev/null || oc -n "$NAMESPACE" create secret generic assisted-chat-ssl-ci --dry-run=client -o yaml \
+ --from-file=client_id="${SECRETS_BASE_PATH}/sso-ci/client_id" \
+ --from-file=client_secret="${SECRETS_BASE_PATH}/sso-ci/client_secret" | oc apply -f -📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oc create secret generic -n "$NAMESPACE" assisted-chat-ssl-ci --from-file=client_id=/var/run/secrets/sso-ci/client_id \ | |
| --from-file=client_secret=/var/run/secrets/sso-ci/client_secret | |
| oc create secret generic -n "$NAMESPACE" assisted-chat-ssl-ci \ | |
| --from-file=client_id="${SECRETS_BASE_PATH}/sso-ci/client_id" \ | |
| --from-file=client_secret="${SECRETS_BASE_PATH}/sso-ci/client_secret" \ | |
| 2>/dev/null || oc -n "$NAMESPACE" create secret generic assisted-chat-ssl-ci --dry-run=client -o yaml \ | |
| --from-file=client_id="${SECRETS_BASE_PATH}/sso-ci/client_id" \ | |
| --from-file=client_secret="${SECRETS_BASE_PATH}/sso-ci/client_secret" | oc apply -f - |
🤖 Prompt for AI Agents
In scripts/ci_test.sh around lines 9 to 11, the script uses `oc create secret`
unconditionally and hardcodes `/var/run/secrets/...`, which fails on reruns and
ignores the new SECRETS_BASE_PATH; change the secret creation to an idempotent
apply-style pattern (generate the secret YAML via `oc create secret ...
--dry-run=client -o yaml` and pipe to `oc apply -f -`) and replace hardcoded
paths with "$SECRETS_BASE_PATH/client_id" and "$SECRETS_BASE_PATH/client_secret"
so the command is safe to re-run and respects the configured base path.
|
|
||
| sleep 5 | ||
| oc get pods -n "$NAMESPACE" | ||
| POD_NAME=$(oc get pods | tr -s ' ' | cut -d ' ' -f1 | grep assisted-chat-eval-tes) |
There was a problem hiding this comment.
Bug: querying pods without namespace and brittle parsing
oc get pods here omits -n "$NAMESPACE", potentially selecting from the default namespace. The parsing is also fragile and can return multiple pods.
Use -n, -o name, and pick a single match deterministically:
-POD_NAME=$(oc get pods | tr -s ' ' | cut -d ' ' -f1 | grep assisted-chat-eval-tes)
+POD_NAME=$(oc get pods -n "$NAMESPACE" -o name | grep -m1 '^pod/assisted-chat-eval-tes' | cut -d/ -f2)
+if [[ -z "${POD_NAME:-}" ]]; then
+ echo "Pod matching prefix 'assisted-chat-eval-tes' not found in namespace ${NAMESPACE}" >&2
+ oc get pods -n "$NAMESPACE"
+ exit 1
+fi🤖 Prompt for AI Agents
In scripts/ci_test.sh around line 16 the pod lookup uses a bare `oc get pods`
with fragile whitespace parsing and no namespace, which can return multiple
results; change the command to query the intended namespace with `-n
"$NAMESPACE"`, request machine-readable output with `-o name` so results are
like "pod/NAME", filter deterministically (e.g. use grep -m1 or head -n1 or a
precise selector) to pick a single pod, and avoid brittle tr/split/cut parsing
so the script reliably selects one pod from the specified namespace.
| while [ $ELAPSED -lt $TIMEOUT ]; do | ||
| # Check if the pod's status is "Running" | ||
| CURRENT_STATUS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.phase}') | ||
| CURRENT_RESTARTS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].restartCount}') | ||
| if [[ $CURRENT_RESTARTS -gt 0 ]]; then | ||
| echo "Pod ${POD_NAME} was restarted, so the tests should run at least once, exiting" | ||
| oc logs -n "$NAMESPACE" "$POD_NAME" | ||
| exit "$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].lastState.terminated.exitCode}')" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing containerStatuses to avoid integer comparison errors
Immediately after pod creation, .status.containerStatuses[0] may be absent, causing [[ $CURRENT_RESTARTS -gt 0 ]] to error. Default to 0 and use arithmetic comparison.
- CURRENT_RESTARTS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].restartCount}')
- if [[ $CURRENT_RESTARTS -gt 0 ]]; then
+ CURRENT_RESTARTS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].restartCount}')
+ CURRENT_RESTARTS=${CURRENT_RESTARTS:-0}
+ if (( CURRENT_RESTARTS > 0 )); then
echo "Pod ${POD_NAME} was restarted, so the tests should run at least once, exiting"
oc logs -n "$NAMESPACE" "$POD_NAME"
exit "$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].lastState.terminated.exitCode}')"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while [ $ELAPSED -lt $TIMEOUT ]; do | |
| # Check if the pod's status is "Running" | |
| CURRENT_STATUS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.phase}') | |
| CURRENT_RESTARTS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].restartCount}') | |
| if [[ $CURRENT_RESTARTS -gt 0 ]]; then | |
| echo "Pod ${POD_NAME} was restarted, so the tests should run at least once, exiting" | |
| oc logs -n "$NAMESPACE" "$POD_NAME" | |
| exit "$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].lastState.terminated.exitCode}')" | |
| fi | |
| while [ $ELAPSED -lt $TIMEOUT ]; do | |
| # Check if the pod's status is "Running" | |
| CURRENT_STATUS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.phase}') | |
| CURRENT_RESTARTS=$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].restartCount}') | |
| CURRENT_RESTARTS=${CURRENT_RESTARTS:-0} | |
| if (( CURRENT_RESTARTS > 0 )); then | |
| echo "Pod ${POD_NAME} was restarted, so the tests should run at least once, exiting" | |
| oc logs -n "$NAMESPACE" "$POD_NAME" | |
| exit "$(oc get pod "$POD_NAME" -n "$NAMESPACE" -o=jsonpath='{.status.containerStatuses[0].lastState.terminated.exitCode}')" | |
| fi |
🤖 Prompt for AI Agents
In scripts/ci_test.sh around lines 21 to 29, the code reads
.status.containerStatuses[0] which can be absent and causes integer comparison
errors; update CURRENT_RESTARTS to default to 0 when the jsonpath returns empty
(e.g. capture the oc output and set CURRENT_RESTARTS=${CURRENT_RESTARTS:-0}) and
replace the string comparison with an arithmetic comparison like if ((
CURRENT_RESTARTS > 0 )); then, and when exiting, guard the lastState exitCode
lookup similarly by defaulting to a safe exit code if the jsonpath is empty.
|
/unhold |
Having it in openshift/release is inconvenient, as it often needs to be modifies. It should just rely on the Makefile target like the install step. For now that target doesn't exist, but it will be added in. Doesn't matter because the CI doesn't work anyway. rh-ecosystem-edge/assisted-chat#126
Having it in openshift/release is inconvenient, as it often needs to be modifies. It should just rely on the Makefile target like the install step. For now that target doesn't exist, but it will be added in. Doesn't matter because the CI doesn't work anyway. rh-ecosystem-edge/assisted-chat#126
Having it in openshift/release is inconvenient, as it often needs to be modifies. It should just rely on the Makefile target like the install step. For now that target doesn't exist, but it will be added in. Doesn't matter because the CI doesn't work anyway. rh-ecosystem-edge/assisted-chat#126
Having it in openshift/release is inconvenient, as it often needs to be modifies. It should just rely on the Makefile target like the install step. For now that target doesn't exist, but it will be added in. Doesn't matter because the CI doesn't work anyway. rh-ecosystem-edge/assisted-chat#126
3 participants
See commit message
Summary by CodeRabbit
New Features
Chores
Tests