Skip to content
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

Uploading compressed tarball file to S3 #250

Merged
merged 17 commits into from
Jun 2, 2023
Merged

Conversation

Beckyrose200
Copy link
Contributor

@Beckyrose200 Beckyrose200 commented May 31, 2023

https://eaflood.atlassian.net/browse/WATER-4019

This pull request is just a small part of a larger project that involves exporting all our database schemas, converting them into CSV files, and uploading them to our Amazon S3 bucket.

This PR's primary focus is to upload a single compressed tarball file to our S3 bucket rather then individual table files.
Additionally, it aims to improve error handling throughout the service, ensuring that errors are only handled at the top-level service and appropriately addressed without interrupting the subsequent schema export process.

@Beckyrose200 Beckyrose200 added the enhancement New feature or request label May 31, 2023
@Beckyrose200 Beckyrose200 self-assigned this May 31, 2023
@Beckyrose200 Beckyrose200 marked this pull request as ready for review June 1, 2023 10:36
Copy link
Contributor

@StuAA78 StuAA78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I noticed we're using readFileSync which we'd be better off avoiding if possible. The "define filename as a const" comment is just a small thing we could maybe refactor. The comment about _uploadToBucket() is more of a nice to have and it's a little dependent on what was decided on about error handling so I'm not going to insist we make changes to it at the moment if it needs any more than a few mins of work.

app/services/db-export/compressed-tarball.service.js Outdated Show resolved Hide resolved
app/services/db-export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/db-export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got some requested changes around service names and I can see Stuart has made some suggestions.

I didn't dig into the unit tests just yet but will do once these suggestions have been reviewed and rolled in.

app/services/db-export/compressed-tarball.service.js Outdated Show resolved Hide resolved
app/services/db-export/export-compressed-table.service.js Outdated Show resolved Hide resolved
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, bit more feedback. The main points are around the SendToS3BucketService which I'm happy to pair on to get sorted and the PR approved as soon as you have time.

@Beckyrose200 Beckyrose200 requested a review from Cruikshanks June 1, 2023 21:41
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@StuAA78 StuAA78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Beckyrose200 Beckyrose200 merged commit 6b218ff into main Jun 2, 2023
@Beckyrose200 Beckyrose200 deleted the fix-compressed-files branch June 2, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants