Add e2e test for on demand secret fetching for llm provider deployment#2439
Add e2e test for on demand secret fetching for llm provider deployment#2439npamudika wants to merge 3 commits into
Conversation
npamudika
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
Naduni Pamudika seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughAdded an end-to-end test for deploying an LLM provider that references an API key stored in a secret. The update includes:
The test flow now explicitly restarts the gateway controller and polls for the provider to become available, making the deployment check more reliable. WalkthroughChangesThis pull request adds an end-to-end test scenario for deploying an LLM provider whose API key is stored in a secret. It introduces a new Gherkin feature file, corresponding Go step implementations for creating a secret, creating an LLM provider referencing it, deploying it to a gateway, restarting the gateway-controller to force a startup full-sync, and polling the gateway management API until the provider is visible. Supporting changes extend the test Sequence Diagram(s)sequenceDiagram
participant Test as world (test steps)
participant PlatformAPI as platform-api
participant GatewayController as gateway-controller
participant Gateway as gateway management API
Test->>PlatformAPI: create GENERIC secret (multipart)
PlatformAPI-->>Test: secret handle
Test->>PlatformAPI: create LLM provider (references secret handle)
PlatformAPI-->>Test: provider ID
Test->>PlatformAPI: deploy provider to gateway
PlatformAPI-->>Test: deployment ID
Test->>GatewayController: restart (force startup full-sync)
loop until 200 or timeout
Test->>Gateway: poll for provider configuration
Gateway-->>Test: HTTP status
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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: 1
🧹 Nitpick comments (2)
tests/integration-e2e/llm_provider_steps_test.go (2)
166-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded gateway management credentials.
The polling request uses fixed basic-auth credentials (
admin/admin) rather than a configured constant/suite value. Consider sourcing these from suite configuration for consistency with other API calls in this file.🤖 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/integration-e2e/llm_provider_steps_test.go` around lines 166 - 184, The polling loop in llm_provider_steps_test.go is hardcoding gateway basic-auth credentials in the request setup. Update the request creation in the polling helper to use the same suite-configured admin credentials used elsewhere in this file instead of literal values, so the auth source stays consistent across the test suite.
46-58: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUnchecked error from
mw.Close().
mw.Close()can return an error if writing the multipart trailer fails; it's currently discarded, which could mask a malformed request body.🛡️ Suggested fix
- mw.Close() + if err := mw.Close(); err != nil { + return err + }🤖 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/integration-e2e/llm_provider_steps_test.go` around lines 46 - 58, The multipart request builder ignores the result of mw.Close(), which can hide failures when finalizing the body. In the setup code that uses multipart.NewWriter and WriteField, capture and return the error from mw.Close() just like the other write errors, so the helper fails if the multipart trailer cannot be written.
🤖 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/integration-e2e/llm_provider_steps_test.go`:
- Around line 160-164: The documentation comment for waitGatewayLLMProvider is
inconsistent with the actual gateway management endpoint being queried. Update
the comment to match the implemented /api/management/v1/llm-providers/{id} path,
or adjust the URL in waitGatewayLLMProvider if v0.9 is the intended API version,
so the comment and implementation align.
---
Nitpick comments:
In `@tests/integration-e2e/llm_provider_steps_test.go`:
- Around line 166-184: The polling loop in llm_provider_steps_test.go is
hardcoding gateway basic-auth credentials in the request setup. Update the
request creation in the polling helper to use the same suite-configured admin
credentials used elsewhere in this file instead of literal values, so the auth
source stays consistent across the test suite.
- Around line 46-58: The multipart request builder ignores the result of
mw.Close(), which can hide failures when finalizing the body. In the setup code
that uses multipart.NewWriter and WriteField, capture and return the error from
mw.Close() just like the other write errors, so the helper fails if the
multipart trailer cannot be written.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0ae5b03-f215-42cc-ba52-647a89ff7043
📒 Files selected for processing (5)
tests/integration-e2e/docker-compose.yamltests/integration-e2e/features/llm_provider.featuretests/integration-e2e/llm_provider_steps_test.gotests/integration-e2e/steps_test.gotests/integration-e2e/suite_test.go
npamudika
force-pushed
the
platform-api-secret-management
branch
from
7d21f34 to
68d65d3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration-e2e/llm_provider_steps_test.go (1)
123-158: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep this scenario on the deployment-event path. Restarting
gateway-controllershifts the test to startup full-sync, so it no longer exercises the on-demand secret fetch described by the feature. If that restart is intentional, update the scenario wording; otherwise remove it and let the deployment event drive the assertion.🤖 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/integration-e2e/llm_provider_steps_test.go` around lines 123 - 158, The deployLLMProviderToGateway helper is switching this test onto the gateway-controller startup full-sync path, which bypasses the deployment-event flow the scenario is meant to cover. In world.deployLLMProviderToGateway, remove the compose restart of gateway-controller so the assertion is driven by the deployment event and on-demand secret fetch; if the restart must stay, update the test scenario wording to match the new behavior.
🧹 Nitpick comments (1)
tests/integration-e2e/llm_provider_steps_test.go (1)
178-199: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCapture the last error for a more actionable timeout message.
When
httpClient.Dofails repeatedly (e.g., connection refused during controller restart),lastStatusstays0and the final error only reports "last status 0", hiding the actual connection failure. Tracking the last error alongside status would aid triage of flaky/timeout-related CI failures.🔍 Suggested improvement
var lastStatus int + var lastErr error for time.Now().Before(deadline) { req, err := http.NewRequest(http.MethodGet, url, nil) if err != nil { return err } req.SetBasicAuth("admin", "admin") resp, err := httpClient.Do(req) if err != nil { + lastErr = err time.Sleep(2 * time.Second) continue } io.Copy(io.Discard, resp.Body) //nolint:errcheck resp.Body.Close() lastStatus = resp.StatusCode if lastStatus == http.StatusOK { return nil } time.Sleep(2 * time.Second) } - return fmt.Errorf("gateway did not configure LLM provider %q within timeout: last status %d", - providerID, lastStatus) + return fmt.Errorf("gateway did not configure LLM provider %q within timeout: last status %d, last error: %v", + providerID, lastStatus, lastErr)🤖 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/integration-e2e/llm_provider_steps_test.go` around lines 178 - 199, The retry loop in the LLM provider wait helper only tracks lastStatus, so repeated httpClient.Do failures end with an unhelpful “last status 0” message. Update the logic in this helper to also keep the most recent request error from httpClient.Do, preserve it through retries, and include it in the final fmt.Errorf timeout message alongside providerID and lastStatus. Use the existing request loop and the lastStatus variable to locate the change, and make the timeout error report whichever of status or error best explains the failure.
🤖 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.
Outside diff comments:
In `@tests/integration-e2e/llm_provider_steps_test.go`:
- Around line 123-158: The deployLLMProviderToGateway helper is switching this
test onto the gateway-controller startup full-sync path, which bypasses the
deployment-event flow the scenario is meant to cover. In
world.deployLLMProviderToGateway, remove the compose restart of
gateway-controller so the assertion is driven by the deployment event and
on-demand secret fetch; if the restart must stay, update the test scenario
wording to match the new behavior.
---
Nitpick comments:
In `@tests/integration-e2e/llm_provider_steps_test.go`:
- Around line 178-199: The retry loop in the LLM provider wait helper only
tracks lastStatus, so repeated httpClient.Do failures end with an unhelpful
“last status 0” message. Update the logic in this helper to also keep the most
recent request error from httpClient.Do, preserve it through retries, and
include it in the final fmt.Errorf timeout message alongside providerID and
lastStatus. Use the existing request loop and the lastStatus variable to locate
the change, and make the timeout error report whichever of status or error best
explains the failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e17c93af-9f58-40a5-a792-8203f392fd4e
📒 Files selected for processing (2)
tests/integration-e2e/llm_provider_steps_test.gotests/integration-e2e/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration-e2e/suite_test.go
2 participants
Purpose
Add e2e test for on demand secret fetching for llm provider deployment