Ses s3 initmodules - #213
Conversation
maxn990
left a comment
There was a problem hiding this comment.
looks great, one quick thing then feel free to merge! thanks!
dburkhart07
left a comment
There was a problem hiding this comment.
a few things i think would be good to clear up now that we are doing this. thanks in advance!
| const value = process.env[name]; | ||
| // Treat unset and empty/whitespace-only values as missing. | ||
| if (!value || value.trim().length === 0) { | ||
| throw new Error(`Missing required environment variable: ${name}`); |
There was a problem hiding this comment.
like the cognito module does, i think we should add a logger here, and just throw a warning not an error (same with ses). i know we were throwing an error beforehand, but i think that was not good in practice.
| exports: [EmailsService], | ||
| }) | ||
| export class EmailsModule {} | ||
| export class EmailsModule implements OnModuleInit { |
There was a problem hiding this comment.
nit: for consistency, can we just name this AWSSESModule?
| useFactory: () => { | ||
| // Create dummy client that is never used when email sending is set to false | ||
| if (process.env.SEND_AUTOMATED_EMAILS !== 'true') { | ||
| if (process.env.SEND_AUTOMATED_EMAILS.toLowerCase() !== 'true') { |
There was a problem hiding this comment.
we need to add an optional check here i think. the factory runs during dependency injection, so we could face issues here if this is unset.
| // Example: [s3Buckets.DOCUMENTS]: process.env.AWS_DOCUMENTS_BUCKET_NAME, | ||
| this.bucketNames = {} as Record<S3Buckets, string>; | ||
|
|
||
| for (const bucket of Object.values(S3Buckets) as unknown as S3Buckets[]) { |
There was a problem hiding this comment.
I just realized i think this will throw an error the way we have the environment variable creation setup. we should:
- Put in the README for s3 (or on line 5 in the module) to add all env bucket names into that const as well)
- In here, read the bucket names from the process.env and assign them to this.bucketNames. If we do step 1, we shouldnt need to throw any errors here, since the env variables will not missing so long as they are added to the const. We can then restructure the loop like this:
for (const bucket of Object.values(S3Buckets) as unknown as S3Buckets[]) {
this.bucketNames[bucket] = process.env[`AWS_${bucket}_BUCKET_NAME`] ?? '';
}
Making sure the user is aware in the README that all env vars need to follow the AWS_bucket_name_BUCKET_NAME format (likely just an update to example.env
ℹ️ Issue
Part of #140 using module initialization errors instead of having checks for environment variables happen in the services.
📝 Description
Write a short summary of what you added. Why is it important? Any member of C4C should be able to read this and understand your contribution -- not just your team members.
Briefly list the changes made to the code:
true), throws error/fails startup✔️ Verification
What steps did you take to verify your changes work? These should be clear enough for someone to be able to clone the branch and follow the steps themselves.
Provide screenshots of any new components, styling changes, or pages.

🏕️ (Optional) Future Work / Notes