Conversation
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…ild`, `nelm chart pack` Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com> # Conflicts: # cmd/nelm/groups.go # internal/chart/chart_render.go # internal/ts/bundle.go # internal/ts/files.go # internal/ts/init.go # pkg/common/common.go
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…g + atomic mv Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com> # Conflicts: # go.mod # go.sum
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
|
/review |
There was a problem hiding this comment.
Code Review Agent Run #5dfe02
Actionable Suggestions - 2
-
pkg/deno/downloader.go - 1
- Cleanup error logging missing · Line 42-44
-
pkg/deno/runtime.go - 1
- Stderr logged at wrong level · Line 167-171
Additional Suggestions - 2
-
pkg/deno/runtime.go - 1
-
Redundant comments on constants · Line 24-35These comments merely repeat what the constant names already convey, violating the rule against obvious/redundant comments.
Code suggestion
@@ -23,14 +23,8 @@ - const ( - // ChartTSBundleFile is the path to the bundle in a Helm chart. - ChartTSBundleFile = ChartTSSourceDir + "dist/bundle.js" - // ChartTSEntryPointJS is the JavaScript entry point path. - ChartTSEntryPointJS = "src/index.js" - // ChartTSEntryPointTS is the TypeScript entry point path. - ChartTSEntryPointTS = "src/index.ts" - // ChartTSSourceDir is the directory containing TypeScript sources in a Helm chart. - ChartTSSourceDir = "ts/" - // RenderInputFileName is the name of the input file with context for the Deno app. - RenderInputFileName = "input.yaml" - // RenderOutputFileName is the name of the output file with rendered manifests from the Deno app. - RenderOutputFileName = "output.yaml" - ) + const ( + ChartTSBundleFile = ChartTSSourceDir + "dist/bundle.js" + ChartTSEntryPointJS = "src/index.js" + ChartTSEntryPointTS = "src/index.ts" + ChartTSSourceDir = "ts/" + RenderInputFileName = "input.yaml" + RenderOutputFileName = "output.yaml" + )
-
-
go.sum - 1
-
New External Dependency Added · Line 114-114New external dependency github.com/dustin/go-humanize v1.0.1 has been added as a direct dependency in go.mod. According to repository guidelines (AGENTS.md), new external dependencies must be flagged to the user first. Please confirm if this addition is intentional and necessary.
-
Review Details
-
Files reviewed - 34 · Commit Range:
e6b3552..e17d511- cmd/nelm/chart.go
- cmd/nelm/chart_lint.go
- cmd/nelm/chart_pack.go
- cmd/nelm/chart_render.go
- cmd/nelm/chart_ts.go
- cmd/nelm/chart_ts_build.go
- cmd/nelm/groups.go
- cmd/nelm/release_install.go
- cmd/nelm/release_plan_install.go
- go.mod
- go.sum
- internal/chart/chart_render.go
- internal/ts/bundle.go
- internal/ts/bundle_ai_test.go
- internal/ts/bundle_test.go
- internal/ts/esbuild.go
- internal/ts/export_test.go
- internal/ts/files.go
- internal/ts/init.go
- internal/ts/init_templates.go
- internal/ts/init_test.go
- internal/ts/package.go
- internal/ts/package_test.go
- internal/ts/render.go
- internal/ts/render_ai_test.go
- internal/ts/render_test.go
- internal/ts/runtime.go
- pkg/action/chart_lint.go
- pkg/action/chart_render.go
- pkg/action/chart_ts_build.go
- pkg/action/release_install.go
- pkg/action/release_plan_install.go
- pkg/deno/downloader.go
- pkg/deno/runtime.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✔︎ Successful
- SNYK (Security Vulnerability) - ✔︎ Successful
- OWASP (Security Vulnerability) - ✔︎ Successful
- GOVULNCHECK (Security Vulnerability) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at ilya.lesikov@flant.com.
Documentation & Help
cmd/nelm/chart_lint.go
Outdated
| return fmt.Errorf("add flag: %w", err) | ||
| } | ||
|
|
||
| if err := cli.AddFlag(cmd, &cfg.RebuildTSBundle, "rebuild-ts", false, "Rebuild the typescript bundle even if it already exists.", cli.AddFlagOptions{ |
There was a problem hiding this comment.
Mention the limitations in the description. And maybe different flag name. How about --ignore-bundle-js? If our file is bundle.js (I don't remember) And make it cfg.IgnoreBundleJS. That we rebuild bundle under the hood if this option is true is implementation detail, the user shouldn't know it.
| return fmt.Errorf("add flag: %w", err) | ||
| } | ||
|
|
||
| if err := cli.AddFlag(cmd, &cfg.LogLevel, "log-level", string(log.InfoLevel), "Set log level. "+allowedLogLevelsHelp(), cli.AddFlagOptions{ |
There was a problem hiding this comment.
Check which flags are present in any command. Even if not always used. At least tempdir flag is missing.
cmd/nelm/chart_ts_init.go
Outdated
| "Initialize a new chart in the specified directory. If PATH is not specified, uses the current directory.", | ||
| 10, // priority for ordering in help | ||
| chartCmdGroup, | ||
| "Initialize a new TypeScript chart.", |
There was a problem hiding this comment.
It's not a TS chart initialization, but an initialization of the TS directory in the existing Helm chart
There was a problem hiding this comment.
Actually, we ensure chart structure too: we create Chart.yaml and values.yaml if it doesn't exist + we check .helmignore/.gitignore have necessary keys. Should we mention in cmd description that we ensure chart structure too?
There was a problem hiding this comment.
Chart.yaml shouldn't be created. Alright, let's say it's not "an initialization of the TS directory", but "an initialization of files needed to render manifests via TS" or something. Write whatever, as long as the user don't think we would generate the chart from scratch.
pkg/deno/runtime.go
Outdated
| if rt.rebuild && bundle != nil { | ||
| chart.RemoveRuntimeFile(ChartTSBundleFile) | ||
| } | ||
|
|
||
| chart.AddRuntimeFile(ChartTSBundleFile, bundleRes) |
There was a problem hiding this comment.
What will happen when rt.rebuild == false && bundle != nil? Do we get two bundles (old and new) in RuntimeFiles?
There was a problem hiding this comment.
Theoretically it shouldn't because of this condition
Line 59 in 30423bf
But i will simplify condition to
if bundle != nil {
pkg/deno/downloader.go
Outdated
| } | ||
|
|
||
| defer func() { | ||
| _ = denoFile.Close() |
pkg/deno/downloader.go
Outdated
| _ = fileReader.Close() | ||
| }() | ||
|
|
||
| limitReader := io.LimitReader(fileReader, 200*1024*1024) |
There was a problem hiding this comment.
I don't think we need this limiter here. Let's not overcomplicate. If its more than 200mb for whatever reason why should we care really
pkg/deno/runtime.go
Outdated
| const ( | ||
| // ChartTSBundleFile is the path to the bundle in a Helm chart. | ||
| ChartTSBundleFile = ChartTSSourceDir + "dist/bundle.js" | ||
| // ChartTSEntryPointJS is the JavaScript entry point path. | ||
| ChartTSEntryPointJS = "src/index.js" | ||
| // ChartTSEntryPointTS is the TypeScript entry point path. | ||
| ChartTSEntryPointTS = "src/index.ts" | ||
| // ChartTSSourceDir is the directory containing TypeScript sources in a Helm chart. | ||
| ChartTSSourceDir = "ts/" | ||
| // RenderInputFileName is the name of the input file with context for the Deno app. | ||
| RenderInputFileName = "input.yaml" | ||
| // RenderOutputFileName is the name of the output file with rendered manifests from the Deno app. | ||
| RenderOutputFileName = "output.yaml" | ||
| ) |
There was a problem hiding this comment.
I'd say let's move these constants into pkg/common, they might be useful in different contexts without any logic of this package.
pkg/deno/runtime.go
Outdated
| } | ||
|
|
||
| if bundle == nil || rt.rebuild { | ||
| if err := rt.ensureBinary(ctx); err != nil { |
There was a problem hiding this comment.
we are ensuring binary every time we recurse into a chart, but really only need to do it once. Maybe we better do downloading/caching outside of denoruntime? All we need is to go through Chart{}'s recursively and check if they have ts dir? And if there is one then download the thing to cache (unless custom path is specified)
Frankly, I don't think we need DenoRuntime class in here. Just make a few functions out of it, one will download binary, the other one will build bundle.js-es, the third one will run the bundles. There is no state to share so we don't need any classes for now
There was a problem hiding this comment.
I used DenoRuntime class only because of 3p-helm... It is simpler to pass it like class and run BundleChartsRecursive in chart pack, and everything will work under the hood (check ts dir, download deno, bundle and add files into RuntimeFiles)
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…ve/ensureDeno, disabled rule for decompress bomb Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com> # Conflicts: # cmd/nelm/chart_pack.go # go.mod # go.sum # internal/ts/bundle.go # internal/ts/bundle_ai_test.go # internal/ts/files.go # internal/ts/render_ai_test.go # internal/ts/render_test.go
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…for ts initialization Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
|
Hey I have a question What wrong with goja runtime and why required to use a full sized runtime like Deno? |
|
Hi @kitsunoff We have an idea to provide users with the flexibility to use standard ecosystem packages such as @kubernetes/client-node, cdk8s, and kubernetes-models. Trying to use goja, we found a critical limitation: it lacks support for Node.js native APIs. Since many vital Kubernetes-related npm packages rely on these built-ins under the hood, they simply will not work in a goja_nodejs environment. Choosing goja would result in a poor dev experience, because users will have a lot of restrictions (e.g., inability to use packages with Node dependencies or requiring external build steps for TypeScript). Deno, with its Node.js compatibility layer and native TypeScript execution, resolves these limitations. |
Summary by Bito
This pull request introduces Deno as a new runtime for TypeScript support in Helm charts, replacing the existing JavaScript runtime implementation based on goja and esbuild with a more secure and modern Deno-based approach that enables bundling and executing TypeScript code with controlled permissions.
Detailed Changes