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.

Sign up for GitHub

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

Jump to bottom

3855 move files to s3 bucket #3898

Closed
wants to merge 17 commits into from
Closed

3855 move files to s3 bucket #3898

wants to merge 17 commits into from

Conversation

sorja
Copy link
Contributor

@sorja sorja commented Aug 30, 2024 •
edited
Loading

implementation:

  • create a bucket to s3
  • implement new service FileStorage to write and read files
  • remove column file from public.file

todo:

  • src/server/repository/public/ddl/getCreatePublicSchemaDDL.ts update public.file with public migration
  • investigate if we want to remove public.file

todo/check:

  • upload and view avatar
  • upload and view file from Links and repository
  • upload and view file from EditorWYSIWYG

@sorja sorja linked an issue Aug 30, 2024 that may be closed by this pull request
Move files to s3 bucket #3855
Open
sorja
sorja commented Aug 30, 2024
View reviewed changes
.env.template
Comment on lines +57 to +60
AWS_ACCESS_KEY_ID=your-access-key-id
AWS_SECRET_ACCESS_KEY=your-secret-access-key
AWS_REGION=eu-west-1
S3_BUCKET_NAME=fra-platform-s3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to test this branch setting these env vars in .env

sorja
sorja commented Aug 30, 2024
View reviewed changes
src/test/migrations/steps/20240829112027-step-files-to-s3.ts
Comment on lines +32 to +39
// This should be changed to all files,
// left for testing purpose (all files 5.7gb, this subset around 80mb)
const files = await client.query(`
select * from public.file
where
octet_length(file) / 1024.0 / 1024.0 < 2
and name ilike '%fin%';
`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: only subset is migrated for testing purposes

@sorja sorja force-pushed the 3855-move-files-to-s3-bucket branch from 2bf6ff9 to b570cd6 Compare August 30, 2024 11:26
sorja
sorja commented Sep 4, 2024
View reviewed changes
src/test/migrations/steps/20240829112027-step-files-to-s3.ts
@@ -0,0 +1,96 @@
import * as path from 'path'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

major change 1

sorja
sorja commented Sep 4, 2024
View reviewed changes
src/server/service/fileStorage/fileStorage.ts
@@ -0,0 +1,77 @@
import { Readable } from 'stream'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

major change 2

@sorja
Copy link
Contributor Author

sorja commented Sep 4, 2024

@minotogna

Purpose of PR:
Remove column file from public.file and move it to s3
This is done after migrating all files to s3 in migration step:
https://github.com/openforis/fra-platform/pull/3898/files#diff-83be78115ed816ab6b8367dbf7871927d63af070b0ab471dda8ed408d6f24952

We introduce new service FileStorage
https://github.com/openforis/fra-platform/pull/3898/files#diff-ae9344079271ecc6b3715c44f69829251ffc276f58f15662569b1b68634d0e8e

With following methods:
getFile
uploadFile
getFileSize
fileExists

The base props taken are { path?: string, key: string }
Where the key is the file name in S3, which for us is the UUID of public.file
The is optional string that defaults to public, later we might have static for static files, tad etc

@sorja
Copy link
Contributor Author

sorja commented Sep 4, 2024

We also deprecate static file server:
src/server/service/file_deprecated/index.ts

This is to migrate the files later to S3

minotogna
minotogna reviewed Sep 16, 2024
View reviewed changes
src/server/api/cycleData/print/report.ts
Comment on lines +79 to +83
const chunks = []
// eslint-disable-next-line no-restricted-syntax
for await (const chunk of cachedPdfInfo.file.file) {
chunks.push(chunk)
}
Copy link
Member

Choose a reason for hiding this comment

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

should we use Promises.each ?

src/server/api/file/get.ts
@@ -1,6 +1,6 @@
import { Request, Response } from 'express'

import { FileRepository, fileTypes } from 'server/service/file'
import { FileRepository, fileTypes } from 'server/service/file_deprecated'
Copy link
Member

Choose a reason for hiding this comment

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

why do we have file_deprecated ?

@minotogna minotogna deleted the branch next September 26, 2024 18:48
@minotogna minotogna closed this Sep 26, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 27, 2024

⚠️ The sha of the head commit of this PR conflicts with #3978. Mergify cannot evaluate rules on this PR. ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minotogna minotogna minotogna left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move files to s3 bucket
2 participants
@sorja @minotogna