-
Notifications
You must be signed in to change notification settings - Fork 0
Add --importmap option to use deno import maps #2
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
Conversation
src/deno/render-chart.ts
Outdated
| "--unstable", | ||
| "--allow-net", | ||
| "--allow-read", | ||
| "--allow-write", | ||
| "--allow-run", | ||
| "--allow-env", | ||
| "--quiet", | ||
| ...importmap, |
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.
Я не очень уверен какие разрешения нужны для процесса рендера чарта
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.
Наверное --allow-net не нужен в данном случае не нужно
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.
I think it's okay to have everything allowed for now, but in the future it will be required to specify manually which security boundaries are disabled
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.
Also, let's use English :) It's an open source project after all
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.
Okay, then I'll keep all the permissions for now.
|
Тесты не проходят, потому что вот тут helm-deno/e2e-tests/e2e.test.ts Line 91 in 2f853d6
import-chart.ts, который вызывается через Deno.run, то у него в выводе путь до временной папки с чартом
Можно отказаться от |
d7ad9af to
8cafa27
Compare
|
@verdel path to chart is replaced in output for better developer experience Lines 141 to 150 in 1e5d9fb
|
| "--deno-log-level", | ||
| "debug", | ||
| "--deno-keep-tmp-chart", | ||
| "--deno-importmap", |
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.
Let's separate words with dash for better readability
| "--deno-importmap", | |
| "--deno-import-map", |
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.
Yes, this name of the option is better
src/deno/render-chart.ts
Outdated
| const deno = path.join(pluginFolderPath, "bin/deno") | ||
| const importer = path.join(pluginFolderPath, "src/deno/import-chart.ts") | ||
|
|
||
| const isImportMap = await fs.exists(denoOptions.importMap) |
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.
| const isImportMap = await fs.exists(denoOptions.importMap) | |
| const hasImportMap = await fs.exists(denoOptions.importMap) |
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.
Done
src/deno/render-chart.ts
Outdated
| const importer = path.join(pluginFolderPath, "src/deno/import-chart.ts") | ||
|
|
||
| const isImportMap = await fs.exists(denoOptions.importMap) | ||
| const importmap = |
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.
| const importmap = | |
| const importMapArgs = |
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.
Done
No description provided.