Build: [AEA-6258] - Use the new SSM parameter CDK construct#2518
Build: [AEA-6258] - Use the new SSM parameter CDK construct#2518
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
This PR is linked to a ticket in an NHS Digital JIRA Project. Here's a handy link to the ticket: AEA-6258 |
There was a problem hiding this comment.
Pull request overview
Adds a new packages/cdk workspace to the monorepo and scaffolds CDK apps/stacks/resources, while refactoring runtime config SSM lookups to be more resilient when parameters are missing/unreadable.
Changes:
- Refactor
PfPConfigSSM test-case lookups into a shared helper with warning+fallback behavior. - Introduce a new
packages/cdkworkspace with CDK apps, stacks, resources, scripts, and synth smoke tests. - Wire repository tooling to include the new workspace (Make targets, gitignore, VS Code workspace, lockfile updates).
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/utilities/src/config.ts | Refactors TC00x SSM param reads via a shared helper and adds logging on failure. |
| packages/cdk/vitest.config.ts | Adds Vitest configuration for the new CDK package. |
| packages/cdk/tsconfig.json | Adds TypeScript config for the CDK workspace. |
| packages/cdk/tests/synth.test.ts | Adds smoke tests that type-check and synth both CDK apps. |
| packages/cdk/stacks/PfPApiStack.ts | Defines the main stack and introduces the new SSM parameters construct usage. |
| packages/cdk/stacks/PfPApiSandboxStack.ts | Adds a sandbox stack placeholder. |
| packages/cdk/scripts/deletePrStacks.ts | Adds script to delete unused PR stacks. |
| packages/cdk/scripts/deleteMainStacks.ts | Adds script to delete unused main stacks. |
| packages/cdk/resources/StateMachines.ts | Adds Step Functions state machine wiring and IAM policies. |
| packages/cdk/resources/StateMachineDefinitions/GetMyPrescriptions.ts | Adds the GetMyPrescriptions state machine definition. |
| packages/cdk/resources/Functions.ts | Defines Lambda functions via shared constructs and policy wiring. |
| packages/cdk/resources/Apis.ts | Defines API Gateway + endpoints for the stack. |
| packages/cdk/package.json | Adds CDK workspace package definition and dependencies/scripts. |
| packages/cdk/nagSuppressions.ts | Adds cdk-nag suppression helpers for the stack resources. |
| packages/cdk/cdk.json | Adds CDK context and watch configuration. |
| packages/cdk/bin/PfPApiSandboxApp.ts | Adds CDK sandbox app entrypoint. |
| packages/cdk/bin/PfPApiApp.ts | Adds CDK main app entrypoint + env-driven config. |
| package.json | Adds packages/cdk to workspaces. |
| package-lock.json | Updates lockfile for the new workspace and dependencies. |
| Makefile | Adds CDK deploy/synth/diff/watch targets and includes cdk lint/test/clean. |
| .vscode/prescriptionsforpatients.code-workspace | Adds packages/cdk folder to the VS Code workspace. |
| .gitignore | Ignores cdk.out/ and .env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "outDir": "lib", | ||
| "noEmit": true, | ||
| "strict": false, | ||
| "lib": [ | ||
| "es2020" | ||
| ], | ||
| "noImplicitAny": true, | ||
| "strictNullChecks": true, | ||
| "noImplicitThis": true, | ||
| "alwaysStrict": true, |
There was a problem hiding this comment.
This tsconfig disables strict even though the repo default (tsconfig.defaults.json) enables it and other packages inherit that setting. Setting strict: false turns off several strictness checks (unless individually re-enabled) and can allow type issues to slip into the CDK code. If there's no strong reason to diverge, consider removing the override so the package stays aligned with the monorepo defaults.
| private readonly logger: Logger | ||
|
|
||
| constructor(ssmProvider?: SSMProvider) { | ||
| this.logger = new Logger() |
There was a problem hiding this comment.
Logger is instantiated without a serviceName, which is inconsistent with other Powertools logger usage in this repo and makes logs harder to attribute/filter. Consider passing a serviceName (and optionally logLevel) when constructing the logger (e.g., aligned to how other packages set it).
| this.logger = new Logger() | |
| this.logger = new Logger({serviceName: "pfp-config"}) |
| reason: "Suppress error for not implementing authorization. Token endpoint should not have an authorizer" | ||
| }, | ||
| { | ||
| id: "AwsSolutions-COG4", | ||
| reason: "Suppress error for not implementing a Cognito user pool authorizer. Token endpoint should not have an authorizer" |
There was a problem hiding this comment.
The suppression reasons mention a "Token endpoint", but this stack only defines Bundle and _status API resources. This looks like a copy/paste and makes the audit trail for the suppression misleading; please update the reason text to accurately describe why these endpoints intentionally have no authorization.
| reason: "Suppress error for not implementing authorization. Token endpoint should not have an authorizer" | |
| }, | |
| { | |
| id: "AwsSolutions-COG4", | |
| reason: "Suppress error for not implementing a Cognito user pool authorizer. Token endpoint should not have an authorizer" | |
| reason: "Suppress error for not implementing authorization on the Bundle and _status GET endpoints, which are intentionally unauthenticated read-only/public endpoints" | |
| }, | |
| { | |
| id: "AwsSolutions-COG4", | |
| reason: "Suppress error for not implementing a Cognito user pool authorizer on the Bundle and _status GET endpoints, which are intentionally unauthenticated read-only/public endpoints" |
| [ | ||
| { | ||
| id: "AwsSolutions-IAM5", | ||
| reason: "Suppress error for not having wildcards in permissions. This is a fine as we need to have permissions on all log streams under path" |
There was a problem hiding this comment.
Spelling/grammar in the suppression reason: "This is a fine" should be "This is fine".
| reason: "Suppress error for not having wildcards in permissions. This is a fine as we need to have permissions on all log streams under path" | |
| reason: "Suppress error for not having wildcards in permissions. This is fine as we need to have permissions on all log streams under path" |
| const awsEnvironment = getConfigFromEnvVar("AWS_ENVIRONMENT", "") | ||
| deleteUnusedMainStacks( | ||
| "pfp-api", | ||
| () => getActiveApiVersions("prescriptions-for-patients"), | ||
| `${awsEnvironment}.eps.national.nhs.uk.` |
There was a problem hiding this comment.
getConfigFromEnvVar("AWS_ENVIRONMENT", "") makes AWS_ENVIRONMENT effectively optional, but this script deletes stacks and uses the value to build the DNS zone string. If the env var is missing/empty, the computed zone becomes .eps.national.nhs.uk., which is risky. Prefer requiring AWS_ENVIRONMENT (no default) or validating it is non-empty before calling deleteUnusedMainStacks.
|
Summary
Details
Based on #2494