feat!: multi tile storage destinations including fs#30
Conversation
melancholiai
commented
Oct 3, 2023
melancholiai
commented
| Question | Answer |
|---|---|
| Bug fix | ✖ |
| New feature | ✔ |
| Breaking change | ✔ |
| Deprecations | ✖ |
| Documentation | ✔ |
| Tests added | ✔ |
| Chore | ✖ |
| } | ||
| "providers": { | ||
| "__name": "TILES_STORAGE_PROVIDERS", | ||
| "__format": "json" |
There was a problem hiding this comment.
we can't validate its schema right?
There was a problem hiding this comment.
added ajv on the factory function
| }); | ||
| cleanupRegistry.register({ | ||
| func: async () => { | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
shouldnt we use server.drain also ?
There was a problem hiding this comment.
no such function
|
|
||
| private readonly jobFinishedEmitter = new EventEmitter(); | ||
| private readonly jobFinishedEventName = 'avi'; | ||
| private readonly jobFinishedEventName = 'jobFinished'; |
| func: async () => { | ||
| return new Promise((resolve) => { | ||
| (s3Client as S3Client).destroy(); | ||
| resolve(undefined); |
There was a problem hiding this comment.
isn't it better to return resolve(undefined);?
| if (!existsSync(dir)) { | ||
| await mkdir(dir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
should we make a dir on our own ? what about permissions and so on, I think the server should fail if path does not exists or is not writeable or readable. we should not change the state of the storage.
There was a problem hiding this comment.
the tiles storage dir is set according to the fs storageProvider's basePath + app.tilesStorage.layout.format configuration value, the latter value changes from time to time.
I don't see a downside to attempt to create the dir, remember it also creates the z/x/y directories.
another manual step is another pain in the ass, and anyway if there are insufficient permissions to write the file the job will fail with a proper error.
| functions: 80, | ||
| lines: 80, | ||
| statements: -10, | ||
| statements: -20, |
There was a problem hiding this comment.
do we have a reason to lower the standard?
There was a problem hiding this comment.
done, I've added a couple of tests to achieve the standard
2 participants
