CCM-16553: Deployment into nft environment in NonProd#293
CCM-16553: Deployment into nft environment in NonProd#293nhsd-angel-pastor wants to merge 3 commits intomainfrom
Conversation
nhsd-angel-pastor
force-pushed
the
feature/CCM-16553_dl_nft
branch
6 times, most recently
from
0abe021 to
07dced7
Compare
| * Common interface for all event destinations (SQS, EventBridge, etc.). | ||
| * Implementations are responsible for batching, retries and back-off. | ||
| */ | ||
| export interface DestinationClient { |
There was a problem hiding this comment.
Originally did this to support in the future other destinations like SQS so the code in SMS nudge could work, but then I removed the SQS client as for the current case it isn't used, but I left this if we want to reintroduce SQS or other destination.
nhsd-angel-pastor
force-pushed
the
feature/CCM-16553_dl_nft
branch
from
01acdc1 to
d5ff9a7
Compare
gareth-allan
left a comment
gareth-allan
left a comment
There was a problem hiding this comment.
I've not reviewed the nft-event-generator application yet, but one comment on everything else.
nhsd-angel-pastor
force-pushed
the
feature/CCM-16553_dl_nft
branch
from
d5ff9a7 to
81f8886
Compare
nhsd-angel-pastor
force-pushed
the
feature/CCM-16553_dl_nft
branch
from
f4fa472 to
91ed32e
Compare
| "bump-terraform-modules": "ts-node terraform/bump-shared-modules.ts", | ||
| "lint": "eslint nft-event-generator/", | ||
| "lint:fix": "eslint nft-event-generator/ --fix", | ||
| "start": "tsx nft-event-generator/src/cli.ts", |
There was a problem hiding this comment.
I support the use of tsx, given ts-node hasn't had an update in 2 years, but we're already using ts-node for the bump-terraform-modules command here. It seems silly to have two dependencies for doing the same thing, so we should settle on one. Do you know if we use one or the other elsewhere in Digital Letters?
| "compilerOptions": { | ||
| "baseUrl": ".", | ||
| "isolatedModules": true | ||
| "baseUrl": "./nft-event-generator/src", |
There was a problem hiding this comment.
Does changing this baseUrl affect the terraform/bump-shared-modules.ts script? It seems odd to have the baseUrl for the scripts application pointing at the nft-event-generator subdirectory. What if we add other applications?
| "jest": [ | ||
| "../../../node_modules/jest" | ||
| ] | ||
| }, | ||
| "typeRoots": [ | ||
| "../node_modules/@types", | ||
| "./node_modules/@types" | ||
| ] |
There was a problem hiding this comment.
Why are we referencing node_modules directories several levels above this one? We'd normally let NPM work this sort of thing out, wouldn't we?
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^24.0.10", | ||
| "@jest/diff-sequences": "^30.0.1", |
There was a problem hiding this comment.
I don't think you're using this dependency, are you?
| "@jest/diff-sequences": "^30.0.1", |
| "@nhsdigital/nhs-notify-event-schemas-status-published": "1.0.1", | ||
| "@nhsdigital/nhs-notify-event-schemas-supplier-api": "1.0.17", | ||
| "csv-parse": "^6.1.0", | ||
| "digital-letters-events": "^0.0.1", |
There was a problem hiding this comment.
Are you using this digital-letters-events dependency?
| function wait(interval: number) { | ||
| return new Promise((resolve) => { | ||
| setTimeout(resolve, interval); | ||
| }); | ||
| } |
There was a problem hiding this comment.
utils/utils/src/util-retry/sleep.ts exports a sleepMs function that does the same as this.
| currentBatch += 1; | ||
|
|
||
| const entries = batch.map((event) => { | ||
| const cloudEvent = event as unknown as CloudEventEnvelope; |
There was a problem hiding this comment.
Why not just define the events argument as CloudEventEnvelope[] (or add the source, type and time properties to the PublishableEvent interface), rather than casting here?
I'm assuming the idea was to keep PublishableEvent generic enough to support multiple publishing methods, but I think it's probably reasonable to say that they all need to accept events in the shape of CloudEvents.
| const events: LetterEvent[] = []; | ||
|
|
||
| for (let i = 0; i < numberOfEvents; i++) { | ||
| events.push( | ||
| generateSupplierApiLetterEvent(environment, status, { | ||
| id, | ||
| time, | ||
| subject, | ||
| messageReference, | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This could use Array.from, I think. Like you've done in send-events-to-event-bus.test.ts
| const events: LetterEvent[] = []; | |
| for (let i = 0; i < numberOfEvents; i++) { | |
| events.push( | |
| generateSupplierApiLetterEvent(environment, status, { | |
| id, | |
| time, | |
| subject, | |
| messageReference, | |
| }), | |
| ); | |
| } | |
| const events = Array.from({ length: numberOfEvents }, () => | |
| generateSupplierApiLetterEvent(environment, status, { | |
| id, | |
| time, | |
| subject, | |
| messageReference, | |
| }), | |
| ); |
There was a problem hiding this comment.
very good point
| }; | ||
|
|
||
| function generatePaperLetterOptOutEvent( | ||
| environment: string, |
There was a problem hiding this comment.
environment isn't used. Should it be going into the source field, as with the other event?
nhsd-angel-pastor
force-pushed
the
feature/CCM-16553_dl_nft
branch
2 times, most recently
from
b9e566d to
bc42881
Compare
nhsd-angel-pastor
force-pushed
the
feature/CCM-16553_dl_nft
branch
from
bc42881 to
35ebe4f
Compare
3 participants
Description
Deployment of main into nft and main environments for NonProd account. Added tool to publish events from the SupplierAPI and from Core (for the opt out letter). Evidences under comment
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.