SSA-292 - Helix-shared-storage/HelixStorage - Bucket population #1083
Conversation
| CONTENT_BUS_BUCKET: contentBusBucket = process.env.CONTENT_BUS_BUCKET || 'helix-content-bus', | ||
| CODE_BUS_BUCKET: codeBusBucket = process.env.CODE_BUS_BUCKET || 'helix-code-bus', | ||
| MEDIA_BUS_BUCKET: mediaBusBucket = process.env.MEDIA_BUS_BUCKET || 'helix-media-bus', | ||
| CONFIG_BUS_BUCKET: configBusBucket = process.env.CONFIG_BUS_BUCKET || 'helix-config-bus', |
There was a problem hiding this comment.
- can we prefix those with
HLX_? - why the defaults? (they are already handled later)
There was a problem hiding this comment.
Removing.
You are saying just manage default in the methods [1] correct? (contentBus, etc).
My goal was to add context.env > process.env > hardcoded type logic. Always falling back to hardcoded so no changes are needed on your end to adopt.
[1] EXAMPLE
contentBus(disableR2 = false) { return this.bucket(process.env.CONTENT_BUS_BUCKET || 'helix-content-bus', disableR2); }
There was a problem hiding this comment.
no. now you have 2 places where it defaults. here, in fromContext() and in the contentBus() etc. methods.
so remove it here.
| @@ -616,6 +616,11 @@ export class HelixStorage { | |||
| CLOUDFLARE_R2_ACCESS_KEY_ID: r2AccessKeyId, | |||
| CLOUDFLARE_R2_SECRET_ACCESS_KEY: r2SecretAccessKey, | |||
| HELIX_STORAGE_DISABLE_R2: disableR2, | |||
| AWS_REGION: region = process.env.AWS_REGION || 'us-east-1', | |||
There was a problem hiding this comment.
why did you move this default to here?
There was a problem hiding this comment.
You are right. You are saying to just leave it where it is in constructor. Something like this right in the constructor? Potentially only leveraging what comes from opts.region and not populating it like EXAMPLE.
HERE:
region = 'us-east-1', accessKeyId, secretAccessKey,
EXAMPLE:
region = process.env.AWS_REGION || 'us-east-1', accessKeyId, secretAccessKey,
Constructor:
See Lines 668 to 676
| */ | ||
| constructor(opts = {}) { | ||
| const { | ||
| region = 'us-east-1', accessKeyId, secretAccessKey, | ||
| region, accessKeyId, secretAccessKey, |
There was a problem hiding this comment.
why did you remove the default value for region here?
Enabling variablized bucket support for helix-shared-storage/HelixStorage function. Tests validated on helix-admin and on this module:
helix admin >>> BEFORE:
1673 passing (33s)
3 pending
=============================== Coverage summary ===============================
Statements : 100% ( 27437/27437 )
Branches : 100% ( 5486/5486 )
Functions : 100% ( 747/747 )
Lines : 100% ( 27437/27437 )
helix admin >>> AFTER:
1673 passing (33s)
3 pending
=============================== Coverage summary ===============================
Statements : 100% ( 27437/27437 )
Branches : 100% ( 5486/5486 )
Functions : 100% ( 747/747 )
Lines : 100% ( 27437/27437 )
helix-shared-storage >>>BEFORE:
object uploaded to: helix-code-bus/foo
✔ can put object
43 passing (256ms)
helix-shared-storage >>> AFTER:
object uploaded to: helix-code-bus/foo
✔ can put object
45 passing (263ms)