-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AWS-SDK: Fixing broken exports #2410
Conversation
…the variable params
…d links regardless of email set-up. Then, if AWS is not set up, maybe create a direct email stream?
{(this.props.organizationData && | ||
this.props.organizationData.emailEnabled) ? | ||
" we'll e-mail you when it's done. " : | ||
" check your Spoke directory. "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried this locally, if we have AWS S3 set up, even if email isn't enabled, the data will save to S3, not in the spoke directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disregard my previous comment, the last change to this check fixed the issue I was seeing
}); | ||
const bucketName = process.env.AWS_S3_BUCKET_NAME; | ||
const command = new CreateBucketCommand({ Bucket : bucketName }); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to create a new bucket each time we export data, it's likely someone will already have bucket created for the data and want to upload directly to it
we can add a check, if the bucket exists, save the exported data, if it doesn't, create a new one and save
|
||
console.log(`S3 bucket "${bucketName}" already exists.`); | ||
} catch (error) { | ||
if (error.name === "NotFound") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (error.code === "NoSuchBucket")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the bucket is not found, it doesn't look like we get this error code, only the error name
894c7a5
to
48f095e
Compare
…voiding confusion. i.e. saying the export was stored locally when it was placed in an AWS bucket
@mau11 this is set up for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Fixes # (issue)
Fixes broken exports.
Description
During the upgrade to Node 20,
aws-sdk
had to be upgraded to V3. This was simple in nature, considering a code-mod existed. However, our specific way of initiating an S3 bucket was not covered by this code-mod, and subsequently failed. Theoretically, our implementation (still in V2) was still supported, but clearly not.This new implementation uses
aws-sdk
V3 api calls to create the S3 client and bucket. Additionally, we can set the region for the bucket, which is called from the environment variables.This PR:
AWS_S3_BUCKET_NAME=
Local Export:
AWS Export:
(NOTE) this UI is fixed with Dynamic Replies Feature - by MoveOn #2423
campaignExportUrl
to match AWS expiration (1 day)Tested on:
*Local download is not supported in production.
Checklist: