feat(backend): migrate to s3 #8545
Conversation
roschaefer
force-pushed
the
migrate-to-s3
branch
3 times, most recently
from
97b8cae to
f3d1dcd
Compare
Side quest of #8545. Looks like this file is not being used at all.
Side quest of #8545. Looks like this file is not being used at all.
Side quest of #8545. Looks like this file is not being used at all.
roschaefer
force-pushed
the
migrate-to-s3
branch
2 times, most recently
from
636dff9 to
2d0074b
Compare
| // eslint-disable-next-line @typescript-eslint/await-thenable | ||
| await getNeode().schema.install() | ||
|
|
||
| getNeode().schema.install() |
There was a problem hiding this comment.
you must wait it - it doesnt work properly if you don't - I tested it - see comment above
There was a problem hiding this comment.
Ah, maybe their types are just broken. It does return a promise?
There was a problem hiding this comment.
The types are broken. It is a Promise thats returned. This library is just a nightmare.
| export async function up(_next) { | ||
| if (CONFIG.NODE_ENV === 'test') { | ||
| // Let's skip this migration for simplicity. | ||
| // There is nothing to migrate in test environment and setting up the S3 services seems overkill. |
There was a problem hiding this comment.
i agree that it is not necessary for test environment to migrate stuff, but since the s3 will be the only option for image handling we need to set it up as well. The E2E tests for example test for correct behaviour if I am correct.
There was a problem hiding this comment.
OK! I can for sure setup a test S3 service - it might slow down CI a bit.
There was a problem hiding this comment.
You will find an appropriate solution - just have a look at https://github.com/Ocelot-Social-Community/Ocelot-Social/blob/master/cypress/e2e/UserProfile.Avatar.feature
|
|
||
| try { | ||
| // Implement your migration here. | ||
| await transaction.run(``) |
There was a problem hiding this comment.
i suggest to just throw here and forget about it
| @@ -0,0 +1,134 @@ | |||
| /* eslint-disable @typescript-eslint/require-await */ | |||
There was a problem hiding this comment.
will we still support local images? You remove some parts of the corresponding code - the migration throws an error if you do not have an s3 configured.
There was a problem hiding this comment.
I was hesitating to remove the code before the migration is done. Maybe I will remove it straight away when I refactor (e.g. pass s3Client via context) so I don't have to maintain it.
There was a problem hiding this comment.
Ok, just for my understanding - its fine to keep the code for a moment for reference
| export const sanitizeRelationshipType = (relationshipType: string) => { | ||
| // Cypher query language does not allow to parameterize relationship types | ||
| // See: https://github.com/neo4j/neo4j/issues/340 | ||
| if (!['HERO_IMAGE', 'AVATAR_IMAGE'].includes(relationshipType)) { |
There was a problem hiding this comment.
probably not your problem, but there are solutions to this problem
- For create with apoc: Cypher: Allow parameters for relationship types neo4j/neo4j#340 (comment)
- For query with normal cypher: Cypher: Allow parameters for relationship types neo4j/neo4j#340 (comment)
There was a problem hiding this comment.
Yes, this code was here before. Maybe I can add this as a separate PR.
Side quest of #8545. Looks like this file is not being used at all.
Side quest of #8545. Looks like this file is not being used at all.
I think we can merge this already. The S3 client dependency is outdated and since the code is behind a feature flag it doesn't get executed. Why not merge it then?
I think we can merge this already. The S3 client dependency is outdated and since the code is behind a feature flag it doesn't get executed. Why not merge it then?
This is a work-in-progress. I'm already able to start minio and
upload+delete pictures.
Todos:
* tests (manual if necessary)
* look into `mergeImage`
* in development the domain for the images is `minio` instead of `localhost`
# commits
* fix: order of cli command
* fix build
* feat: add required migration for s3 storage
* refactor: more typescript fixes
* refactor: DRY, typescript
* fix docker build
* build(backend): update aws s3 package
This will fix the problem of differing domains in docker-compose locally but it also sets the ground for a later use of a image resize service.
TODOs: * does it make sense to store with the image where it has been saved to? * `await` delete callback
TODO: * remove kubernetes code for uploads in the future
Side quest of #8545. Looks like this file is not being used at all.
e.g. let the migration crash if some files cannot be found on disk
Side quest of #8545. Looks like this file is not being used at all.
🍰 Pullrequest
Issues
Todo