291 - Refactor values.dev.yaml to contain only dev overrides#469
291 - Refactor values.dev.yaml to contain only dev overrides#469alexbottenberg wants to merge 6 commits intomasterfrom
Conversation
Plan covers Helm merge semantics, exact file changes for apps/api and apps/web, verification approach, and open questions about null vault handling in the HMCTS nodejs chart. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors Helm Changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
docs/tickets/291/plan.md (1)
193-200: Open questions are valuable — ensure they're resolved before merging.These questions correctly identify risks:
- Chart null-handling behaviour needs verification via
helm template.- Current local dev behaviour should be confirmed to understand if changes have observable effect.
- Local tooling invocation order should be verified.
Consider converting these to explicit checkboxes in
tasks.mdto track resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 092e9939-a3d6-4edd-b766-96453afe2a83
📒 Files selected for processing (5)
apps/api/helm/values.dev.yamlapps/web/helm/values.dev.yamldocs/tickets/291/plan.mddocs/tickets/291/tasks.mddocs/tickets/291/ticket.md
| * End to end tests are passing on local | ||
| * Github pipeline is passing |
There was a problem hiding this comment.
Minor text quality issues in acceptance criteria.
- Line 15: "End to end" should be hyphenated as "End-to-end".
- Line 16: "Github" should be "GitHub" (capital H).
📝 Proposed fix
- * End to end tests are passing on local
- * Github pipeline is passing
+ * End-to-end tests are passing on local
+ * GitHub pipeline is passing📝 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.
| * End to end tests are passing on local | |
| * Github pipeline is passing | |
| * End-to-end tests are passing on local | |
| * GitHub pipeline is passing |
🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Did you mean the adjective “End-to-end” (spelled with hyphens)?
Context: ...en needed. Acceptance criteria: * End to end tests are passing on local * Github pi...
(END_TO_END_HYPHEN)
[uncategorized] ~16-~16: The official name of this software platform is spelled with a capital “H”.
Context: ...nd to end tests are passing on local * Github pipeline is passing ## Comments ### C...
(GITHUB)
🎭 Playwright E2E Test Results257 tests 257 ✅ 24m 58s ⏱️ Results for commit b2c2b36. ♻️ This comment has been updated with latest results. |
…ation tests - Add subscription-notifications.spec.ts with tests for email notifications to single and multiple subscribers - Fix blob-ingestion GOV.UK Notify test to also skip when GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION is not set (previously ran and failed if only the API key was set) - Exclude generated/ from biome linting to prevent Prisma-generated files from failing lint checks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0edc160d-410b-43b4-84e3-0e8c7d401523
📒 Files selected for processing (3)
biome.jsone2e-tests/tests/api/blob-ingestion-notifications.spec.tse2e-tests/tests/subscription-notifications.spec.ts
✅ Files skipped from review due to trivial changes (1)
- biome.json
| test.skip( | ||
| !process.env.GOVUK_NOTIFY_API_KEY || !process.env.GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION, | ||
| "Skipping: GOVUK_NOTIFY_API_KEY or GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION not set" | ||
| ); |
There was a problem hiding this comment.
Guard CFT_VALID_TEST_ACCOUNT in the same skip condition.
This test still hard-fails at Line 74 if CFT_VALID_TEST_ACCOUNT is unset, even when the new skip condition passes.
Suggested patch
test.skip(
- !process.env.GOVUK_NOTIFY_API_KEY || !process.env.GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION,
- "Skipping: GOVUK_NOTIFY_API_KEY or GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION not set"
+ !process.env.GOVUK_NOTIFY_API_KEY ||
+ !process.env.GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION ||
+ !process.env.CFT_VALID_TEST_ACCOUNT,
+ "Skipping: GOVUK_NOTIFY_API_KEY, GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION, or CFT_VALID_TEST_ACCOUNT not set"
);📝 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.
| test.skip( | |
| !process.env.GOVUK_NOTIFY_API_KEY || !process.env.GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION, | |
| "Skipping: GOVUK_NOTIFY_API_KEY or GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION not set" | |
| ); | |
| test.skip( | |
| !process.env.GOVUK_NOTIFY_API_KEY || | |
| !process.env.GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION || | |
| !process.env.CFT_VALID_TEST_ACCOUNT, | |
| "Skipping: GOVUK_NOTIFY_API_KEY, GOVUK_NOTIFY_TEMPLATE_ID_SUBSCRIPTION, or CFT_VALID_TEST_ACCOUNT not set" | |
| ); |
| test("should create notification audit log entry when publication is posted for subscribed court @nightly", async ({ request }) => { | ||
| const testUser = await createTestUser("notify-test@test.hmcts.net"); | ||
| testData.userIds.push(testUser.userId); | ||
|
|
||
| const subscription = await createTestSubscription(testUser.userId, 9001); | ||
| testData.subscriptionIds.push(subscription.subscriptionId); | ||
|
|
||
| const token = await getApiAuthToken(); | ||
| const response = await request.post(ENDPOINT, { | ||
| data: VALID_PAYLOAD, | ||
| headers: { Authorization: `Bearer ${token}` } | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(201); | ||
| const result = await response.json(); | ||
| testData.publicationIds.push(result.artefact_id); | ||
|
|
||
| const notifications = await waitForNotifications(result.artefact_id); | ||
|
|
||
| const myNotifications = notifications.filter((n) => n.subscriptionId === subscription.subscriptionId); | ||
| expect(myNotifications.length).toBeGreaterThan(0); | ||
| expect(myNotifications[0].publicationId).toBe(result.artefact_id); | ||
| expect(myNotifications[0].userId).toBe(testUser.userId); | ||
| expect(["Sent", "Failed", "Pending"]).toContain(myNotifications[0].status); | ||
| }); | ||
|
|
||
| test("should send email notification with case summary to subscribers @nightly", async ({ request }) => { | ||
| const testUser = await createTestUser("subscriber-summary@test.hmcts.net"); | ||
| testData.userIds.push(testUser.userId); | ||
|
|
||
| const subscription = await createTestSubscription(testUser.userId, 9001); | ||
| testData.subscriptionIds.push(subscription.subscriptionId); | ||
|
|
||
| const token = await getApiAuthToken(); | ||
| const response = await request.post(ENDPOINT, { | ||
| data: VALID_PAYLOAD, | ||
| headers: { Authorization: `Bearer ${token}` } | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(201); | ||
| const result = await response.json(); | ||
| expect(result.success).toBe(true); | ||
| expect(result.artefact_id).toBeDefined(); | ||
| testData.publicationIds.push(result.artefact_id); | ||
|
|
||
| const notifications = await waitForNotifications(result.artefact_id); | ||
|
|
||
| const myNotifications = notifications.filter((n) => n.subscriptionId === subscription.subscriptionId); | ||
| expect(myNotifications.length).toBeGreaterThan(0); | ||
|
|
||
| const notification = myNotifications[0]; | ||
| expect(notification.publicationId).toBe(result.artefact_id); | ||
| expect(notification.userId).toBe(testUser.userId); | ||
| expect(notification.subscriptionId).toBe(subscription.subscriptionId); | ||
| expect(["Sent", "Pending", "Failed"]).toContain(notification.status); | ||
| expect(notification.createdAt).toBeDefined(); | ||
| }); | ||
|
|
||
| test("should send notifications to multiple subscribers for same publication @nightly", async ({ request }) => { | ||
| const userA = await createTestUser("subscriber-a@test.hmcts.net"); | ||
| testData.userIds.push(userA.userId); | ||
| const userB = await createTestUser("subscriber-b@test.hmcts.net"); | ||
| testData.userIds.push(userB.userId); | ||
|
|
||
| const subscriptionA = await createTestSubscription(userA.userId, 9001); | ||
| testData.subscriptionIds.push(subscriptionA.subscriptionId); | ||
| const subscriptionB = await createTestSubscription(userB.userId, 9001); | ||
| testData.subscriptionIds.push(subscriptionB.subscriptionId); | ||
|
|
||
| const token = await getApiAuthToken(); | ||
| const response = await request.post(ENDPOINT, { | ||
| data: VALID_PAYLOAD, | ||
| headers: { Authorization: `Bearer ${token}` } | ||
| }); | ||
|
|
||
| expect(response.status()).toBe(201); | ||
| const result = await response.json(); | ||
| expect(result.success).toBe(true); | ||
| testData.publicationIds.push(result.artefact_id); | ||
|
|
||
| const notifications = await waitForNotifications(result.artefact_id); | ||
|
|
||
| const notificationsForA = notifications.filter((n) => n.subscriptionId === subscriptionA.subscriptionId); | ||
| const notificationsForB = notifications.filter((n) => n.subscriptionId === subscriptionB.subscriptionId); | ||
|
|
||
| expect(notificationsForA.length).toBeGreaterThan(0); | ||
| expect(notificationsForB.length).toBeGreaterThan(0); | ||
|
|
||
| expect(notificationsForA[0].userId).toBe(userA.userId); | ||
| expect(notificationsForB[0].userId).toBe(userB.userId); | ||
|
|
||
| const allNotifications = await getNotificationsByPublicationId(result.artefact_id); | ||
| const ourNotifications = allNotifications.filter( | ||
| (n) => n.subscriptionId === subscriptionA.subscriptionId || n.subscriptionId === subscriptionB.subscriptionId | ||
| ); | ||
| expect(ourNotifications.length).toBeGreaterThanOrEqual(2); | ||
| }); |
There was a problem hiding this comment.
Please consolidate into one complete journey test with inline Welsh/a11y checks.
This spec currently splits the flow into multiple tests and does not include inline Welsh translation and accessibility validation in the journey itself.
As per coding guidelines, "E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests" and "Use AxeBuilder with Playwright to test accessibility inline within journey tests, not as separate tests."
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After dotenv was removed from production builds, the web server and db:seed script lost access to DATABASE_URL in local development. Pass --env-file=.env to tsx so env vars are available before module imports initialise the Prisma connection pool. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/package.json (1)
8-9: Redundant env loading with existing dotenv call.The
--env-file=../../.envflag loads environment variables at Node startup. However,apps/web/src/server.ts(lines 8-15) already callsdotenv.config({ path: path.join(__dirname, "../../../.env") })targeting the same file.This works correctly (dotenv won't override already-set variables), but it's redundant. Consider removing the
dotenv.config()call inserver.tsto avoid maintaining two paths to the same file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fde2c02-92a8-4efc-9696-c9df35bbcbf8
📒 Files selected for processing (2)
apps/web/package.jsonpackage.json
…in CI Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
2 participants
Summary
apps/api/helm/values.dev.yaml: replaced duplicatedpip-ss-kv-stgsecrets block (identical tovalues.yaml) withcath: nullto correctly suppress the production vault in local dev via Helm merge semanticsapps/web/helm/values.dev.yaml: removed all settings duplicated fromvalues.yaml(applicationPort,aadIdentityName,ingressHost,image,REDIS_HOST,BASE_URL,SSO_ALLOW_HTTP_REDIRECT, allGOVUK_NOTIFY_TEMPLATE_ID_*), keeping only genuine dev overrides (CFT_IDAM_URLand the fullpip-ss-kv-stg.secretsarray with dev-specific secret names)Closes #291
Test plan
helm template cath-api apps/api/helm -f apps/api/helm/values.yaml -f apps/api/helm/values.dev.yamland confirmcathvault is absent from rendered outputhelm template cath-web apps/web/helm -f apps/web/helm/values.yaml -f apps/web/helm/values.dev.yamland confirm dev SSO secret names andCFT_IDAM_URLare present🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests
Documentation