OLS-3327 - Bedrock tests - #2980
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds Bedrock ChangesBedrock end-to-end coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant E2EScript as e2e cluster script
participant Installer as ols_installer.py
participant Environment as test environment
participant Cluster as OpenShift cluster
E2EScript->>Installer: create_secrets(Bedrock provider)
Installer->>Environment: Read IAM or assume-role credentials
Installer->>Cluster: Create llmcreds secret
E2EScript->>Cluster: Run Bedrock suite
Cluster-->>E2EScript: Return query responses
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/utils/ols_installer.py`:
- Around line 288-291: The Bedrock credential selection in the helper that maps
`creds` to env keys currently treats every non-`iam_role` value as IAM, which
can hide bad discriminator values and break `create_secrets()` recovery. Update
the branching in the Bedrock env-key helper to accept only the supported
credential modes (`iam` and `iam_role`) and raise a clear failure for anything
else, so typos in `PROVIDER_KEY_PATH` do not silently choose the wrong key set.
In `@tests/scripts/test-e2e-cluster-periodics.sh`:
- Around line 75-76: The Bedrock tool-calling run is placed too early in the
periodic suite order, which can leave the tool-calling CR shape active for later
suites. Move the bedrock_deepseek_tool_calling run into the existing
tool_calling section in test-e2e-cluster-periodics.sh, alongside the other
tool-calling suites, so rhoai_vllm, rhelai_vllm, and certificates still run
after the non-tool-calling setup. Use the run_suite calls for
bedrock_deepseek_tool_calling and the surrounding tool_calling section as the
unique markers when relocating it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d69150a1-6347-4793-b261-2298cffdd1f4
📒 Files selected for processing (6)
tests/config/operator_install/olsconfig.crd.bedrock_anthropic.yamltests/config/operator_install/olsconfig.crd.bedrock_deepseek.yamltests/config/operator_install/olsconfig.crd.bedrock_deepseek_tool_calling.yamltests/e2e/utils/ols_installer.pytests/scripts/test-e2e-cluster-periodics.shtests/scripts/test-e2e-cluster.sh
0fe24bf to
75f3e95
Compare
addressing comments addressing comments
75f3e95 to
13fdbd7
Compare
|
/retest |
1 similar comment
|
/retest |
|
test_invalid_question failed because deepseek rejected answering "how to make a burger" in a different way from any other provider. |
|
/retest |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/test_query_endpoint.py (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM for the assertion broadening, but consider extracting the duplicated regex.
The
r"(sorry|questions|assist|help)"pattern is now duplicated in two test functions here and again intests/e2e/test_streaming_query_endpoint.py(line 76). If the accepted-keyword list changes again, all three locations must be updated in lockstep. Extracting a module-level constant (e.g.,INVALID_QUESTION_RESPONSE_PATTERN) would keep them aligned and reduce the risk of divergence.♻️ Suggested refactor: extract shared regex constant
+# Common pattern for responses to invalid/non-OCP questions. +INVALID_QUESTION_RESPONSE_PATTERN = r"(sorry|questions|assist|help)" + def test_invalid_question(): """Check the REST API /v1/query with POST HTTP method for invalid question.""" ... assert re.search( - r"(sorry|questions|assist|help)", + INVALID_QUESTION_RESPONSE_PATTERN, json_response["response"], re.IGNORECASE, ) ... def test_invalid_question_without_conversation_id(): ... assert re.search( - r"(sorry|questions|assist|help)", + INVALID_QUESTION_RESPONSE_PATTERN, json_response["response"], re.IGNORECASE, )Also applies to: 77-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_query_endpoint.py` around lines 47 - 51, Extract the duplicated invalid-response regex into a shared module-level constant such as INVALID_QUESTION_RESPONSE_PATTERN, then update both assertions in tests/e2e/test_query_endpoint.py and the corresponding assertion in test_streaming_query_endpoint.py to reference it, preserving the existing case-insensitive matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/e2e/test_query_endpoint.py`:
- Around line 47-51: Extract the duplicated invalid-response regex into a shared
module-level constant such as INVALID_QUESTION_RESPONSE_PATTERN, then update
both assertions in tests/e2e/test_query_endpoint.py and the corresponding
assertion in test_streaming_query_endpoint.py to reference it, preserving the
existing case-insensitive matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bb9f8059-cda7-49d1-98a7-726583ac3b4f
📒 Files selected for processing (8)
tests/config/operator_install/olsconfig.crd.bedrock_anthropic.yamltests/config/operator_install/olsconfig.crd.bedrock_deepseek.yamltests/config/operator_install/olsconfig.crd.bedrock_deepseek_tool_calling.yamltests/e2e/test_query_endpoint.pytests/e2e/test_streaming_query_endpoint.pytests/e2e/utils/ols_installer.pytests/scripts/test-e2e-cluster-periodics.shtests/scripts/test-e2e-cluster.sh
✅ Files skipped from review due to trivial changes (1)
- tests/config/operator_install/olsconfig.crd.bedrock_deepseek.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/config/operator_install/olsconfig.crd.bedrock_anthropic.yaml
- tests/config/operator_install/olsconfig.crd.bedrock_deepseek_tool_calling.yaml
- tests/scripts/test-e2e-cluster.sh
- tests/e2e/utils/ols_installer.py
- tests/scripts/test-e2e-cluster-periodics.sh
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
…p over assist adding a wait for ols and a retry to generated_service_certs_rotation, changed check condition for test_ca_service_certs_rotation and increased timeout (OKP pod takes longer to run)
5789793 to
6b32244
Compare
|
/retest |
1 similar comment
|
/retest |
|
failure was from known flakiness being investigated in parallel. retesting |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/retest |
1 similar comment
|
/retest |
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
ensure_bedrock_iam_secret can silently fail: unlike other providers where create_secrets always creates llmcreds (even with creds == "empty"), for Bedrock this function IS the only path that creates llmcreds — so a silent skip (when env vars are missing) means no credentials exist at all after create_secrets returns.
Consider raising instead of returning when the required env vars are absent — the test will fail anyway, and an explicit error with the missing variable names is more helpful than a pod crash minutes later when the operator cannot mount the secret.
f5a1de2 to
6de07ae
Compare
|
/retest |
1 similar comment
|
/retest |
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Invalid-question regex: avoid global help
Agree that DeepSeek may need different refusal wording, but adding bare help to the shared pattern weakens the check for all providers. A non-refusal like "I can help you make a burger" would now pass.
Prefer scoping the DeepSeek quirk instead of diluting every suite:
- Gate on
PROVIDER(e.g. allowhelponly forbedrock_deepseek), or - Use a tighter DeepSeek-oriented phrase if refusals are consistent (e.g.
can't help/cannot help).
| # Below assert is minimal due to model randomness. | ||
| assert re.search( | ||
| r"(sorry|questions|assist)", | ||
| r"(sorry|questions|assist|help)", |
There was a problem hiding this comment.
Adding bare help here applies to every provider suite, not just DeepSeek. Prefer a PROVIDER-gated pattern (or a tighter phrase like can't help) rather than weakening this assert globally.
Same change is also at line 78 in this file and in test_streaming_query_endpoint.py.
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Nit (non-blocking): document Bedrock PROVIDER_KEY_PATH overload in script headers
The new Bedrock call sites correctly note that PROVIDER_KEY_PATH carries "iam" / "iam_role" instead of a file path. The script headers (line 5 in both test-e2e-cluster.sh and test-e2e-cluster-periodics.sh) still say it is only a credentials file path, and they do not list the required BEDROCK_AWS_* / BEDROCK_ROLE_* env vars.
Docs polish only — not a blocker.
changing gemini model to valid one addressing comments
6de07ae to
fbfd3ee
Compare
Review note: OpenAI-prefix Bedrock coverage gapPriority: must-fix (vs OLS-3327 / What the AC/spec ask forBedrock
The planned second family was What this PR does
Why it matters
This is a coverage / AC mismatch, not a broken test run ( How to resolveEither:
Until one of those is done, merge either ships incomplete AC or leaves the written plan out of date. AI-assisted review finding — please validate before treating as a blocking request. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: blublinsky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@JoaoFula: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6a90271
into
openshift:main
2 participants
Description
add bedrock test files and adaptations to existing scripts
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Tests
Documentation
Chores