Skip to content

feat: create deno.lock during chart ts init#594

Draft
dmmordvi wants to merge 2 commits intomainfrom
feat/init-deno-lock-file
Draft

feat: create deno.lock during chart ts init#594
dmmordvi wants to merge 2 commits intomainfrom
feat/init-deno-lock-file

Conversation

@dmmordvi
Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Copy link
Copy Markdown

@utafrali utafrali left a comment

Choose a reason for hiding this comment

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

The core logic is sound: deno install is correctly placed after the boilerplate files are written (so deno.json exists), and the new --deno-binary-path flag mirrors the existing one on chart ts build. The main issues are that successful install output is silently discarded and the error-output logging is gated behind an errors.As check that misses non-ExitError failures.

Comment thread pkg/ts/deno.go
cmd.Dir = filepath.Join(chartPath, common.ChartTSSourceDir)

output, err := cmd.CombinedOutput()
if err != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The errors.As guard means the captured output is only logged when the error is specifically an *exec.ExitError. For other failure modes (e.g., the binary is not executable, or the context is cancelled mid-run), any output already written to the combined pipe is silently dropped. Move the log outside the errors.As block so output is always surfaced on any error:

output, err := cmd.CombinedOutput()
if err != nil {
    if len(output) > 0 {
        log.Default.Error(ctx, "deno install output: %s", string(output))
    }
    return fmt.Errorf("run deno install: %w", err)
}

Comment thread pkg/ts/deno.go
return fmt.Errorf("run deno install: %w", err)
}

return nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On success, the combined output from deno install (package resolution, download progress, etc.) is captured by CombinedOutput() but then thrown away. Users running with --log-level=debug would benefit from seeing it. Add a debug log before the return nil:

if len(output) > 0 {
    log.Default.Debug(ctx, "deno install output: %s", string(output))
}
return nil

Comment thread cmd/nelm/chart_ts_init.go
return fmt.Errorf("add flag: %w", err)
}

if err := cli.AddFlag(cmd, &cfg.DenoBinaryPath, "deno-binary-path", "", "Path to the Deno binary to use instead of auto-downloading.", cli.AddFlagOptions{
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The --deno-binary-path flag is registered last here, after the log flags. In chart_ts_build.go the order is: color-mode, log-level, deno-binary-path, temp-dir. For consistency, move --deno-binary-path to register before --temp-dir and the log flags, matching the build command's ordering.

@dmmordvi dmmordvi marked this pull request as draft April 13, 2026 19:45
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
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.

2 participants