-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat(marxan-runner): use output directory provided via input.dat #331
Conversation
This pull request is being automatically deployed with Vercel (learn more). marxan – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan/132SodPcwXQtDkn6Logdi5DTE5Ey marxan-storybook – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/9TZeVHqTedazQjHQXTAirc6NDjkF |
442f6fc
to
432f31f
Compare
432f31f
to
2a492f9
Compare
Most likely it is about time to consider refactor of Workspace to contain all fetches/creations. |
return fullPath as WorkingDirectory; | ||
} | ||
|
||
#hasPoisonNullByte = (path: string): boolean => path.indexOf('\0') !== -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about escaping by ..
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async () => this.workingDirectoryService.cleanup(workingDirectory), | ||
async () => | ||
this.workingDirectoryService.createOutputDirectory(workingDirectory), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't just pass an object here?
also, why isn't port abstract?
async cleanup(directory: string): Promise<void> { | ||
// TODO check if starts with tempDirectory | ||
if (this.#hasPoisonNullByte(directory)) { | ||
throw new Error(`Hacking is not allowed.`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we validate it on input, before cleaning up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dyostiq would you mind to elaborate? It is stateless, so you can pass any directory and it validates it on every use 🤔
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
)