Conversation
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
utafrali
left a comment
There was a problem hiding this comment.
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.
| cmd.Dir = filepath.Join(chartPath, common.ChartTSSourceDir) | ||
|
|
||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { |
There was a problem hiding this comment.
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)
}| return fmt.Errorf("run deno install: %w", err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
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| 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{ |
There was a problem hiding this comment.
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.
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
No description provided.