Skip to content

Conversation

@verdel
Copy link
Contributor

@verdel verdel commented Feb 18, 2021

No description provided.

@verdel verdel requested a review from Nitive February 18, 2021 12:50
@verdel verdel requested a review from a team as a code owner February 18, 2021 12:50
Comment on lines 45 to 52
"--unstable",
"--allow-net",
"--allow-read",
"--allow-write",
"--allow-run",
"--allow-env",
"--quiet",
...importmap,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я не очень уверен какие разрешения нужны для процесса рендера чарта

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наверное --allow-net не нужен в данном случае не нужно

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

@verdel
Copy link
Contributor Author

verdel commented Feb 18, 2021

Тесты не проходят, потому что вот тут

path.join(chartPath, "deno-templates/index.ts")
ожидается путь до чарта, а так как ошибку возвращает теперь import-chart.ts, который вызывается через Deno.run, то у него в выводе путь до временной папки с чартом

Можно отказаться от chartPath

@verdel verdel force-pushed the feature-import-map branch from d7ad9af to 8cafa27 Compare February 18, 2021 18:09
@Nitive
Copy link
Contributor

Nitive commented Feb 19, 2021

@verdel path to chart is replaced in output for better developer experience

helm-deno/src/index.ts

Lines 141 to 150 in 1e5d9fb

const replaceChartPath = (str: string) => {
return isLocalChart
? str.replaceAll(workdir, chartLocation)
: str.replaceAll(`file://${workdir}`, "<chart-root>")
}
// Replace paths in stacktrace with readable value
if (err?.stack) {
err.stack = replaceChartPath(err.stack)
}

"--deno-log-level",
"debug",
"--deno-keep-tmp-chart",
"--deno-importmap",
Copy link
Contributor

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

Suggested change
"--deno-importmap",
"--deno-import-map",

Copy link
Contributor Author

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

const deno = path.join(pluginFolderPath, "bin/deno")
const importer = path.join(pluginFolderPath, "src/deno/import-chart.ts")

const isImportMap = await fs.exists(denoOptions.importMap)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isImportMap = await fs.exists(denoOptions.importMap)
const hasImportMap = await fs.exists(denoOptions.importMap)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const importer = path.join(pluginFolderPath, "src/deno/import-chart.ts")

const isImportMap = await fs.exists(denoOptions.importMap)
const importmap =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const importmap =
const importMapArgs =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Nitive Nitive enabled auto-merge (squash) February 19, 2021 11:08
@Nitive Nitive merged commit b3848e9 into main Feb 19, 2021
@Nitive Nitive deleted the feature-import-map branch February 19, 2021 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants