Conversation
There was a problem hiding this comment.
Pull request overview
This PR switches notification filtering to default to using supplier application IDs across all environments, and populates the PROD enabled supplier application IDs configuration used during releases.
Changes:
- Set
USE_APP_ID_FOR_NOTIFICATIONS_FILTERINGtotruein the SAM functions template so app-ID filtering is the default. - Update the PROD release workflow to pass a populated
ENABLED_SUPPLIER_APPLICATION_IDSCSV instead of the placeholder value.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
SAMtemplates/functions/main.yaml |
Flips the Lambda env flag so filtering uses application IDs by default. |
.github/workflows/release.yml |
Provides the PROD application ID allow-list used to populate the SSM parameter at deploy time. |
| # Remove this once we've confirmed that product ID based filtering is working as expected and we | ||
| # no longer need the ability to switch back to the old filtering method |
There was a problem hiding this comment.
The comment above this flag refers to confirming “product ID based filtering” and switching back to the “old filtering method”, but the flag is specifically about using application IDs for filtering. Please update/remove the comment so it accurately describes the current behaviour and what the rollback path actually is.
| # Remove this once we've confirmed that product ID based filtering is working as expected and we | |
| # no longer need the ability to switch back to the old filtering method | |
| # Keep this flag while application ID based filtering remains behind a rollback switch. | |
| # Set to false to revert to the legacy notifications filtering behaviour. |
| # Remove this once we've confirmed that product ID based filtering is working as expected and we | ||
| # no longer need the ability to switch back to the old filtering method | ||
| USE_APP_ID_FOR_NOTIFICATIONS_FILTERING: false | ||
| USE_APP_ID_FOR_NOTIFICATIONS_FILTERING: true |
There was a problem hiding this comment.
USE_APP_ID_FOR_NOTIFICATIONS_FILTERING is being set as a YAML boolean. Lambda environment variable values are strings, and the TypeScript code reads this from process.env as a string; relying on implicit coercion here is fragile and can fail schema/type validation in CloudFormation/SAM. Set the value explicitly as a string (e.g., "true").
| USE_APP_ID_FOR_NOTIFICATIONS_FILTERING: true | |
| USE_APP_ID_FOR_NOTIFICATIONS_FILTERING: "true" |
|
Summary
Details
This PR updates the environment variable flag so that all environments use application ID by default. It also populates the application IDs for PROD, but does not do so for any other environment.
When this is deployed, we MUST check that the number of notifications being sent per minute is unaffected. Use this splunk query to do that: TODO