feat(marxan-runner): pipe assets to input files #293
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). marxan – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan/AcsmGxqhqFMMWRpj96aND9Txhgwh marxan-storybook – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/6inJbVoBaibdPEf9e6srL3Lj5cXT |
kgajowy
force-pushed
the
feat/MARXAN-42-MARXAN-486-input-dat-2
branch
from
8437e38 to
889276a
Compare
kgajowy
force-pushed
the
feat/MARXAN-42-MARXAN-486-input-dat-2
branch
from
889276a to
bff2cee
Compare
kgajowy
force-pushed
the
feat/MARXAN-42-MARXAN-486-input-dat-2
branch
from
bff2cee to
609bbe9
Compare
kgajowy
requested review from
hotzevzl and
Dyostiq
and removed request for
Dyostiq
| assetUrl: `http://localhost:3000/puvsp.dat`, | ||
| targetRelativeDestination: `input/puvsp.dat`, | ||
| }, | ||
| { | ||
| name: `puvsp_sporder.dat`, | ||
| assetUrl: `http://localhost:3000/puvsp_sporder.dat`, | ||
| targetRelativeDestination: `input/puvsp_sporder.dat`, |
There was a problem hiding this comment.
could be but it was taken directly from marxan's example ; )
There was a problem hiding this comment.
@kgajowy which warez site did you download this copy of Marxan from? 😛
they're listed as puvspr.dat and puvspr_sporder.dat in the manual - no big deal of course as we can name these files as we wish, but just noting this to keep to the defaults mentioned in the manual
There was a problem hiding this comment.
| constructor(private readonly httpService: HttpService) {} | ||
|
|
||
| async include(directory: string, assets: Assets): Promise<void> { | ||
| // TODO security: ensure that none of the target filename starts with either `/` or `.` (tree traversing) |
There was a problem hiding this comment.
| return new Promise((resolve, reject) => { | ||
| const writer = createWriteStream(dest); | ||
| writer.on('finish', resolve); | ||
| writer.on('error', reject); | ||
| // "stream" seems to be broken, does not return stream but data itself | ||
| // https://github.com/nestjs/nest/issues/4144 - not really working | ||
| this.httpService | ||
| .get(sourceUri, { | ||
| responseType: 'stream', | ||
| }) | ||
| .toPromise() | ||
| .then((response) => { | ||
| writer.write(response.data); | ||
| writer.end(); | ||
| }) | ||
| .catch(reject); | ||
| }); |
There was a problem hiding this comment.
hmm, so if stream does not work and it is not really streaming, why do you use createWriteStream? I'd suggest to just use writeFile
There was a problem hiding this comment.
Thought maybe someone can pinpoint some obvious thing I missed. :(
| import { Injectable } from '@nestjs/common'; | ||
| import { InputFiles } from '../../ports/input-files'; | ||
| import { resolve, dirname } from 'path'; | ||
| import { createWriteStream, promises } from 'fs'; |
There was a problem hiding this comment.
import { createWriteStream, promises as fsPromises } from 'fs';promises.mkdir is surprising a bit :)
| // asset: keyof Settings; | ||
| url: string; | ||
| relativeDestination: string; // this |
There was a problem hiding this comment.
I don't understand the comments 😅
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { copySync } from 'fs-extra'; | ||
| import AxiosMockAdapter from 'axios-mock-adapter'; |
There was a problem hiding this comment.
I'd stick to use nock, it's library agnostic and a bit easier to use
kgajowy
force-pushed
the
feat/MARXAN-42-MARXAN-486-input-dat-2
branch
from
1d75fdc to
8e1814b
Compare
Substitute this line for a meaningful title for your changes
Overview
Please write a description. If the PR is hard to understand, provide a quick explanation of the code.
Designs
Link to the related design prototypes (if applicable)
Testing instructions
Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.
Feature relevant tickets
Link to the related task manager tickets
Checklist before submitting
develop.deploying to staging/production, please add brief testing instructions
to the deploy checklist (
docs/deployment-checklist.md)