From a95a90337741e3b7b1265850457e3a15fb9f09b9 Mon Sep 17 00:00:00 2001 From: tytv2 Date: Tue, 30 Jun 2026 19:19:01 +0700 Subject: [PATCH 1/7] fix(configure): prevent nil-pointer panic on set/list for new profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LoadConfig returns (nil, err) for a profile not present in the credentials file. configure set and configure list discarded that error and dereferenced the nil *Config, crashing with SIGSEGV (e.g. `grn configure set region HCM-3 --profile prod-hcm-qc3`). Fall back to empty defaults — matching the fix already applied to interactive `configure` — so a value can be set or listed for a brand-new profile. Adds regression tests. Co-Authored-By: Claude Opus 4.8 --- .../bugfix-configure-dkiwga2j.json | 5 ++ go/cmd/configure/list.go | 7 ++- go/cmd/configure/set.go | 13 +++-- go/cmd/configure/set_test.go | 55 +++++++++++++++++++ 4 files changed, 74 insertions(+), 6 deletions(-) create mode 100644 .changes/next-release/bugfix-configure-dkiwga2j.json create mode 100644 go/cmd/configure/set_test.go diff --git a/.changes/next-release/bugfix-configure-dkiwga2j.json b/.changes/next-release/bugfix-configure-dkiwga2j.json new file mode 100644 index 0000000..6bcc5c7 --- /dev/null +++ b/.changes/next-release/bugfix-configure-dkiwga2j.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "configure", + "description": "Fix panic (nil pointer) when running 'grn configure set/list --profile ' for a profile that does not exist yet; fall back to empty defaults so the value can be set" +} diff --git a/go/cmd/configure/list.go b/go/cmd/configure/list.go index 81b2896..3cad7e2 100644 --- a/go/cmd/configure/list.go +++ b/go/cmd/configure/list.go @@ -31,7 +31,12 @@ func runList(cmd *cobra.Command, args []string) { profile = "default" } - cfg, _ := config.LoadConfig(profile) + // A non-existent profile yields (nil, err); fall back to empty defaults so + // list shows unset values instead of panicking. + cfg, err := config.LoadConfig(profile) + if err != nil || cfg == nil { + cfg = &config.Config{} + } configDir := config.DefaultConfigDir() credsFile := filepath.Join(configDir, "credentials") configFile := filepath.Join(configDir, "config") diff --git a/go/cmd/configure/set.go b/go/cmd/configure/set.go index 457dd25..2e06f40 100644 --- a/go/cmd/configure/set.go +++ b/go/cmd/configure/set.go @@ -28,33 +28,36 @@ func runSet(cmd *cobra.Command, args []string) { writer := config.NewConfigFileWriter() + // Load existing config so unrelated fields are preserved on write. For a + // brand-new profile LoadConfig returns (nil, err); fall back to empty + // defaults so the value can still be set instead of panicking. + cfg, err := config.LoadConfig(profile) + if err != nil || cfg == nil { + cfg = &config.Config{} + } + switch key { case "client_id": - cfg, _ := config.LoadConfig(profile) if err := writer.WriteCredentials(profile, value, cfg.ClientSecret); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } case "client_secret": - cfg, _ := config.LoadConfig(profile) if err := writer.WriteCredentials(profile, cfg.ClientID, value); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } case "region": - cfg, _ := config.LoadConfig(profile) if err := writer.WriteConfig(profile, value, cfg.Output, cfg.ProjectID); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } case "output": - cfg, _ := config.LoadConfig(profile) if err := writer.WriteConfig(profile, cfg.Region, value, cfg.ProjectID); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } case "project_id": - cfg, _ := config.LoadConfig(profile) if err := writer.WriteConfig(profile, cfg.Region, cfg.Output, value); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) diff --git a/go/cmd/configure/set_test.go b/go/cmd/configure/set_test.go new file mode 100644 index 0000000..fc3c4cf --- /dev/null +++ b/go/cmd/configure/set_test.go @@ -0,0 +1,55 @@ +package configure + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// newConfigureTestCmd wires a root command with the persistent `profile` flag +// (registered on rootCmd in production) so the configure subcommands resolve it +// exactly as they do at runtime. +func newConfigureTestCmd() *cobra.Command { + root := &cobra.Command{Use: "grn"} + root.PersistentFlags().String("profile", "", "") + root.AddCommand(ConfigureCmd) + return root +} + +// Regression: `configure set --profile ` for a profile that +// does not exist in the credentials file must not panic. LoadConfig returns +// (nil, err) for an unknown profile; set previously dereferenced the nil *Config. +func TestSetRegionOnNonExistentProfile(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + root := newConfigureTestCmd() + root.SetArgs([]string{"configure", "set", "region", "HCM-3", "--profile", "prod-hcm-qc3"}) + if err := root.Execute(); err != nil { + t.Fatalf("set region on new profile failed: %v", err) + } + + data, err := os.ReadFile(filepath.Join(home, ".greenode", "config")) + if err != nil { + t.Fatalf("config file not written: %v", err) + } + content := string(data) + if !strings.Contains(content, "prod-hcm-qc3") || !strings.Contains(content, "HCM-3") { + t.Errorf("config missing profile/region; got:\n%s", content) + } +} + +// Regression: `configure list --profile ` for an unknown profile must not +// panic and should render unset values. +func TestListOnNonExistentProfile(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + root := newConfigureTestCmd() + root.SetArgs([]string{"configure", "list", "--profile", "ghost"}) + if err := root.Execute(); err != nil { + t.Fatalf("list on new profile failed: %v", err) + } +} From 52fe90033af8024f43e08d3ef51e18703847e59b Mon Sep 17 00:00:00 2001 From: tytv2 Date: Tue, 30 Jun 2026 19:29:07 +0700 Subject: [PATCH 2/7] fix(configure): read credentials and config files independently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A profile present only in the config file — e.g. just after `configure set region --profile X` on a profile that has no credentials yet — previously failed: LoadConfig returned early with an error when the credentials section was missing, so region/output/project_id (which live in the config file) were never read, and set/list saw a nil *Config. LoadConfig now treats the two files independently: a profile "exists" if it has a section in either file (or credentials via env vars). It only returns a "profile does not exist" error when config files are present but the profile is in neither — matching `aws configure`. Adds LoadConfig tests for the config-only, neither-file, creds-only, and no-files cases. Co-Authored-By: Claude Opus 4.8 --- .../bugfix-configure-dkiwga2j.json | 2 +- go/internal/config/config.go | 53 ++++++--- go/internal/config/loadconfig_test.go | 106 ++++++++++++++++++ 3 files changed, 145 insertions(+), 16 deletions(-) create mode 100644 go/internal/config/loadconfig_test.go diff --git a/.changes/next-release/bugfix-configure-dkiwga2j.json b/.changes/next-release/bugfix-configure-dkiwga2j.json index 6bcc5c7..90519d6 100644 --- a/.changes/next-release/bugfix-configure-dkiwga2j.json +++ b/.changes/next-release/bugfix-configure-dkiwga2j.json @@ -1,5 +1,5 @@ { "type": "bugfix", "category": "configure", - "description": "Fix panic (nil pointer) when running 'grn configure set/list --profile ' for a profile that does not exist yet; fall back to empty defaults so the value can be set" + "description": "Fix panic (nil pointer) when running 'grn configure set/list --profile ' for a profile that does not exist yet. LoadConfig now reads the credentials and config files independently, so a profile created via 'configure set' (config file only) loads its region/output/project_id correctly, and 'configure get' on a truly missing profile reports a clear 'profile does not exist' error instead of crashing" } diff --git a/go/internal/config/config.go b/go/internal/config/config.go index 2ea9a97..622b8a7 100644 --- a/go/internal/config/config.go +++ b/go/internal/config/config.go @@ -54,6 +54,13 @@ func LoadConfig(profile string) (*Config, error) { Regions: REGIONS, } + // A profile "exists" if it has a section in the credentials file, the config + // file, or credentials are supplied via env vars. credentials and config are + // read independently so a profile created by `configure set region` (config + // file only, no credentials yet) still loads instead of erroring. + foundProfile := false + anyFileExists := false + // Load credentials — env vars override file if v := os.Getenv("GRN_ACCESS_KEY_ID"); v != "" { cfg.ClientID = v @@ -61,30 +68,36 @@ func LoadConfig(profile string) (*Config, error) { if v := os.Getenv("GRN_SECRET_ACCESS_KEY"); v != "" { cfg.ClientSecret = v } + if cfg.ClientID != "" && cfg.ClientSecret != "" { + foundProfile = true + } if cfg.ClientID == "" || cfg.ClientSecret == "" { credsFile := filepath.Join(configDir, "credentials") if _, err := os.Stat(credsFile); err == nil { + anyFileExists = true iniCreds, err := ini.Load(credsFile) if err != nil { return nil, fmt.Errorf("failed to parse credentials file: %w", err) } - section, err := iniCreds.GetSection(profile) - if err != nil { - return nil, fmt.Errorf("profile '%s' does not exist in %s", profile, credsFile) - } - if cfg.ClientID == "" { - cfg.ClientID = section.Key("client_id").String() - } - if cfg.ClientSecret == "" { - cfg.ClientSecret = section.Key("client_secret").String() + // Missing section is not fatal — the profile may live in the config + // file only. Just skip credentials for this profile. + if section, err := iniCreds.GetSection(profile); err == nil { + foundProfile = true + if cfg.ClientID == "" { + cfg.ClientID = section.Key("client_id").String() + } + if cfg.ClientSecret == "" { + cfg.ClientSecret = section.Key("client_secret").String() + } } } } - // Load config file + // Load config file (independent of credentials) configFile := filepath.Join(configDir, "config") if _, err := os.Stat(configFile); err == nil { + anyFileExists = true iniCfg, err := ini.Load(configFile) if err != nil { return nil, fmt.Errorf("failed to parse config file: %w", err) @@ -95,12 +108,15 @@ func LoadConfig(profile string) (*Config, error) { sectionName = "profile " + profile } - section, err := iniCfg.GetSection(sectionName) - if err != nil && profile == "default" { - // Try DEFAULT section for default profile - section = iniCfg.Section("") + section, serr := iniCfg.GetSection(sectionName) + if serr != nil && profile == "default" { + // Try the root DEFAULT section for the default profile. + if root := iniCfg.Section(ini.DefaultSection); len(root.Keys()) > 0 { + section, serr = root, nil + } } - if section != nil { + if serr == nil && section != nil { + foundProfile = true if v := section.Key("region").String(); v != "" { cfg.Region = v } @@ -113,6 +129,13 @@ func LoadConfig(profile string) (*Config, error) { } } + // Config files exist but the profile is in neither — report it like + // `aws configure` does ("profile could not be found") so reads (get/list) + // and API clients fail clearly instead of acting on empty config. + if anyFileExists && !foundProfile { + return nil, fmt.Errorf("profile '%s' does not exist (run 'grn configure --profile %s' to create it)", profile, profile) + } + // Env var overrides for region if v := os.Getenv("GRN_DEFAULT_REGION"); v != "" { cfg.Region = v diff --git a/go/internal/config/loadconfig_test.go b/go/internal/config/loadconfig_test.go new file mode 100644 index 0000000..6c62e59 --- /dev/null +++ b/go/internal/config/loadconfig_test.go @@ -0,0 +1,106 @@ +package config + +import ( + "os" + "path/filepath" + "testing" +) + +// isolateConfigEnv points HOME at a temp dir and clears GRN_* env vars so +// LoadConfig reads only the files we create under /.greenode. +func isolateConfigEnv(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + for _, k := range []string{ + "GRN_PROFILE", "GRN_ACCESS_KEY_ID", "GRN_SECRET_ACCESS_KEY", + "GRN_DEFAULT_REGION", "GRN_DEFAULT_PROJECT_ID", + } { + t.Setenv(k, "") + } + dir := filepath.Join(home, ".greenode") + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatalf("mkdir: %v", err) + } + return dir +} + +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0600); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +// Regression: a profile present only in the config file (e.g. just after +// `configure set region`) must load its region/output without erroring, even +// when a credentials file exists but has no section for that profile. +func TestLoadConfigProfileInConfigOnly(t *testing.T) { + dir := isolateConfigEnv(t) + writeFile(t, filepath.Join(dir, "credentials"), + "[default]\nclient_id = AKIA-default\nclient_secret = secret-default\n") + writeFile(t, filepath.Join(dir, "config"), + "[profile ghost]\nregion = HCM-3\noutput = table\n") + + cfg, err := LoadConfig("ghost") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg == nil { + t.Fatal("cfg is nil") + } + if cfg.Region != "HCM-3" { + t.Errorf("region = %q, want HCM-3", cfg.Region) + } + if cfg.Output != "table" { + t.Errorf("output = %q, want table", cfg.Output) + } + if cfg.ClientID != "" || cfg.ClientSecret != "" { + t.Errorf("expected empty credentials, got id=%q secret=%q", cfg.ClientID, cfg.ClientSecret) + } +} + +// A profile in neither file, when config files exist, errors like +// `aws configure` ("could not be found"). +func TestLoadConfigProfileInNeitherFile(t *testing.T) { + dir := isolateConfigEnv(t) + writeFile(t, filepath.Join(dir, "credentials"), + "[default]\nclient_id = AKIA-default\nclient_secret = secret-default\n") + + if _, err := LoadConfig("ghost"); err == nil { + t.Error("expected error for profile present in neither file") + } +} + +// A profile present only in credentials loads its credentials with no error and +// applies the default output. +func TestLoadConfigProfileInCredsOnly(t *testing.T) { + dir := isolateConfigEnv(t) + writeFile(t, filepath.Join(dir, "credentials"), + "[work]\nclient_id = AKIA-work\nclient_secret = secret-work\n") + + cfg, err := LoadConfig("work") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.ClientID != "AKIA-work" { + t.Errorf("client_id = %q, want AKIA-work", cfg.ClientID) + } + if cfg.Output != "json" { + t.Errorf("output = %q, want json (default)", cfg.Output) + } +} + +// Fresh machine with no config files: not an error (preserves first-run UX); +// returns a populated default cfg. +func TestLoadConfigNoFiles(t *testing.T) { + isolateConfigEnv(t) + + cfg, err := LoadConfig("default") + if err != nil { + t.Fatalf("unexpected error on fresh machine: %v", err) + } + if cfg == nil || cfg.Output != "json" { + t.Errorf("expected non-nil cfg with default output, got %#v", cfg) + } +} From 207b4738dc5704e4d089f812ef6c4907b7a847cb Mon Sep 17 00:00:00 2001 From: tytv2 Date: Tue, 30 Jun 2026 19:31:56 +0700 Subject: [PATCH 3/7] fix(configure): list reports missing profile like aws configure configure list now exits non-zero with "profile does not exist" when the profile is absent from existing config files, instead of printing empty values. A fresh machine with no config files still lists unset defaults, matching `aws configure list`. Co-Authored-By: Claude Opus 4.8 --- .changes/next-release/bugfix-configure-dkiwga2j.json | 2 +- go/cmd/configure/list.go | 11 ++++++++--- go/cmd/configure/set_test.go | 10 ++++++---- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/.changes/next-release/bugfix-configure-dkiwga2j.json b/.changes/next-release/bugfix-configure-dkiwga2j.json index 90519d6..319aefe 100644 --- a/.changes/next-release/bugfix-configure-dkiwga2j.json +++ b/.changes/next-release/bugfix-configure-dkiwga2j.json @@ -1,5 +1,5 @@ { "type": "bugfix", "category": "configure", - "description": "Fix panic (nil pointer) when running 'grn configure set/list --profile ' for a profile that does not exist yet. LoadConfig now reads the credentials and config files independently, so a profile created via 'configure set' (config file only) loads its region/output/project_id correctly, and 'configure get' on a truly missing profile reports a clear 'profile does not exist' error instead of crashing" + "description": "Fix panic (nil pointer) when running 'grn configure set/list --profile ' for a profile that does not exist yet. LoadConfig now reads the credentials and config files independently, so a profile created via 'configure set' (config file only) loads its region/output/project_id correctly, and 'configure get'/'configure list' on a truly missing profile report a clear 'profile does not exist' error (exit 1, like 'aws configure') instead of crashing" } diff --git a/go/cmd/configure/list.go b/go/cmd/configure/list.go index 3cad7e2..2ebcfea 100644 --- a/go/cmd/configure/list.go +++ b/go/cmd/configure/list.go @@ -31,10 +31,15 @@ func runList(cmd *cobra.Command, args []string) { profile = "default" } - // A non-existent profile yields (nil, err); fall back to empty defaults so - // list shows unset values instead of panicking. + // Report a missing profile like `aws configure list` does, rather than + // printing empty values. LoadConfig only errors when config files exist but + // the profile is in neither — a fresh machine still lists unset defaults. cfg, err := config.LoadConfig(profile) - if err != nil || cfg == nil { + if err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + if cfg == nil { cfg = &config.Config{} } configDir := config.DefaultConfigDir() diff --git a/go/cmd/configure/set_test.go b/go/cmd/configure/set_test.go index fc3c4cf..b522ee4 100644 --- a/go/cmd/configure/set_test.go +++ b/go/cmd/configure/set_test.go @@ -42,14 +42,16 @@ func TestSetRegionOnNonExistentProfile(t *testing.T) { } } -// Regression: `configure list --profile ` for an unknown profile must not -// panic and should render unset values. -func TestListOnNonExistentProfile(t *testing.T) { +// On a fresh machine (no config files at all) `configure list` must not panic +// and renders unset defaults — matching `aws configure list`. (A profile that +// is missing while config files DO exist exits non-zero via os.Exit, which is +// covered by the binary-level checks rather than here.) +func TestListOnFreshMachineNoFiles(t *testing.T) { t.Setenv("HOME", t.TempDir()) root := newConfigureTestCmd() root.SetArgs([]string{"configure", "list", "--profile", "ghost"}) if err := root.Execute(); err != nil { - t.Fatalf("list on new profile failed: %v", err) + t.Fatalf("list on fresh machine failed: %v", err) } } From a4fa38752bd7469d7d029bb56cda59b69e3ec9fb Mon Sep 17 00:00:00 2001 From: tytv2 Date: Tue, 30 Jun 2026 19:38:21 +0700 Subject: [PATCH 4/7] fix(output): reject invalid --output instead of silent JSON fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unrecognized --output value (e.g. the typo 'tabel') previously fell through formatter's default case to JSON and exited 0, so users got a different format than intended with no warning. Add a root PersistentPreRunE that validates the --output flag up front and fails fast with a non-zero exit, a "maybe you meant" suggestion (Levenshtein-nearest valid format), and the list of valid formats — matching `aws`/`gcloud`. Only validates the flag when explicitly set. Co-Authored-By: Claude Opus 4.8 --- .../next-release/bugfix-output-m4udwit8.json | 5 ++ go/cmd/root.go | 5 ++ go/cmd/validate.go | 79 +++++++++++++++++++ go/cmd/validate_test.go | 61 ++++++++++++++ 4 files changed, 150 insertions(+) create mode 100644 .changes/next-release/bugfix-output-m4udwit8.json create mode 100644 go/cmd/validate.go create mode 100644 go/cmd/validate_test.go diff --git a/.changes/next-release/bugfix-output-m4udwit8.json b/.changes/next-release/bugfix-output-m4udwit8.json new file mode 100644 index 0000000..34f7529 --- /dev/null +++ b/.changes/next-release/bugfix-output-m4udwit8.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "output", + "description": "Reject an invalid --output value (e.g. a typo like 'tabel') with a clear error, a 'maybe you meant' suggestion, and a non-zero exit, instead of silently falling back to JSON" +} diff --git a/go/cmd/root.go b/go/cmd/root.go index 2c417b3..89991c9 100644 --- a/go/cmd/root.go +++ b/go/cmd/root.go @@ -39,6 +39,11 @@ To get started, run: For help on any command: grn --help`, + // Validate global flags up front so an invalid --output fails fast with a + // suggestion, rather than silently falling back to JSON. + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return validateGlobalFlags(cmd) + }, Run: func(cmd *cobra.Command, args []string) { cmd.Help() }, diff --git a/go/cmd/validate.go b/go/cmd/validate.go new file mode 100644 index 0000000..abc3695 --- /dev/null +++ b/go/cmd/validate.go @@ -0,0 +1,79 @@ +package cmd + +import ( + "fmt" + "slices" + "strings" + + "github.com/spf13/cobra" +) + +var validOutputFormats = []string{"json", "text", "table"} + +// validateGlobalFlags rejects invalid values for global flags before any work +// runs, mirroring `aws`/`gcloud` which fail fast on an unknown --output instead +// of silently falling back. Only validates flags the user explicitly set. +func validateGlobalFlags(cmd *cobra.Command) error { + if cmd.Flags().Changed("output") { + v, _ := cmd.Flags().GetString("output") + if err := validateOutputFormat(v); err != nil { + return err + } + } + return nil +} + +// validateOutputFormat returns an error when value is a non-empty, +// unrecognized output format. The message echoes the bad value, suggests the +// closest valid format when one is near, and always lists the valid formats. +func validateOutputFormat(value string) error { + if value == "" { + return nil + } + if slices.Contains(validOutputFormats, value) { + return nil + } + msg := fmt.Sprintf("invalid output format: '%s'", value) + if s := suggestClosest(value, validOutputFormats); s != "" { + msg += fmt.Sprintf(", maybe you meant '%s'", s) + } + msg += fmt.Sprintf(" (valid formats: %s)", strings.Join(validOutputFormats, ", ")) + return fmt.Errorf("%s", msg) +} + +// suggestClosest returns the option nearest to value by Levenshtein distance, +// or "" when the nearest is too far to be a likely typo (distance > 2). +func suggestClosest(value string, options []string) string { + best := "" + bestDist := 1 << 30 + for _, opt := range options { + if d := levenshtein(value, opt); d < bestDist { + bestDist, best = d, opt + } + } + if bestDist > 2 { + return "" + } + return best +} + +func levenshtein(a, b string) int { + ra, rb := []rune(a), []rune(b) + prev := make([]int, len(rb)+1) + curr := make([]int, len(rb)+1) + for j := range prev { + prev[j] = j + } + for i := 1; i <= len(ra); i++ { + curr[0] = i + for j := 1; j <= len(rb); j++ { + cost := 1 + if ra[i-1] == rb[j-1] { + cost = 0 + } + curr[j] = min(prev[j]+1, curr[j-1]+1, prev[j-1]+cost) + } + prev, curr = curr, prev + } + return prev[len(rb)] +} diff --git a/go/cmd/validate_test.go b/go/cmd/validate_test.go new file mode 100644 index 0000000..0bb9d13 --- /dev/null +++ b/go/cmd/validate_test.go @@ -0,0 +1,61 @@ +package cmd + +import ( + "strings" + "testing" +) + +func TestValidateOutputFormatValid(t *testing.T) { + for _, v := range []string{"json", "text", "table"} { + if err := validateOutputFormat(v); err != nil { + t.Errorf("%q should be valid, got %v", v, err) + } + } +} + +func TestValidateOutputFormatEmptyIsValid(t *testing.T) { + // Empty means the flag was not provided; default resolution handles it. + if err := validateOutputFormat(""); err != nil { + t.Errorf("empty should be valid, got %v", err) + } +} + +func TestValidateOutputFormatTypoSuggests(t *testing.T) { + err := validateOutputFormat("tabel") + if err == nil { + t.Fatal("typo 'tabel' should be rejected") + } + msg := err.Error() + if !strings.Contains(msg, "tabel") { + t.Errorf("error should echo the bad value, got %q", msg) + } + if !strings.Contains(msg, "table") { + t.Errorf("error should suggest 'table' (did you mean), got %q", msg) + } +} + +func TestValidateOutputFormatUnknownNoSuggestion(t *testing.T) { + err := validateOutputFormat("xml") + if err == nil { + t.Fatal("'xml' should be rejected") + } + // Far from any valid value: no bogus suggestion, but valid list is shown. + if !strings.Contains(err.Error(), "json") { + t.Errorf("error should list valid formats, got %q", err.Error()) + } +} + +func TestSuggestClosest(t *testing.T) { + opts := []string{"json", "text", "table"} + cases := map[string]string{ + "tabel": "table", + "jsno": "json", + "txt": "text", + "xml": "", // too far -> no suggestion + } + for in, want := range cases { + if got := suggestClosest(in, opts); got != want { + t.Errorf("suggestClosest(%q) = %q, want %q", in, got, want) + } + } +} From 997dd53a45513a3a6f8f08dd95fe0eb4c4c975d3 Mon Sep 17 00:00:00 2001 From: tytv2 Date: Tue, 30 Jun 2026 19:42:24 +0700 Subject: [PATCH 5/7] fix(output): validate --color; print concise errors without usage dump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend up-front global-flag validation to --color (on/off/auto) using the same "maybe you meant" suggestion as --output. Set SilenceErrors/SilenceUsage on the root command and print a single "Error: ..." line in Execute, so failures no longer emit cobra's duplicated error plus a full usage block — matching aws/gcloud. Co-Authored-By: Claude Opus 4.8 --- .../next-release/bugfix-output-m4udwit8.json | 2 +- .../enhancement-core-i13acudc.json | 5 +++ go/cmd/root.go | 6 ++- go/cmd/validate.go | 45 ++++++++++++++----- go/cmd/validate_test.go | 15 +++++++ 5 files changed, 59 insertions(+), 14 deletions(-) create mode 100644 .changes/next-release/enhancement-core-i13acudc.json diff --git a/.changes/next-release/bugfix-output-m4udwit8.json b/.changes/next-release/bugfix-output-m4udwit8.json index 34f7529..becc99c 100644 --- a/.changes/next-release/bugfix-output-m4udwit8.json +++ b/.changes/next-release/bugfix-output-m4udwit8.json @@ -1,5 +1,5 @@ { "type": "bugfix", "category": "output", - "description": "Reject an invalid --output value (e.g. a typo like 'tabel') with a clear error, a 'maybe you meant' suggestion, and a non-zero exit, instead of silently falling back to JSON" + "description": "Reject an invalid --output or --color value (e.g. a typo like 'tabel') with a clear error, a 'maybe you meant' suggestion, and a non-zero exit, instead of silently falling back" } diff --git a/.changes/next-release/enhancement-core-i13acudc.json b/.changes/next-release/enhancement-core-i13acudc.json new file mode 100644 index 0000000..fa991e0 --- /dev/null +++ b/.changes/next-release/enhancement-core-i13acudc.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "core", + "description": "On error, print a single concise 'Error: ...' line instead of cobra's duplicated error plus a full usage dump (use 'grn --help' for usage)" +} diff --git a/go/cmd/root.go b/go/cmd/root.go index 89991c9..982cade 100644 --- a/go/cmd/root.go +++ b/go/cmd/root.go @@ -31,6 +31,10 @@ var rootCmd = &cobra.Command{ Use: "grn", Short: "GreenNode CLI - unified command-line tool for GreenNode (VNG Cloud) services", Version: fmt.Sprintf("%s Go/%s %s/%s", cliVersion, runtime.Version()[2:], runtime.GOOS, runtime.GOARCH), + // Print a single clean "Error: ..." line on failure (done in Execute) rather + // than cobra's error plus a full usage dump. + SilenceErrors: true, + SilenceUsage: true, Long: `GreenNode CLI (grn) is a unified command-line tool for managing GreenNode (VNG Cloud) services including VKS (VNG Kubernetes Service). @@ -77,7 +81,7 @@ func init() { // Execute runs the root command. func Execute() { if err := rootCmd.Execute(); err != nil { - fmt.Fprintln(os.Stderr, err) + fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } } diff --git a/go/cmd/validate.go b/go/cmd/validate.go index abc3695..47a9bef 100644 --- a/go/cmd/validate.go +++ b/go/cmd/validate.go @@ -8,15 +8,29 @@ import ( "github.com/spf13/cobra" ) -var validOutputFormats = []string{"json", "text", "table"} +var ( + validOutputFormats = []string{"json", "text", "table"} + validColorModes = []string{"on", "off", "auto"} +) // validateGlobalFlags rejects invalid values for global flags before any work -// runs, mirroring `aws`/`gcloud` which fail fast on an unknown --output instead -// of silently falling back. Only validates flags the user explicitly set. +// runs, mirroring `aws`/`gcloud` which fail fast on an unknown value instead of +// silently falling back. Only validates flags the user explicitly set. func validateGlobalFlags(cmd *cobra.Command) error { - if cmd.Flags().Changed("output") { - v, _ := cmd.Flags().GetString("output") - if err := validateOutputFormat(v); err != nil { + checks := []struct { + flag string + label string + valid []string + }{ + {"output", "output format", validOutputFormats}, + {"color", "color mode", validColorModes}, + } + for _, c := range checks { + if !cmd.Flags().Changed(c.flag) { + continue + } + v, _ := cmd.Flags().GetString(c.flag) + if err := validateChoice(c.label, v, c.valid); err != nil { return err } } @@ -24,20 +38,27 @@ func validateGlobalFlags(cmd *cobra.Command) error { } // validateOutputFormat returns an error when value is a non-empty, -// unrecognized output format. The message echoes the bad value, suggests the -// closest valid format when one is near, and always lists the valid formats. +// unrecognized output format. func validateOutputFormat(value string) error { + return validateChoice("output format", value, validOutputFormats) +} + +// validateChoice returns an error when value is non-empty and not one of valid. +// The message echoes the bad value, suggests the closest valid option when one +// is near, and always lists the valid options. label names the thing being +// validated (e.g. "output format"). +func validateChoice(label, value string, valid []string) error { if value == "" { return nil } - if slices.Contains(validOutputFormats, value) { + if slices.Contains(valid, value) { return nil } - msg := fmt.Sprintf("invalid output format: '%s'", value) - if s := suggestClosest(value, validOutputFormats); s != "" { + msg := fmt.Sprintf("invalid %s: '%s'", label, value) + if s := suggestClosest(value, valid); s != "" { msg += fmt.Sprintf(", maybe you meant '%s'", s) } - msg += fmt.Sprintf(" (valid formats: %s)", strings.Join(validOutputFormats, ", ")) + msg += fmt.Sprintf(" (valid values: %s)", strings.Join(valid, ", ")) return fmt.Errorf("%s", msg) } diff --git a/go/cmd/validate_test.go b/go/cmd/validate_test.go index 0bb9d13..ff570e4 100644 --- a/go/cmd/validate_test.go +++ b/go/cmd/validate_test.go @@ -45,6 +45,21 @@ func TestValidateOutputFormatUnknownNoSuggestion(t *testing.T) { } } +func TestValidateChoiceColor(t *testing.T) { + for _, v := range []string{"on", "off", "auto", ""} { + if err := validateChoice("color mode", v, validColorModes); err != nil { + t.Errorf("%q should be valid, got %v", v, err) + } + } + err := validateChoice("color mode", "atuo", validColorModes) + if err == nil { + t.Fatal("typo 'atuo' should be rejected") + } + if !strings.Contains(err.Error(), "auto") { + t.Errorf("error should suggest 'auto', got %q", err.Error()) + } +} + func TestSuggestClosest(t *testing.T) { opts := []string{"json", "text", "table"} cases := map[string]string{ From d6dc7ee73f3d005f1e672cd3158764697e4ce19c Mon Sep 17 00:00:00 2001 From: tytv2 Date: Tue, 30 Jun 2026 19:50:35 +0700 Subject: [PATCH 6/7] fix(core): honor --cli-connect-timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flag was registered but never read, and the HTTP transport used a bare http.Transport with no dial timeout — so the connect/TLS phase was unbounded and an unreachable endpoint hung for ~127s (4 attempts x the 30s read timeout) regardless of --cli-connect-timeout. NewGreenodeClient now takes connectTimeout and wires it into the transport's net.Dialer (DialContext) and TLSHandshakeTimeout; both the VKS and vServer client builders read --cli-connect-timeout and pass it through. A connect to an unreachable host with --cli-connect-timeout 1 now fails per-attempt in ~1s (dial i/o timeout) instead of 30s. Co-Authored-By: Claude Opus 4.8 --- .../next-release/bugfix-core-qyk3eljj.json | 5 ++++ go/internal/cli/client.go | 6 +++-- go/internal/client/client.go | 21 ++++++++++++----- go/internal/client/client_test.go | 23 +++++++++++++++++-- go/internal/vserverclient/client.go | 6 +++-- 5 files changed, 49 insertions(+), 12 deletions(-) create mode 100644 .changes/next-release/bugfix-core-qyk3eljj.json diff --git a/.changes/next-release/bugfix-core-qyk3eljj.json b/.changes/next-release/bugfix-core-qyk3eljj.json new file mode 100644 index 0000000..98f6893 --- /dev/null +++ b/.changes/next-release/bugfix-core-qyk3eljj.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "core", + "description": "Honor --cli-connect-timeout: the TCP connect and TLS handshake are now bounded by the flag (previously it was accepted but ignored, so a slow/unreachable endpoint hung for ~127s regardless). Wired via the HTTP transport's dialer in both VKS and vServer clients" +} diff --git a/go/internal/cli/client.go b/go/internal/cli/client.go index 75b4c48..b7be48d 100644 --- a/go/internal/cli/client.go +++ b/go/internal/cli/client.go @@ -22,6 +22,7 @@ func NewClient(cmd *cobra.Command, serviceName string) (*client.GreenodeClient, endpointURL, _ := cmd.Flags().GetString("endpoint-url") noVerifySSL, _ := cmd.Flags().GetBool("no-verify-ssl") debug, _ := cmd.Flags().GetBool("debug") + connectTimeout, _ := cmd.Flags().GetInt("cli-connect-timeout") readTimeout, _ := cmd.Flags().GetInt("cli-read-timeout") cfg, err := config.LoadConfig(profile) @@ -52,7 +53,8 @@ func NewClient(cmd *cobra.Command, serviceName string) (*client.GreenodeClient, } tokenManager := auth.NewTokenManager(cfg.ClientID, cfg.ClientSecret) - timeout := time.Duration(readTimeout) * time.Second + connect := time.Duration(connectTimeout) * time.Second + read := time.Duration(readTimeout) * time.Second - return client.NewGreenodeClient(baseURL, tokenManager, timeout, !noVerifySSL, debug), nil + return client.NewGreenodeClient(baseURL, tokenManager, connect, read, !noVerifySSL, debug), nil } diff --git a/go/internal/client/client.go b/go/internal/client/client.go index 2eac694..6c209fb 100644 --- a/go/internal/client/client.go +++ b/go/internal/client/client.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "net/url" "os" @@ -46,13 +47,21 @@ type GreenodeClient struct { debug bool } -// NewGreenodeClient creates a new API client. -func NewGreenodeClient(baseURL string, tokenManager *auth.TokenManager, timeout time.Duration, verifySSL bool, debug bool) *GreenodeClient { - if timeout == 0 { - timeout = defaultTimeout +// NewGreenodeClient creates a new API client. connectTimeout bounds the TCP +// connect and TLS handshake (the --cli-connect-timeout flag); readTimeout bounds +// the overall request (the --cli-read-timeout flag). A zero readTimeout falls +// back to the default; a zero connectTimeout means no explicit connect bound. +func NewGreenodeClient(baseURL string, tokenManager *auth.TokenManager, connectTimeout, readTimeout time.Duration, verifySSL bool, debug bool) *GreenodeClient { + if readTimeout == 0 { + readTimeout = defaultTimeout } - transport := &http.Transport{} + transport := &http.Transport{ + DialContext: (&net.Dialer{Timeout: connectTimeout}).DialContext, + } + if connectTimeout > 0 { + transport.TLSHandshakeTimeout = connectTimeout + } if !verifySSL { transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec } @@ -61,7 +70,7 @@ func NewGreenodeClient(baseURL string, tokenManager *auth.TokenManager, timeout baseURL: baseURL, tokenManager: tokenManager, httpClient: &http.Client{ - Timeout: timeout, + Timeout: readTimeout, Transport: transport, }, debug: debug, diff --git a/go/internal/client/client_test.go b/go/internal/client/client_test.go index fee2602..46ba6af 100644 --- a/go/internal/client/client_test.go +++ b/go/internal/client/client_test.go @@ -12,6 +12,25 @@ import ( "github.com/vngcloud/greennode-cli/internal/auth" ) +func TestNewGreenodeClientWiresConnectTimeout(t *testing.T) { + tm := auth.NewTokenManager("id", "secret") + c := NewGreenodeClient("https://example.invalid", tm, 2*time.Second, 7*time.Second, true, false) + + if c.httpClient.Timeout != 7*time.Second { + t.Errorf("read timeout: httpClient.Timeout = %v, want 7s", c.httpClient.Timeout) + } + tr, ok := c.httpClient.Transport.(*http.Transport) + if !ok { + t.Fatalf("transport is %T, want *http.Transport", c.httpClient.Transport) + } + if tr.DialContext == nil { + t.Error("connect timeout not wired: Transport.DialContext is nil") + } + if tr.TLSHandshakeTimeout != 2*time.Second { + t.Errorf("connect timeout: TLSHandshakeTimeout = %v, want 2s", tr.TLSHandshakeTimeout) + } +} + func TestPatchSendsPatchMethodAndBody(t *testing.T) { var gotMethod, gotBody string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -28,7 +47,7 @@ func TestPatchSendsPatchMethodAndBody(t *testing.T) { // Pre-seed a static token so GetToken never calls the real IAM endpoint. tm.SetToken("test-token", time.Now().Add(1*time.Hour)) - c := NewGreenodeClient(srv.URL, tm, 5*time.Second, false, false) + c := NewGreenodeClient(srv.URL, tm, 5*time.Second, 5*time.Second, false, false) _, err := c.Patch("/v1/thing", map[string]interface{}{"enableAutoHealing": true}) if err != nil { @@ -70,7 +89,7 @@ func TestGetReturnsTypedAPIErrorOn404(t *testing.T) { tm := auth.NewTokenManager("id", "secret") tm.SetToken("test-token", time.Now().Add(1*time.Hour)) - c := NewGreenodeClient(srv.URL, tm, 5*time.Second, false, false) + c := NewGreenodeClient(srv.URL, tm, 5*time.Second, 5*time.Second, false, false) _, err := c.Get("/v1/clusters/x", nil) if err == nil { diff --git a/go/internal/vserverclient/client.go b/go/internal/vserverclient/client.go index eb0513b..d220c18 100644 --- a/go/internal/vserverclient/client.go +++ b/go/internal/vserverclient/client.go @@ -19,6 +19,7 @@ func BuildClient(cmd *cobra.Command) (*client.GreenodeClient, *config.Config, er endpointURL, _ := cmd.Flags().GetString("endpoint-url") noVerifySSL, _ := cmd.Flags().GetBool("no-verify-ssl") debug, _ := cmd.Flags().GetBool("debug") + connectTimeout, _ := cmd.Flags().GetInt("cli-connect-timeout") readTimeout, _ := cmd.Flags().GetInt("cli-read-timeout") cfg, err := config.LoadConfig(profile) @@ -49,9 +50,10 @@ func BuildClient(cmd *cobra.Command) (*client.GreenodeClient, *config.Config, er } tokenManager := auth.NewTokenManager(cfg.ClientID, cfg.ClientSecret) - timeout := time.Duration(readTimeout) * time.Second + connect := time.Duration(connectTimeout) * time.Second + read := time.Duration(readTimeout) * time.Second - return client.NewGreenodeClient(baseURL, tokenManager, timeout, !noVerifySSL, debug), cfg, nil + return client.NewGreenodeClient(baseURL, tokenManager, connect, read, !noVerifySSL, debug), cfg, nil } // ProjectID extracts and validates the project ID from config. From 38549e163cd487d17a37a9108fd3398465f5999b Mon Sep 17 00:00:00 2001 From: tytv2 Date: Tue, 30 Jun 2026 19:57:19 +0700 Subject: [PATCH 7/7] docs(vks): clarify --unhealthy-range format in help config-auto-healing's --unhealthy-range help said only "Unhealthy range" with no hint at the expected syntax. State the format ("[min-max]", e.g. "[2-5]") matching the API spec, like --max-unhealthy already shows "30%". Co-Authored-By: Claude Opus 4.8 --- .changes/next-release/enhancement-vks-p7174a2k.json | 5 +++++ go/cmd/vks/config_auto_healing.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changes/next-release/enhancement-vks-p7174a2k.json diff --git a/.changes/next-release/enhancement-vks-p7174a2k.json b/.changes/next-release/enhancement-vks-p7174a2k.json new file mode 100644 index 0000000..82a9bd5 --- /dev/null +++ b/.changes/next-release/enhancement-vks-p7174a2k.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "vks", + "description": "Document the --unhealthy-range format in config-auto-healing help: expects \"[min-max]\" (e.g. \"[2-5]\")" +} diff --git a/go/cmd/vks/config_auto_healing.go b/go/cmd/vks/config_auto_healing.go index 9d5202d..2803f39 100644 --- a/go/cmd/vks/config_auto_healing.go +++ b/go/cmd/vks/config_auto_healing.go @@ -19,7 +19,7 @@ func init() { f.String("cluster-id", "", "Cluster ID (required)") f.Bool("enable-auto-healing", false, "Enable auto-healing (required)") f.String("max-unhealthy", "", "Max unhealthy nodes, e.g. \"30%\"") - f.String("unhealthy-range", "", "Unhealthy range") + f.String("unhealthy-range", "", "Unhealthy node count range as \"[min-max]\", e.g. \"[2-5]\"") f.Int("timeout-unhealthy", 0, "Unhealthy timeout in seconds") configAutoHealingCmd.MarkFlagRequired("cluster-id")