Reapply "PO-8886 Adding support for managed redis cluster sessions (#… - #357
Reapply "PO-8886 Adding support for managed redis cluster sessions (#…#357iamfrankiemoran wants to merge 1 commit into
Conversation
|
louisbriggs
left a comment
louisbriggs
left a comment
There was a problem hiding this comment.
Potential issue for existing consumers.
| domain!: string; | ||
| redisEnabled!: boolean; | ||
| redisConnectionString!: string; | ||
| redisClusterEnabled!: boolean; |
There was a problem hiding this comment.
[P0]: Build and API Integrity
Problem: redisClusterEnabled is added as a required property on the exported SessionStorageConfiguration type.
Why: Existing consumers that construct this config without the new field will fail TypeScript compilation, even though enableFor already treats a missing value as standalone Redis via sessionStorage.redisClusterEnabled === true. That makes this a breaking API change without a version bump or breaking-change migration note.
Fix: Make the field optional, or provide a default-compatible shape, for example redisClusterEnabled?: boolean;.
| const redisUrl = new URL(connectionString); | ||
| logger.info('Using Redis session store', `${redisUrl.protocol}//${redisUrl.host}`); | ||
| const client = createClient({ | ||
| private getReconnectStrategy() { |
iamfrankiemoran
left a comment
iamfrankiemoran
left a comment
There was a problem hiding this comment.
Looks like this change isn't actually needed:
hmcts/opal-frontend#2877
|
@marcmollins confirmed we have managed to do managed redis without this pull request so closing off |
3 participants
…3… (#356)
This reverts commit 56fc016.
Jira link
Change description
Testing done
Security Vulnerability Assessment
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist