diff --git a/cmd/megaport/megaport.go b/cmd/megaport/megaport.go index 9085cf2e..fdf538e1 100644 --- a/cmd/megaport/megaport.go +++ b/cmd/megaport/megaport.go @@ -22,6 +22,13 @@ func init() { // Initialize common components InitializeCommon() + // Structurally tag flag-parsing failures (unknown flag, unknown shorthand + // flag, malformed value) as usage errors at the point cobra generates them, + // instead of pattern-matching the message text later in exitCodeFromError. + rootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { + return exitcodes.NewUsageError(err) + }) + // Apply non-WASM specific initialization rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { defaultWarnings := applyDefaultSettings(cmd) @@ -75,6 +82,14 @@ func init() { return utils.FinishPreRunError(cmd, args, exitcodes.NewUsageError(err)) } + // Run cobra's own required-flags check here, ahead of when cobra would + // normally run it after PersistentPreRunE, so a missing required flag + // gets tagged as a typed usage error instead of falling back to + // isCobraUsageError's message-substring match. + if err := cmd.ValidateRequiredFlags(); err != nil { + return utils.FinishPreRunError(cmd, args, exitcodes.NewUsageError(err)) + } + return nil } @@ -313,6 +328,11 @@ func exitCodeFromError(err error) int { return exitcodes.General } +// isCobraUsageError is a defensive fallback matching cobra's usage-error text. +// Flag-parse and missing-required-flag errors are already typed as usage errors +// at the source (SetFlagErrorFunc and the ValidateRequiredFlags check), so the +// matches for those are a backstop for any that arrive untyped; "unknown command" +// and the arg-count validators have no such hook and rely on this match. func isCobraUsageError(msg string) bool { cobraPatterns := []string{ "unknown command", diff --git a/cmd/megaport/megaport_test.go b/cmd/megaport/megaport_test.go index 590c9313..7f5e3053 100644 --- a/cmd/megaport/megaport_test.go +++ b/cmd/megaport/megaport_test.go @@ -351,3 +351,40 @@ func TestExitCodeFromError_CobraArgValidators(t *testing.T) { }) } } + +// TestUnknownFlagIsTypedUsageError verifies that rootCmd.SetFlagErrorFunc +// tags a flag-parse failure as a typed *exitcodes.CLIError at the point +// cobra generates it, rather than relying on exitCodeFromError's message +// substring match. +func TestUnknownFlagIsTypedUsageError(t *testing.T) { + rootCmd.SetArgs([]string{"version", "--totally-bogus-flag"}) + var execErr error + _ = output.CaptureOutput(func() { + execErr = rootCmd.Execute() + }) + + require.Error(t, execErr) + var cliErr *exitcodes.CLIError + require.True(t, errors.As(execErr, &cliErr), "expected a typed *exitcodes.CLIError, got %T: %v", execErr, execErr) + assert.Equal(t, exitcodes.Usage, cliErr.Code) +} + +// TestMissingRequiredFlagIsTypedUsageError verifies that the proactive +// cmd.ValidateRequiredFlags() check added to PersistentPreRunE tags a +// missing-required-flag error as a typed *exitcodes.CLIError ahead of +// cobra's own (redundant) ValidateRequiredFlags call. +func TestMissingRequiredFlagIsTypedUsageError(t *testing.T) { + t.Setenv("MEGAPORT_CONFIG_DIR", t.TempDir()) + defer output.ResetState() + + rootCmd.SetArgs([]string{"billing-market", "set"}) + var execErr error + _ = output.CaptureOutput(func() { + execErr = rootCmd.Execute() + }) + + require.Error(t, execErr) + var cliErr *exitcodes.CLIError + require.True(t, errors.As(execErr, &cliErr), "expected a typed *exitcodes.CLIError, got %T: %v", execErr, execErr) + assert.Equal(t, exitcodes.Usage, cliErr.Code) +} diff --git a/docs/index.md b/docs/index.md index 93211632..71d4d528 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1,6 +1,6 @@ # Megaport CLI Documentation -> Generated on July 8, 2026 for version v1.0.0-beta.1 +> Generated on July 13, 2026 for version v1.0.0-beta.1 ## Available Commands diff --git a/docs/megaport-cli_ix_buy.md b/docs/megaport-cli_ix_buy.md index 7c63dfe7..61ee6a5a 100644 --- a/docs/megaport-cli_ix_buy.md +++ b/docs/megaport-cli_ix_buy.md @@ -77,5 +77,5 @@ megaport-cli ix buy [flags] | `--rate-limit` | | `0` | Rate limit in Mbps | true | | `--shutdown` | | `false` | Whether the IX is initially shut down | false | | `--vlan` | | `0` | VLAN ID for the IX connection | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_mcr_buy.md b/docs/megaport-cli_mcr_buy.md index 8c21d630..4878e2ae 100644 --- a/docs/megaport-cli_mcr_buy.md +++ b/docs/megaport-cli_mcr_buy.md @@ -96,5 +96,5 @@ megaport-cli mcr buy [flags] | `--resource-tags` | | | Resource tags as a JSON string (e.g. {"key1":"value1","key2":"value2"}) | false | | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--term` | | `0` | The term of the MCR (1, 12, 24, 36, 48, or 60 months) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_mve_buy.md b/docs/megaport-cli_mve_buy.md index 71d9e205..3a9a26c4 100644 --- a/docs/megaport-cli_mve_buy.md +++ b/docs/megaport-cli_mve_buy.md @@ -100,5 +100,5 @@ megaport-cli mve buy [flags] | `--term` | | `0` | The term of the MVE (1, 12, 24, 36, 48, or 60 months) | true | | `--vendor-config` | | | JSON string with vendor-specific configuration (for flag mode) | true | | `--vnics` | | | JSON array of network interfaces (for flag mode) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_nat-gateway_create.md b/docs/megaport-cli_nat-gateway_create.md index 38b9fbf5..8fef4801 100644 --- a/docs/megaport-cli_nat-gateway_create.md +++ b/docs/megaport-cli_nat-gateway_create.md @@ -84,5 +84,5 @@ megaport-cli nat-gateway create [flags] | `--session-count` | | `0` | The number of NAT sessions (optional) | false | | `--speed` | | `0` | The speed of the NAT Gateway in Mbps | true | | `--term` | | `0` | The contract term in months (1, 12, 24, 36, 48, or 60) | true | -| `--yes` | `-y` | `false` | Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy') | false | +| `--yes` | `-y` | `false` | Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy'); required when using --json or --json-file | false | diff --git a/docs/megaport-cli_ports_buy-lag.md b/docs/megaport-cli_ports_buy-lag.md index 9f229113..eeaadc8e 100644 --- a/docs/megaport-cli_ports_buy-lag.md +++ b/docs/megaport-cli_ports_buy-lag.md @@ -85,5 +85,5 @@ megaport-cli ports buy-lag [flags] | `--resource-tags` | | | Resource tags as a JSON string (e.g. {"key1":"value1","key2":"value2"}) | false | | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--term` | | `0` | The term of the port (1, 12, or 24 months) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_ports_buy.md b/docs/megaport-cli_ports_buy.md index f6f9f597..d9780e54 100644 --- a/docs/megaport-cli_ports_buy.md +++ b/docs/megaport-cli_ports_buy.md @@ -84,5 +84,5 @@ megaport-cli ports buy [flags] | `--resource-tags` | | | Resource tags as a JSON string (e.g. {"key1":"value1","key2":"value2"}) | false | | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--term` | | `0` | The term of the port (1, 12, 24, 36, 48, or 60 months) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/docs/megaport-cli_vxc_buy.md b/docs/megaport-cli_vxc_buy.md index 2a131a53..091c25b7 100644 --- a/docs/megaport-cli_vxc_buy.md +++ b/docs/megaport-cli_vxc_buy.md @@ -95,5 +95,5 @@ megaport-cli vxc buy [flags] | `--resource-tags-file` | | | Path to JSON file containing resource tags | false | | `--service-key` | | | Service key | false | | `--term` | | `0` | Contract term in months (1, 12, 24, 36, 48, or 60) | true | -| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase | false | +| `--yes` | `-y` | `false` | Skip confirmation prompt for purchase; required when using --json or --json-file | false | diff --git a/internal/base/cmdbuilder/docs/megaport-cli.md b/internal/base/cmdbuilder/docs/megaport-cli.md new file mode 100644 index 00000000..4252ad6e --- /dev/null +++ b/internal/base/cmdbuilder/docs/megaport-cli.md @@ -0,0 +1,3 @@ +# Embedded Sample + +Embedded documentation used by docs_render tests. diff --git a/internal/base/cmdbuilder/docs_render.go b/internal/base/cmdbuilder/docs_render.go index 849df225..9884851e 100644 --- a/internal/base/cmdbuilder/docs_render.go +++ b/internal/base/cmdbuilder/docs_render.go @@ -7,7 +7,6 @@ import ( "os" "path" "path/filepath" - "strings" "github.com/charmbracelet/glamour" "github.com/spf13/cobra" @@ -16,8 +15,9 @@ import ( // DocsDirectory is the fallback location for markdown documentation files var DocsDirectory = "./docs" -// FindDocFile locates the markdown file for a specific command -func FindDocFile(cmd *cobra.Command) (string, error) { +// FindDocFile locates the markdown file for a specific command. The second +// return value reports whether the path is a temp file the caller must remove. +func FindDocFile(cmd *cobra.Command) (string, bool, error) { cmdPath := getCommandPath(cmd) docName := cmdPath + ".md" @@ -30,18 +30,18 @@ func FindDocFile(cmd *cobra.Command) (string, error) { // by path and the caller removes it after rendering. tempFile, err := os.CreateTemp("", "megaport-docs-*.md") if err != nil { - return "", fmt.Errorf("failed to create temporary file: %w", err) + return "", false, fmt.Errorf("failed to create temporary file: %w", err) } if _, err := tempFile.Write(content); err != nil { _ = tempFile.Close() _ = os.Remove(tempFile.Name()) - return "", fmt.Errorf("failed to write to temporary file: %w", err) + return "", false, fmt.Errorf("failed to write to temporary file: %w", err) } if err := tempFile.Close(); err != nil { _ = os.Remove(tempFile.Name()) - return "", fmt.Errorf("failed to close temporary file: %w", err) + return "", false, fmt.Errorf("failed to close temporary file: %w", err) } - return tempFile.Name(), nil + return tempFile.Name(), true, nil } // If embedded file not found, try local docs directory as fallback @@ -49,10 +49,10 @@ func FindDocFile(cmd *cobra.Command) (string, error) { // Check if the file exists if _, err := os.Stat(docPath); os.IsNotExist(err) { - return "", fmt.Errorf("documentation file not found for %s: %w", cmdPath, err) + return "", false, fmt.Errorf("documentation file not found for %s: %w", cmdPath, err) } - return docPath, nil + return docPath, false, nil } // RenderDocFile reads and renders a markdown file using Glamour @@ -83,13 +83,13 @@ func RenderDocFile(filePath string) (string, error) { // ShowDocumentation displays rendered documentation for a command func ShowDocumentation(cmd *cobra.Command) error { - docPath, err := FindDocFile(cmd) + docPath, isTemp, err := FindDocFile(cmd) if err != nil { return err } // If we created a temporary file, ensure it gets deleted - if strings.Contains(docPath, "megaport-docs-") { + if isTemp { defer os.Remove(docPath) } diff --git a/internal/base/cmdbuilder/docs_render_native_test.go b/internal/base/cmdbuilder/docs_render_native_test.go index f35cba44..47e72811 100644 --- a/internal/base/cmdbuilder/docs_render_native_test.go +++ b/internal/base/cmdbuilder/docs_render_native_test.go @@ -12,6 +12,9 @@ import ( "github.com/spf13/cobra" ) +//go:embed docs/megaport-cli.md +var embeddedTestDocsFS embed.FS + // ShowDocumentation must write to the command's configured output writer, not // straight to os.Stdout, so callers (and tests) can capture/redirect docs output. func TestShowDocumentationWritesToCobraWriter(t *testing.T) { @@ -39,3 +42,54 @@ func TestShowDocumentationWritesToCobraWriter(t *testing.T) { t.Fatal("expected rendered docs on the cobra writer, got nothing") } } + +// ShowDocumentation must stage embedded docs in a temp file and clean it up +// afterwards, since embed.FS content can't be read back by path directly. +func TestShowDocumentationEmbeddedTempFile(t *testing.T) { + origFS := embeddedDocsFS + embeddedDocsFS = embeddedTestDocsFS + t.Cleanup(func() { embeddedDocsFS = origFS }) + + root := &cobra.Command{Use: "megaport-cli"} + var buf bytes.Buffer + root.SetOut(&buf) + + docPath, isTemp, err := FindDocFile(root) + if err != nil { + t.Fatalf("FindDocFile: %v", err) + } + if !isTemp { + t.Fatal("expected embedded doc to be staged in a temp file") + } + defer os.Remove(docPath) + + if _, err := os.Stat(docPath); err != nil { + t.Fatalf("expected temp doc file to exist: %v", err) + } + + if err := ShowDocumentation(root); err != nil { + t.Fatalf("ShowDocumentation: %v", err) + } + if buf.Len() == 0 { + t.Fatal("expected rendered docs on the cobra writer, got nothing") + } +} + +func TestFindDocFileNotFound(t *testing.T) { + origFS := embeddedDocsFS + embeddedDocsFS = embed.FS{} // force the on-disk fallback + t.Cleanup(func() { embeddedDocsFS = origFS }) + + origDir := DocsDirectory + DocsDirectory = t.TempDir() // empty, no doc files present + t.Cleanup(func() { DocsDirectory = origDir }) + + root := &cobra.Command{Use: "megaport-cli"} + _, isTemp, err := FindDocFile(root) + if err == nil { + t.Fatal("expected an error for a missing doc file") + } + if isTemp { + t.Fatal("expected isTemp to be false when no doc file is found") + } +} diff --git a/internal/base/cmdbuilder/flagsets.go b/internal/base/cmdbuilder/flagsets.go index d88aa3fc..02e5ff3b 100644 --- a/internal/base/cmdbuilder/flagsets.go +++ b/internal/base/cmdbuilder/flagsets.go @@ -81,7 +81,7 @@ func (b *CommandBuilder) WithDeferredDeleteFlags() *CommandBuilder { // WithBuyConfirmFlags adds the --yes/-y flag to skip buy confirmation prompts func (b *CommandBuilder) WithBuyConfirmFlags() *CommandBuilder { - b.WithBoolFlagP("yes", "y", false, "Skip confirmation prompt for purchase") + b.WithBoolFlagP("yes", "y", false, "Skip confirmation prompt for purchase; required when using --json or --json-file") return b } diff --git a/internal/base/output/common.go b/internal/base/output/common.go index 8e97c56c..0bc5611f 100644 --- a/internal/base/output/common.go +++ b/internal/base/output/common.go @@ -452,6 +452,63 @@ func filterByFields(headers, jsonNames []string, indices []int, selected []strin return outHeaders, outJSONNames, outIndices, nil } +// isXMLNameStartChar reports whether r is legal as the first character of an XML +// element local name. Colon is excluded on purpose: it is namespace-significant, +// so a name like "a:b" would trip an undeclared-prefix error in aware parsers. +func isXMLNameStartChar(r rune) bool { + return r == '_' || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') +} + +// isXMLNameChar reports whether r is legal as a non-first character of an XML +// element local name. +func isXMLNameChar(r rune) bool { + return isXMLNameStartChar(r) || r == '.' || r == '-' || (r >= '0' && r <= '9') +} + +// sanitizeXMLElementName rewrites name so it is safe to use as an XML element +// local name. Struct json tags are written for JSON, not XML, so a name like +// "a/b" or one starting with a digit would otherwise produce malformed XML. +func sanitizeXMLElementName(name string) string { + var b strings.Builder + b.Grow(len(name)) + for _, r := range name { + if isXMLNameChar(r) { + b.WriteRune(r) + } else { + b.WriteRune('_') + } + } + name = b.String() + if name == "" { + return "field" + } + if !isXMLNameStartChar(rune(name[0])) { + name = "_" + name + } + if strings.HasPrefix(strings.ToLower(name), "xml") { + name = "_" + name + } + return name +} + +// sanitizeXMLElementNames sanitizes each name and disambiguates any collisions +// caused by distinct names sanitizing to the same value (e.g. "a/b" and "a b" +// both becoming "a_b"), so no two elements in the same XML item share a name. +func sanitizeXMLElementNames(names []string) []string { + used := make(map[string]bool, len(names)) + out := make([]string, len(names)) + for i, name := range names { + base := sanitizeXMLElementName(name) + candidate := base + for n := 2; used[candidate]; n++ { + candidate = fmt.Sprintf("%s_%d", base, n) + } + used[candidate] = true + out[i] = candidate + } + return out +} + // isOutputCompatibleType checks if a type can be output func isOutputCompatibleType(t reflect.Type) bool { // Handle pointer types by checking the element type diff --git a/internal/base/output/output.go b/internal/base/output/output.go index 7287bec6..5f54881b 100644 --- a/internal/base/output/output.go +++ b/internal/base/output/output.go @@ -120,6 +120,8 @@ func printXML[T OutputFields](data []T, opts printOptions) error { } } + xmlNames := sanitizeXMLElementNames(jsonNames) + encoder := xml.NewEncoder(os.Stdout) encoder.Indent("", " ") @@ -143,8 +145,8 @@ func printXML[T OutputFields](data []T, opts printOptions) error { return err } - for i, name := range jsonNames { - elemStart := xml.StartElement{Name: xml.Name{Local: name}} + for i := range jsonNames { + elemStart := xml.StartElement{Name: xml.Name{Local: xmlNames[i]}} if err := encoder.EncodeToken(elemStart); err != nil { return err } diff --git a/internal/base/output/output_test.go b/internal/base/output/output_test.go index b85c9d83..6bef2dcc 100644 --- a/internal/base/output/output_test.go +++ b/internal/base/output/output_test.go @@ -7,6 +7,7 @@ import ( "encoding/xml" "errors" "fmt" + "io" "os" "reflect" "regexp" @@ -52,6 +53,16 @@ type NoTagStruct struct { Name string } +type IllegalTagStruct struct { + ID int `json:"1id"` + Value string `json:"a/b c"` +} + +type CollidingTagStruct struct { + First string `json:"a/b"` + Second string `json:"a b"` +} + func TestPrintCSV_SimpleStruct(t *testing.T) { data := []SimpleStruct{ {ID: 1, Name: "Item 1", Active: true}, @@ -870,6 +881,21 @@ func TestPrintXML_NilSlice(t *testing.T) { }) } +// assertValidXML decodes output in full and requires it to end in io.EOF, so +// a malformed document (which would otherwise stop the decoder early with a +// syntax error) fails the assertion instead of silently passing. +func assertValidXML(t *testing.T, output string) { + t.Helper() + decoder := xml.NewDecoder(strings.NewReader(output)) + for { + _, err := decoder.Token() + if err != nil { + assert.ErrorIs(t, err, io.EOF, "output is not well-formed XML") + return + } + } +} + func TestPrintXML_ComplexStruct(t *testing.T) { now := time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC) ref := &SimpleStruct{ID: 100, Name: "Referenced", Active: true} @@ -898,13 +924,7 @@ func TestPrintXML_ComplexStruct(t *testing.T) { assert.Contains(t, output, "") // Verify it's parseable XML - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } - } + assertValidXML(t, output) } func TestPrintXML_PointerStruct(t *testing.T) { @@ -974,15 +994,73 @@ func TestPrintXML_SpecialCharacters(t *testing.T) { assert.Contains(t, output, "&") // Should still be parseable - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } + assertValidXML(t, output) +} + +func TestPrintXML_IllegalElementName(t *testing.T) { + data := []IllegalTagStruct{ + {ID: 1, Value: "test"}, + } + + output := CaptureOutput(func() { + err := printXML(data, currentPrintOptions()) + assert.NoError(t, err) + }) + + // json tag "1id" starts with a digit, which is illegal as an XML name-start + // character; json tag "a/b c" contains characters illegal in an XML name. + assert.NotContains(t, output, "<1id>") + assert.NotContains(t, output, "") + + assertValidXML(t, output) +} + +func TestSanitizeXMLElementName(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + {"already valid", "name", "name"}, + {"empty", "", "field"}, + {"leading digit", "1id", "_1id"}, + {"leading underscore", "_id", "_id"}, + {"illegal characters", "a/b c", "a_b_c"}, + {"xml prefix", "xmlns", "_xmlns"}, + {"mixed case xml prefix", "XMLThing", "_XMLThing"}, + {"colon is namespace-significant, treated as illegal", "a:b", "a_b"}, + {"other punctuation", "a@b#c(d)e,f;g=h", "a_b_c_d_e_f_g_h"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sanitizeXMLElementName(tt.input)) + }) } } +func TestSanitizeXMLElementNames_DisambiguatesCollisions(t *testing.T) { + // "a/b" and "a b" both sanitize to "a_b"; the second must be renamed so + // no two elements in the same XML item share a name. + got := sanitizeXMLElementNames([]string{"a/b", "a b", "c"}) + assert.Equal(t, []string{"a_b", "a_b_2", "c"}, got) +} + +func TestPrintXML_CollidingElementNames(t *testing.T) { + data := []CollidingTagStruct{ + {First: "one", Second: "two"}, + } + + output := CaptureOutput(func() { + err := printXML(data, currentPrintOptions()) + assert.NoError(t, err) + }) + + assert.Contains(t, output, "one") + assert.Contains(t, output, "two") + + assertValidXML(t, output) +} + func TestPrintOutput_XMLFormat(t *testing.T) { data := []SimpleStruct{ {ID: 1, Name: "Test", Active: true}, @@ -1000,13 +1078,7 @@ func TestPrintOutput_XMLFormat(t *testing.T) { assert.Contains(t, output, "") // Verify parseable by xml.Decoder - decoder := xml.NewDecoder(strings.NewReader(output)) - for { - _, err := decoder.Token() - if err != nil { - break - } - } + assertValidXML(t, output) } func TestPrintXML_Parseable(t *testing.T) { @@ -1027,6 +1099,7 @@ func TestPrintXML_Parseable(t *testing.T) { for { tok, err := decoder.Token() if err != nil { + assert.ErrorIs(t, err, io.EOF, "output is not well-formed XML") break } if se, ok := tok.(xml.StartElement); ok && se.Name.Local == "item" { diff --git a/internal/base/output/output_wasm.go b/internal/base/output/output_wasm.go index 9c2035a7..07ffe5eb 100644 --- a/internal/base/output/output_wasm.go +++ b/internal/base/output/output_wasm.go @@ -297,6 +297,12 @@ func printXML[T OutputFields](data []T, opts printOptions) error { fields = filtered } + fieldNames := make([]string, len(fields)) + for i, f := range fields { + fieldNames[i] = f.name + } + xmlNames := sanitizeXMLElementNames(fieldNames) + encoder := xml.NewEncoder(WasmXMLWriter) encoder.Indent("", " ") @@ -326,11 +332,11 @@ func printXML[T OutputFields](data []T, opts printOptions) error { return err } - for _, f := range fields { + for i, f := range fields { fieldVal := v.Field(f.index) valueStr := formatFieldValue(fieldVal) - elemStart := xml.StartElement{Name: xml.Name{Local: f.name}} + elemStart := xml.StartElement{Name: xml.Name{Local: xmlNames[i]}} if err := encoder.EncodeToken(elemStart); err != nil { return err } diff --git a/internal/commands/config/login.go b/internal/commands/config/login.go index e99627ed..f77ad360 100644 --- a/internal/commands/config/login.go +++ b/internal/commands/config/login.go @@ -13,6 +13,7 @@ import ( "sync" "time" + "github.com/megaport/megaport-cli/internal/base/exitcodes" "github.com/megaport/megaport-cli/internal/base/output" "github.com/megaport/megaport-cli/internal/utils" megaport "github.com/megaport/megaportgo" @@ -154,24 +155,27 @@ var loginFuncWithOutput = func(ctx context.Context, outputFormat string) (*megap } else { // Credential selection: if --env flag is used, prefer env vars over profile if utils.Env != "" { - // --env flag was explicitly set, prioritize environment variables - accessKey = os.Getenv("MEGAPORT_ACCESS_KEY") - secretKey = os.Getenv("MEGAPORT_SECRET_KEY") - - // If env vars are empty, fall back to profile - if accessKey == "" || secretKey == "" { + // --env flag was explicitly set, prioritize environment variables, but + // never mix halves across sources: both keys must come from the + // environment, or both from the profile, never one from each. + envAccessKey := os.Getenv("MEGAPORT_ACCESS_KEY") + envSecretKey := os.Getenv("MEGAPORT_SECRET_KEY") + + switch { + case envAccessKey != "" && envSecretKey != "": + accessKey = envAccessKey + secretKey = envSecretKey + case envAccessKey == "" && envSecretKey == "": manager, err := NewConfigManager() if err == nil { profile, _, err := manager.GetCurrentProfile() if err == nil { - if accessKey == "" { - accessKey = profile.AccessKey - } - if secretKey == "" { - secretKey = profile.SecretKey - } + accessKey = profile.AccessKey + secretKey = profile.SecretKey } } + default: + return nil, exitcodes.NewUsageError(fmt.Errorf("only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set; with --env, both must come from the environment or neither should be set")) } } else { // No --env flag, use original priority: profile > env vars diff --git a/internal/commands/config/login_test.go b/internal/commands/config/login_test.go index 48e58d1f..1908a965 100644 --- a/internal/commands/config/login_test.go +++ b/internal/commands/config/login_test.go @@ -5,6 +5,7 @@ package config import ( "bytes" "context" + "errors" "fmt" "io" "log/slog" @@ -15,6 +16,7 @@ import ( "testing" "time" + "github.com/megaport/megaport-cli/internal/base/exitcodes" "github.com/megaport/megaport-cli/internal/utils" megaport "github.com/megaport/megaportgo" "github.com/stretchr/testify/assert" @@ -352,6 +354,81 @@ func TestProfileOverrideLogin(t *testing.T) { }) } +func TestEnvFlagPartialEnvVarsDoesNotMixWithProfile(t *testing.T) { + // Save and restore non-env-var globals + originalEnv := utils.Env + originalProfileOverride := utils.ProfileOverride + originalBaseURL := utils.BaseURL + originalTokenURL := utils.TokenURL + defer func() { + utils.Env = originalEnv + utils.ProfileOverride = originalProfileOverride + utils.BaseURL = originalBaseURL + utils.TokenURL = originalTokenURL + }() + + tempDir, err := os.MkdirTemp("", "megaport-login-test") + assert.NoError(t, err) + t.Cleanup(func() { os.RemoveAll(tempDir) }) + t.Setenv("MEGAPORT_CONFIG_DIR", tempDir) + + manager, err := NewConfigManager() + assert.NoError(t, err) + err = manager.CreateProfile("prod", "profile-access", "profile-secret", "production", "") + assert.NoError(t, err) + err = manager.UseProfile("prod") + assert.NoError(t, err) + + utils.ProfileOverride = "" + utils.Env = "production" + + t.Run("only access key set in env errors instead of mixing with profile", func(t *testing.T) { + t.Setenv("MEGAPORT_ACCESS_KEY", "env-access-key") + t.Setenv("MEGAPORT_SECRET_KEY", "") + + _, err := LoginWithOutput(context.Background(), "json") + assert.Error(t, err) + assert.Contains(t, err.Error(), "only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set") + + var cliErr *exitcodes.CLIError + assert.True(t, errors.As(err, &cliErr), "expected a typed usage error") + assert.Equal(t, exitcodes.Usage, cliErr.Code) + }) + + t.Run("only secret key set in env errors instead of mixing with profile", func(t *testing.T) { + t.Setenv("MEGAPORT_ACCESS_KEY", "") + t.Setenv("MEGAPORT_SECRET_KEY", "env-secret-key") + + _, err := LoginWithOutput(context.Background(), "json") + assert.Error(t, err) + assert.Contains(t, err.Error(), "only one of MEGAPORT_ACCESS_KEY and MEGAPORT_SECRET_KEY is set") + + var cliErr *exitcodes.CLIError + assert.True(t, errors.As(err, &cliErr), "expected a typed usage error") + assert.Equal(t, exitcodes.Usage, cliErr.Code) + }) + + t.Run("neither env var set falls back fully to profile", func(t *testing.T) { + t.Setenv("MEGAPORT_ACCESS_KEY", "") + t.Setenv("MEGAPORT_SECRET_KEY", "") + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer ts.Close() + utils.BaseURL = ts.URL + utils.TokenURL = ts.URL + "/oauth2/token" + + _, err := LoginWithOutput(context.Background(), "json") + assert.Error(t, err) + // Reaches the Authorize call (which fails against the local test + // server) rather than failing on missing credentials or the + // partial-env-var mixing guard. + assert.NotContains(t, err.Error(), "access key not provided") + assert.NotContains(t, err.Error(), "only one of") + }) +} + func TestNewUnauthenticatedClient(t *testing.T) { // Save and restore non-env-var globals originalEnv := utils.Env diff --git a/internal/commands/config/manager.go b/internal/commands/config/manager.go index acfa6ba1..ab7a9577 100644 --- a/internal/commands/config/manager.go +++ b/internal/commands/config/manager.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" "time" ) @@ -227,9 +228,38 @@ func (m *ConfigManager) Save() error { if err != nil { return fmt.Errorf("failed to marshal config: %w", err) } - if err := os.WriteFile(configPath, configData, 0600); err != nil { + + tmpFile, err := os.CreateTemp(filepath.Dir(configPath), ".megaport-config-*.tmp") + if err != nil { + return fmt.Errorf("failed to create temp config file: %w", err) + } + tmpPath := tmpFile.Name() + defer os.Remove(tmpPath) // no-op once the rename below succeeds + + n, err := tmpFile.Write(configData) + if err != nil { + tmpFile.Close() + return fmt.Errorf("failed to write temp config file: %w", err) + } + if n != len(configData) { + tmpFile.Close() + return fmt.Errorf("failed to write temp config file: short write (%d of %d bytes)", n, len(configData)) + } + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close temp config file: %w", err) + } + if err := chmodFile(tmpPath, 0600); err != nil { + return fmt.Errorf("failed to set permissions on temp config file: %w", err) + } + if err := renameFile(tmpPath, configPath); err != nil { return fmt.Errorf("failed to write config file: %w", err) } + // Rename already carries over the temp file's 0600 mode, but re-tighten + // explicitly in case configPath pre-existed with looser permissions on a + // platform where rename doesn't replace the mode bits. + if err := chmodFile(configPath, 0600); err != nil { + return fmt.Errorf("failed to set permissions on config file: %w", err) + } return nil } diff --git a/internal/commands/config/manager_test.go b/internal/commands/config/manager_test.go index 57187241..e8e61c93 100644 --- a/internal/commands/config/manager_test.go +++ b/internal/commands/config/manager_test.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "testing" "github.com/megaport/megaport-cli/internal/utils" @@ -501,6 +502,63 @@ func TestCorruptedConfigFile_ChmodFailure(t *testing.T) { assert.Empty(t, profiles) } +func TestConfigManagerSave_ChmodTmpFileError(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + old := chmodFile + defer func() { chmodFile = old }() + chmodFile = func(_ string, _ os.FileMode) error { + return fmt.Errorf("chmod: operation not permitted") + } + + err = manager.Save() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to set permissions on temp config file") +} + +func TestConfigManagerSave_RenameError(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + old := renameFile + defer func() { renameFile = old }() + renameFile = func(_, _ string) error { + return fmt.Errorf("rename: permission denied") + } + + err = manager.Save() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to write config file") +} + +func TestConfigManagerSave_ChmodFinalError(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + old := chmodFile + defer func() { chmodFile = old }() + callCount := 0 + chmodFile = func(path string, mode os.FileMode) error { + callCount++ + if callCount == 1 { + // Let the temp file's own chmod succeed; only fail the final one. + return os.Chmod(path, mode) + } + return fmt.Errorf("chmod: operation not permitted") + } + + err = manager.Save() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to set permissions on config file") +} + func TestCorruptedConfigFile_ConcurrentRecovery(t *testing.T) { setupTestConfig(t) @@ -641,6 +699,9 @@ func TestConfigVersionHandling(t *testing.T) { assert.Equal(t, ConfigVersion, manager.config.Version, "Config should be upgraded to current version") } func TestReadOnlyConfigFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("os.Chmod doesn't reliably block directory writes on Windows, and error text differs") + } if os.Geteuid() == 0 { t.Skip("Skipping test when running as root") } @@ -667,17 +728,62 @@ func TestReadOnlyConfigFile(t *testing.T) { err = os.WriteFile(configPath, data, 0644) require.NoError(t, err) - // Make the config file read-only - err = os.Chmod(configPath, 0444) + // Make the config directory read-only. Save() now writes atomically + // (temp file + rename), so a read-only config file no longer blocks a + // write; only a read-only directory does, since it prevents creating + // the temp file. + err = os.Chmod(configDir, 0555) require.NoError(t, err) + defer os.Chmod(configDir, 0700) //nolint:errcheck // best-effort restore so cleanup can remove tempDir // Try to load config - should fail due to version migration - // attempting to save to read-only file + // attempting to save while the config directory is read-only _, err = NewConfigManager() assert.Error(t, err) assert.Contains(t, err.Error(), "permission denied") } +func TestSaveRetightensOverPermissiveFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits are not meaningful on Windows") + } + if os.Geteuid() == 0 { + t.Skip("Skipping test when running as root") + } + + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + + configPath, err := GetConfigFilePath() + require.NoError(t, err) + + // Simulate a pre-existing config file left world-readable. + require.NoError(t, os.Chmod(configPath, 0644)) + + require.NoError(t, manager.CreateProfile("test-profile", "access123", "secret123", "production", "")) + + info, err := os.Stat(configPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0600), info.Mode().Perm(), "Save should re-tighten an over-permissive config file to 0600") +} + +func TestSaveLeavesNoTempFile(t *testing.T) { + setupTestConfig(t) + + manager, err := NewConfigManager() + require.NoError(t, err) + require.NoError(t, manager.CreateProfile("test-profile", "access123", "secret123", "production", "")) + + configPath, err := GetConfigFilePath() + require.NoError(t, err) + + matches, err := filepath.Glob(filepath.Join(filepath.Dir(configPath), ".megaport-config-*.tmp")) + require.NoError(t, err) + assert.Empty(t, matches, "Save should not leave temp files behind after a successful write") +} + func TestProfileNameCaseSensitivity(t *testing.T) { setupTestConfig(t) diff --git a/internal/commands/ix/ix_actions.go b/internal/commands/ix/ix_actions.go index d0125c01..e9007c97 100644 --- a/internal/commands/ix/ix_actions.go +++ b/internal/commands/ix/ix_actions.go @@ -198,6 +198,11 @@ func BuyIX(cmd *cobra.Command, args []string, noColor bool) error { return err } + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err + } + // Flag read errors are intentionally ignored — flags are registered by the command builder. noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK @@ -220,10 +225,7 @@ func BuyIX(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Network Service Type", Value: req.NetworkServiceType}, diff --git a/internal/commands/ix/ix_actions_test.go b/internal/commands/ix/ix_actions_test.go index 98d47790..953a8219 100644 --- a/internal/commands/ix/ix_actions_test.go +++ b/internal/commands/ix/ix_actions_test.go @@ -1556,9 +1556,11 @@ func TestBuyIX_JSONStringMode(t *testing.T) { cmd.Flags().String("promo-code", "", "Promo code") cmd.Flags().String("json", "", "JSON string") cmd.Flags().String("json-file", "", "JSON file") + cmd.Flags().Bool("yes", false, "Skip confirmation") jsonInput := `{"productUid":"port-uid-json","productName":"JSON IX","networkServiceType":"Sydney IX","asn":65100,"macAddress":"AA:BB:CC:DD:EE:FF","rateLimit":2000,"vlan":200}` _ = cmd.Flags().Set("json", jsonInput) + _ = cmd.Flags().Set("yes", "true") var err error var capturedStderr string @@ -1735,15 +1737,25 @@ func TestBuyIX_Confirmation(t *testing.T) { promptShouldBeCalled: false, }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", flags: map[string]string{ "json": `{"productUid":"port-uid-123","productName":"Test IX","networkServiceType":"Los Angeles IX","asn":65000,"macAddress":"00:11:22:33:44:55","rateLimit":1000,"vlan":100}`, + "yes": "true", }, confirmResult: false, expectBuyCalled: true, expectedOutput: "IX created", promptShouldBeCalled: false, }, + { + name: "json input without yes is a usage error", + flags: map[string]string{ + "json": `{"productUid":"port-uid-123","productName":"Test IX","networkServiceType":"Los Angeles IX","asn":65000,"macAddress":"00:11:22:33:44:55","rateLimit":1000,"vlan":100}`, + }, + expectBuyCalled: false, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + promptShouldBeCalled: false, + }, } for _, tt := range tests { diff --git a/internal/commands/ix/ix_test.go b/internal/commands/ix/ix_test.go index c4e853f3..e66c259f 100644 --- a/internal/commands/ix/ix_test.go +++ b/internal/commands/ix/ix_test.go @@ -1268,7 +1268,7 @@ func TestBuildIXRequestFromPrompt(t *testing.T) { "Los Angeles IX", "notanumber", }, - expectedError: "invalid ASN", + expectedError: "Invalid ASN", }, { name: "empty MAC address", @@ -1291,7 +1291,7 @@ func TestBuildIXRequestFromPrompt(t *testing.T) { "00:11:22:33:44:55", "notanumber", }, - expectedError: "invalid rate limit", + expectedError: "Invalid rate limit", }, { name: "invalid VLAN (non-numeric)", @@ -1304,7 +1304,7 @@ func TestBuildIXRequestFromPrompt(t *testing.T) { "1000", "notanumber", }, - expectedError: "invalid VLAN", + expectedError: "Invalid VLAN", }, { name: "prompt error on first prompt", @@ -1474,7 +1474,7 @@ func TestBuildUpdateIXRequestFromPrompt(t *testing.T) { "", // name "notanumber", // rate-limit }, - expectedError: "invalid rate limit", + expectedError: "Invalid rate limit", }, { name: "invalid VLAN", @@ -1484,7 +1484,7 @@ func TestBuildUpdateIXRequestFromPrompt(t *testing.T) { "", // cost-centre "notanumber", // vlan }, - expectedError: "invalid VLAN", + expectedError: "Invalid VLAN", }, { name: "invalid ASN", @@ -1496,7 +1496,7 @@ func TestBuildUpdateIXRequestFromPrompt(t *testing.T) { "", // mac-address "notanumber", // asn }, - expectedError: "invalid ASN", + expectedError: "Invalid ASN", }, { name: "prompt error", diff --git a/internal/commands/locations/locations_actions.go b/internal/commands/locations/locations_actions.go index 8f0c9f77..e868aa28 100644 --- a/internal/commands/locations/locations_actions.go +++ b/internal/commands/locations/locations_actions.go @@ -322,6 +322,9 @@ func GetLocation(cmd *cobra.Command, args []string, noColor bool, outputFormat s var targetLocation *megaport.LocationV3 for _, loc := range locations { + if loc == nil { + continue + } if loc.ID == locationID { targetLocation = loc break diff --git a/internal/commands/locations/locations_actions_test.go b/internal/commands/locations/locations_actions_test.go index cc26849a..a984a4f5 100644 --- a/internal/commands/locations/locations_actions_test.go +++ b/internal/commands/locations/locations_actions_test.go @@ -347,7 +347,7 @@ func TestGetLocation(t *testing.T) { name: "invalid ID arg", args: []string{"abc"}, setupMock: func(m *MockLocationsService) {}, - expectedErr: "invalid location ID", + expectedErr: "Invalid location ID", }, { name: "not found", @@ -365,6 +365,14 @@ func TestGetLocation(t *testing.T) { }, expectedErr: "failed to list locations", }, + { + name: "nil entry in list does not panic", + args: []string{"2"}, + setupMock: func(m *MockLocationsService) { + m.ListLocationsV3Result = append([]*megaport.LocationV3{nil}, testLocationsV3...) + }, + expectedOutput: "London Data Center", + }, { name: "client creation error", args: []string{"1"}, diff --git a/internal/commands/mcr/mcr_actions.go b/internal/commands/mcr/mcr_actions.go index 487a3902..ab165513 100644 --- a/internal/commands/mcr/mcr_actions.go +++ b/internal/commands/mcr/mcr_actions.go @@ -80,6 +80,11 @@ func BuyMCR(cmd *cobra.Command, args []string, noColor bool) error { }) } + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err + } + // Flag read errors are intentionally ignored — flags are registered by the command builder. noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK @@ -102,10 +107,7 @@ func BuyMCR(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/mcr/mcr_actions_prefix_filter_test.go b/internal/commands/mcr/mcr_actions_prefix_filter_test.go index 28cd8e91..a508d375 100644 --- a/internal/commands/mcr/mcr_actions_prefix_filter_test.go +++ b/internal/commands/mcr/mcr_actions_prefix_filter_test.go @@ -66,5 +66,5 @@ func TestUpdateMCRPrefixFilterList_NonNumericID_PrintsToStderr(t *testing.T) { require.Error(t, err) }) - assert.Contains(t, stderr, "invalid prefix filter list ID", "failing command must print its error to stderr") + assert.Contains(t, stderr, "Invalid prefix filter list ID", "failing command must print its error to stderr") } diff --git a/internal/commands/mcr/mcr_actions_test.go b/internal/commands/mcr/mcr_actions_test.go index 28046771..9bb30ece 100644 --- a/internal/commands/mcr/mcr_actions_test.go +++ b/internal/commands/mcr/mcr_actions_test.go @@ -688,7 +688,7 @@ func TestGetMCRPrefixFilterListCmd_WithMockClient(t *testing.T) { mcrUID: "mcr-123", rawPrefixListID: "abc", setupMock: func(m *MockMCRService) {}, - expectedError: "invalid prefix filter list ID", + expectedError: "Invalid prefix filter list ID", }, } @@ -793,7 +793,7 @@ func TestDeleteMCRPrefixFilterListCmd_WithMockClient(t *testing.T) { rawPrefixListID: "abc", force: true, setupMock: func(m *MockMCRService) {}, - expectedError: "invalid prefix filter list ID", + expectedError: "Invalid prefix filter list ID", }, } @@ -1098,6 +1098,7 @@ func TestBuyMCRCmd_WithMockClient(t *testing.T) { name: "JSON string mode success", flags: map[string]string{ "json": `{"name":"JSON MCR","term":24,"portSpeed":10000,"locationId":123,"mcrAsn":65000,"diversityZone":"green","costCentre":"cost-789","promoCode":"JSONPROMO","marketplaceVisibility":true}`, + "yes": "true", }, setupMock: func(m *MockMCRService) { m.BuyMCRResult = &megaport.BuyMCRResponse{ @@ -2683,7 +2684,7 @@ func TestUpdateMCRPrefixFilterList(t *testing.T) { { name: "invalid prefix filter list ID", args: []string{"mcr-123", "abc"}, - expectedError: "invalid prefix filter list ID", + expectedError: "Invalid prefix filter list ID", }, { name: "API error", @@ -3017,15 +3018,25 @@ func TestBuyMCR_Confirmation(t *testing.T) { promptShouldBeCalled: false, }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", flags: map[string]string{ "json": `{"name":"JSON MCR","term":12,"portSpeed":10000,"locationId":123,"mcrAsn":65000}`, + "yes": "true", }, confirmResult: false, expectBuyCalled: true, expectedOutput: "MCR created", promptShouldBeCalled: false, }, + { + name: "json input without yes is a usage error", + flags: map[string]string{ + "json": `{"name":"JSON MCR","term":12,"portSpeed":10000,"locationId":123,"mcrAsn":65000}`, + }, + expectBuyCalled: false, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + promptShouldBeCalled: false, + }, } for _, tt := range tests { diff --git a/internal/commands/mcr/mcr_prompts_test.go b/internal/commands/mcr/mcr_prompts_test.go index 16ffeffd..e51ee491 100644 --- a/internal/commands/mcr/mcr_prompts_test.go +++ b/internal/commands/mcr/mcr_prompts_test.go @@ -72,7 +72,7 @@ func TestPromptForMCRDetails_InvalidTerm(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") } func TestPromptForMCRDetails_InvalidPortSpeed(t *testing.T) { @@ -93,7 +93,7 @@ func TestPromptForMCRDetails_InvalidLocationID(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") } func TestPromptForMCRDetails_MarketplaceVisibilityPromptError(t *testing.T) { @@ -125,7 +125,7 @@ func TestPromptForMCRDetails_InvalidPortSpeedNotNumeric(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid port speed") + assert.Contains(t, err.Error(), "Invalid port speed") assert.NotContains(t, err.Error(), "strconv") } @@ -148,7 +148,7 @@ func TestPromptForMCRDetails_InvalidASN(t *testing.T) { _, err := promptForMCRDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid ASN") + assert.Contains(t, err.Error(), "Invalid ASN") assert.NotContains(t, err.Error(), "strconv") } @@ -309,7 +309,7 @@ func TestPromptForUpdateMCRDetails_InvalidTermNotNumeric(t *testing.T) { _, err := promptForUpdateMCRDetails("mcr-123", "", true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") assert.NotContains(t, err.Error(), "strconv") } @@ -359,7 +359,7 @@ func TestPromptForUpdateMCRDetails_InvalidASN(t *testing.T) { _, err := promptForUpdateMCRDetails("mcr-123", "", true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid ASN") + assert.Contains(t, err.Error(), "Invalid ASN") assert.NotContains(t, err.Error(), "strconv") } @@ -515,7 +515,7 @@ func TestPromptForIPSecTunnelCount(t *testing.T) { {"valid count 10", "10", 10, false, ""}, {"valid count 20", "20", 20, false, ""}, {"valid count 30", "30", 30, false, ""}, - {"non-numeric input", "abc", 0, true, "invalid tunnel count"}, + {"non-numeric input", "abc", 0, true, "Invalid tunnel count"}, } for _, tt := range tests { @@ -550,7 +550,7 @@ func TestPromptForIPSecTunnelCountUpdate(t *testing.T) { {"valid count 30", "30", 30, false, ""}, {"zero disables IPSec", "0", 0, false, ""}, {"empty input requires value", "", 0, true, "tunnel count is required"}, - {"non-numeric input", "abc", 0, true, "invalid tunnel count"}, + {"non-numeric input", "abc", 0, true, "Invalid tunnel count"}, } for _, tt := range tests { diff --git a/internal/commands/mve/mve_actions.go b/internal/commands/mve/mve_actions.go index 16283a32..6d6cc697 100644 --- a/internal/commands/mve/mve_actions.go +++ b/internal/commands/mve/mve_actions.go @@ -141,6 +141,11 @@ func BuyMVE(cmd *cobra.Command, args []string, noColor bool) error { return err } + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err + } + client, err := config.Login(ctx) if err != nil { output.PrintError("Failed to log in: %v", noColor, err) @@ -165,10 +170,7 @@ func BuyMVE(cmd *cobra.Command, args []string, noColor bool) error { output.PrintInfo("Validation successful", noColor) - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/mve/mve_actions_test.go b/internal/commands/mve/mve_actions_test.go index a2b6d756..39100b86 100644 --- a/internal/commands/mve/mve_actions_test.go +++ b/internal/commands/mve/mve_actions_test.go @@ -939,6 +939,7 @@ func TestBuyMVE(t *testing.T) { name: "json mode success", args: []string{}, flags: map[string]string{ + "yes": "true", "json": `{ "name": "JSON MVE", "term": 12, @@ -1041,10 +1042,36 @@ func TestBuyMVE(t *testing.T) { { name: "invalid JSON returns error", flags: map[string]string{ + "yes": "true", "json": `{bad json}`, }, expectedError: "failed to parse JSON", }, + { + name: "json without yes is a usage error", + flags: map[string]string{ + "json": `{ + "name": "JSON MVE", + "term": 12, + "locationId": 123, + "vendorConfig": { + "vendor": "cisco", + "imageId": 1, + "productSize": "LARGE", + "mveLabel": "json-label", + "manageLocally": true, + "adminSshPublicKey": "admin-ssh", + "sshPublicKey": "ssh-key", + "cloudInit": "cloud-init", + "fmcIpAddress": "fmc-ip", + "fmcRegistrationKey": "fmc-key", + "fmcNatId": "fmc-nat" + }, + "vnics": [{"description": "JSON VNIC", "vlan": 200}] + }`, + }, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + }, { name: "interactive combined with JSON is a usage error", interactive: true, @@ -1109,6 +1136,7 @@ func TestBuyMVE(t *testing.T) { cmd.Flags().Int("location-id", 0, "") cmd.Flags().String("vendor-config", "", "") cmd.Flags().String("vnics", "", "") + cmd.Flags().Bool("yes", false, "") testutil.SetFlags(t, cmd, tt.flags) @@ -2248,15 +2276,25 @@ func TestBuyMVE_Confirmation(t *testing.T) { promptShouldBeCalled: false, }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", flags: map[string]string{ "json": `{"name":"JSON MVE","term":12,"locationId":123,"vendorConfig":{"vendor":"cisco","imageId":1,"productSize":"LARGE","mveLabel":"label-1","manageLocally":true,"adminSshPublicKey":"admin-ssh","sshPublicKey":"ssh-key","cloudInit":"cloud-init","fmcIpAddress":"fmc-ip","fmcRegistrationKey":"fmc-key","fmcNatId":"fmc-nat"},"vnics":[{"description":"VNIC 1","vlan":100}]}`, + "yes": "true", }, confirmResult: false, expectBuyCalled: true, expectedOutput: "MVE created", promptShouldBeCalled: false, }, + { + name: "json input without yes is a usage error", + flags: map[string]string{ + "json": `{"name":"JSON MVE","term":12,"locationId":123,"vendorConfig":{"vendor":"cisco","imageId":1,"productSize":"LARGE","mveLabel":"label-1","manageLocally":true,"adminSshPublicKey":"admin-ssh","sshPublicKey":"ssh-key","cloudInit":"cloud-init","fmcIpAddress":"fmc-ip","fmcRegistrationKey":"fmc-key","fmcNatId":"fmc-nat"},"vnics":[{"description":"VNIC 1","vlan":100}]}`, + }, + expectBuyCalled: false, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + promptShouldBeCalled: false, + }, } for _, tt := range tests { diff --git a/internal/commands/mve/mve_prompts_test.go b/internal/commands/mve/mve_prompts_test.go index d880bd05..faf4a2b5 100644 --- a/internal/commands/mve/mve_prompts_test.go +++ b/internal/commands/mve/mve_prompts_test.go @@ -151,7 +151,7 @@ func TestPromptMVEBaseDetails_InvalidTerm(t *testing.T) { _, _, _, _, _, err := promptMVEBaseDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") } func TestPromptMVEBaseDetails_InvalidLocationID(t *testing.T) { @@ -162,7 +162,7 @@ func TestPromptMVEBaseDetails_InvalidLocationID(t *testing.T) { _, _, _, _, _, err := promptMVEBaseDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") } func TestPromptMVEBaseDetails_InvalidImageID(t *testing.T) { @@ -176,7 +176,7 @@ func TestPromptMVEBaseDetails_InvalidImageID(t *testing.T) { _, _, _, _, _, err := promptMVEBaseDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid image ID") + assert.Contains(t, err.Error(), "Invalid image ID") } // promptForUpdateMVEDetails tests @@ -217,7 +217,7 @@ func TestPromptForUpdateMVEDetails_InvalidContractTerm(t *testing.T) { _, err := promptForUpdateMVEDetails("mve-123", "", nil, true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid contract term") + assert.Contains(t, err.Error(), "Invalid contract term") } func TestPromptForUpdateMVEDetails_UpdateVnicDescriptions(t *testing.T) { @@ -397,7 +397,7 @@ func TestPromptMVEVnics_InvalidVLAN(t *testing.T) { _, err := promptMVEVnics(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid VLAN ID") + assert.Contains(t, err.Error(), "Invalid VLAN ID") } // promptMVEVendorConfig tests — cisco/palo_alto admin password handling diff --git a/internal/commands/nat_gateway/nat_gateway.go b/internal/commands/nat_gateway/nat_gateway.go index 94acbd86..ded8f25d 100644 --- a/internal/commands/nat_gateway/nat_gateway.go +++ b/internal/commands/nat_gateway/nat_gateway.go @@ -63,7 +63,7 @@ func buildNATGatewayCommands(rootCmd *cobra.Command) (get, list, create, update, create = cmdbuilder.NewCommand("create", "Create a new NAT Gateway"). WithColorAwareRunFunc(CreateNATGateway). - WithBoolFlagP("yes", "y", false, "Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy')"). + WithBoolFlagP("yes", "y", false, "Skip the confirmation prompt for creating the NAT Gateway design (no charges are incurred until 'nat-gateway buy'); required when using --json or --json-file"). WithNATGatewayCreateFlags(). WithStandardInputFlags(). WithLongDesc("Create a new NAT Gateway through the Megaport API.\n\nThis command creates a NAT Gateway by providing the necessary details."). diff --git a/internal/commands/nat_gateway/nat_gateway_actions.go b/internal/commands/nat_gateway/nat_gateway_actions.go index f3e9c235..40ad8845 100644 --- a/internal/commands/nat_gateway/nat_gateway_actions.go +++ b/internal/commands/nat_gateway/nat_gateway_actions.go @@ -23,6 +23,7 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { interactive, _ := cmd.Flags().GetBool("interactive") jsonStr, _ := cmd.Flags().GetString("json") jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") flagsProvided := cmd.Flags().Changed("name") || cmd.Flags().Changed("term") || cmd.Flags().Changed("speed") || cmd.Flags().Changed("location-id") || cmd.Flags().Changed("session-count") || cmd.Flags().Changed("asn") || @@ -34,6 +35,10 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { return err } + if !yes && (jsonStr != "" || jsonFile != "") { + return exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm creating a NAT Gateway design when using --json or --json-file")) + } + var req *megaport.CreateNATGatewayRequest var err error @@ -53,8 +58,6 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { return err } - yes, _ := cmd.Flags().GetBool("yes") - client, err := config.Login(ctx) if err != nil { output.PrintError("Failed to log in: %v", noColor, err) @@ -66,7 +69,7 @@ func CreateNATGateway(cmd *cobra.Command, args []string, noColor bool) error { return err } - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.ProductName}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/nat_gateway/nat_gateway_additional_test.go b/internal/commands/nat_gateway/nat_gateway_additional_test.go index 4ff4bb44..fce7214a 100644 --- a/internal/commands/nat_gateway/nat_gateway_additional_test.go +++ b/internal/commands/nat_gateway/nat_gateway_additional_test.go @@ -880,6 +880,7 @@ func TestCreateNATGateway_JSONWithSessionCount(t *testing.T) { cmd := newTestCmd("create") require.NoError(t, cmd.Flags().Set("json", `{"name":"GW","term":12,"speed":1000,"locationId":1,"sessionCount":500,"diversityZone":"blue","autoRenewTerm":true}`)) + require.NoError(t, cmd.Flags().Set("yes", "true")) err := CreateNATGateway(cmd, nil, true) assert.NoError(t, err) diff --git a/internal/commands/nat_gateway/nat_gateway_test.go b/internal/commands/nat_gateway/nat_gateway_test.go index 2673dc5a..f01634cd 100644 --- a/internal/commands/nat_gateway/nat_gateway_test.go +++ b/internal/commands/nat_gateway/nat_gateway_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/megaport/megaport-cli/internal/testutil" + "github.com/megaport/megaport-cli/internal/utils" megaport "github.com/megaport/megaportgo" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -80,6 +81,7 @@ func TestCreateNATGateway_JSON(t *testing.T) { cmd := newTestCmd("create") require.NoError(t, cmd.Flags().Set("json", `{"name":"JSON GW","term":12,"speed":2000,"locationId":456}`)) + require.NoError(t, cmd.Flags().Set("yes", "true")) err := CreateNATGateway(cmd, nil, true) assert.NoError(t, err) @@ -88,6 +90,64 @@ func TestCreateNATGateway_JSON(t *testing.T) { assert.Equal(t, 456, mock.CapturedCreateReq.LocationID) } +func TestCreateNATGateway_JSONWithoutYes(t *testing.T) { + mock := &MockNATGatewayService{} + defer setupMockNATGateway(mock)() + + cmd := newTestCmd("create") + require.NoError(t, cmd.Flags().Set("json", `{"name":"JSON GW","term":12,"speed":2000,"locationId":456}`)) + + err := CreateNATGateway(cmd, nil, true) + assert.Error(t, err) + assert.Contains(t, err.Error(), "--yes is required to confirm creating a NAT Gateway design when using --json or --json-file") +} + +func TestCreateNATGateway_ConfirmationPrompt(t *testing.T) { + mock := &MockNATGatewayService{} + defer setupMockNATGateway(mock)() + + originalDesignConfirmPrompt := utils.GetDesignConfirmPrompt() + defer func() { utils.SetDesignConfirmPrompt(originalDesignConfirmPrompt) }() + + promptCalled := false + utils.SetDesignConfirmPrompt(func(_ string, _ []utils.BuyConfirmDetail, _ bool) bool { + promptCalled = true + return true + }) + + cmd := newTestCmd("create") + require.NoError(t, cmd.Flags().Set("name", "My NAT GW")) + require.NoError(t, cmd.Flags().Set("term", "12")) + require.NoError(t, cmd.Flags().Set("speed", "1000")) + require.NoError(t, cmd.Flags().Set("location-id", "123")) + + err := CreateNATGateway(cmd, nil, true) + assert.NoError(t, err) + assert.True(t, promptCalled) + require.NotNil(t, mock.CapturedCreateReq) + assert.Equal(t, "My NAT GW", mock.CapturedCreateReq.ProductName) +} + +func TestCreateNATGateway_ConfirmationDenied(t *testing.T) { + mock := &MockNATGatewayService{} + defer setupMockNATGateway(mock)() + + originalDesignConfirmPrompt := utils.GetDesignConfirmPrompt() + defer func() { utils.SetDesignConfirmPrompt(originalDesignConfirmPrompt) }() + utils.SetDesignConfirmPrompt(func(_ string, _ []utils.BuyConfirmDetail, _ bool) bool { return false }) + + cmd := newTestCmd("create") + require.NoError(t, cmd.Flags().Set("name", "My NAT GW")) + require.NoError(t, cmd.Flags().Set("term", "12")) + require.NoError(t, cmd.Flags().Set("speed", "1000")) + require.NoError(t, cmd.Flags().Set("location-id", "123")) + + err := CreateNATGateway(cmd, nil, true) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cancelled by user") + assert.Nil(t, mock.CapturedCreateReq) +} + func TestCreateNATGateway_NoInput(t *testing.T) { mock := &MockNATGatewayService{} defer setupMockNATGateway(mock)() diff --git a/internal/commands/partners/partners_actions_test.go b/internal/commands/partners/partners_actions_test.go index fa03a206..0f72c55c 100644 --- a/internal/commands/partners/partners_actions_test.go +++ b/internal/commands/partners/partners_actions_test.go @@ -109,7 +109,7 @@ func TestFindPartners(t *testing.T) { "", "table", }, - expectedError: "invalid location ID", + expectedError: "Invalid location ID", setupMock: func(t *testing.T, m *MockPartnerService) { m.listPartnersResponse = []*megaport.PartnerMegaport{} m.listPartnersErr = nil diff --git a/internal/commands/ports/ports_actions.go b/internal/commands/ports/ports_actions.go index bbcfce7f..5b09f843 100644 --- a/internal/commands/ports/ports_actions.go +++ b/internal/commands/ports/ports_actions.go @@ -73,6 +73,11 @@ func BuyPort(cmd *cobra.Command, args []string, noColor bool) error { return err } + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err + } + // Flag read errors are intentionally ignored — flags are registered by the command builder. noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK @@ -95,10 +100,7 @@ func BuyPort(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, @@ -222,6 +224,11 @@ func BuyLAGPort(cmd *cobra.Command, args []string, noColor bool) error { return err } + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err + } + noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK // must not also poll for provisioning: a 429 raised during polling would @@ -243,10 +250,7 @@ func BuyLAGPort(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.Name}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/ports/ports_actions_test.go b/internal/commands/ports/ports_actions_test.go index 783e6f2a..1ee07e63 100644 --- a/internal/commands/ports/ports_actions_test.go +++ b/internal/commands/ports/ports_actions_test.go @@ -979,9 +979,11 @@ func TestBuyPort(t *testing.T) { cmd.Flags().Bool("marketplace-visibility", false, "") cmd.Flags().String("diversity-zone", "", "") cmd.Flags().Bool("cost-confirm", true, "") + cmd.Flags().Bool("yes", false, "") if tt.jsonInput != "" { require.NoError(t, cmd.Flags().Set("json", tt.jsonInput)) + require.NoError(t, cmd.Flags().Set("yes", "true")) } for k, v := range tt.flags { require.NoError(t, cmd.Flags().Set(k, v)) @@ -1417,7 +1419,7 @@ func TestCheckPortVLANAvailability(t *testing.T) { name: "invalid VLAN arg", portUID: "port-vlan-3", vlanArg: "abc", - expectedError: "invalid VLAN ID", + expectedError: "Invalid VLAN ID", }, { name: "VLAN out of assignable range", @@ -1862,11 +1864,17 @@ func TestBuyPort_Confirmation(t *testing.T) { expectedContains: "new-port-uid-123", }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", jsonInput: `{"name":"json-port","term":12,"portSpeed":1000,"locationId":1,"marketPlaceVisibility":false}`, + yesFlag: true, expectPromptCalled: false, expectedContains: "new-port-uid-123", }, + { + name: "json input without yes is a usage error", + jsonInput: `{"name":"json-port","term":12,"portSpeed":1000,"locationId":1,"marketPlaceVisibility":false}`, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + }, } for _, tt := range tests { @@ -2077,6 +2085,34 @@ func TestBuyLAGPort_ConfirmationDenied(t *testing.T) { assert.Contains(t, capturedOutput, "Purchase cancelled") } +func TestBuyLAGPort_JSONWithoutYes(t *testing.T) { + cleanup := testutil.SetupLogin(func(c *megaport.Client) { + c.PortService = &MockPortService{} + }) + defer cleanup() + + cmd := &cobra.Command{Use: "buy-lag"} + cmd.Flags().Bool("interactive", false, "") + cmd.Flags().Bool("no-wait", false, "") + cmd.Flags().Bool("yes", false, "") + cmd.Flags().String("json", "", "") + cmd.Flags().String("json-file", "", "") + cmd.Flags().String("name", "", "") + cmd.Flags().Int("term", 0, "") + cmd.Flags().Int("port-speed", 0, "") + cmd.Flags().Int("location-id", 0, "") + cmd.Flags().Bool("marketplace-visibility", false, "") + cmd.Flags().String("diversity-zone", "", "") + cmd.Flags().Bool("cost-confirm", true, "") + cmd.Flags().Int("lag-count", 0, "") + + require.NoError(t, cmd.Flags().Set("json", `{"name":"json-lag","term":12,"portSpeed":10000,"locationId":1,"lagCount":2,"marketPlaceVisibility":true}`)) + + err := BuyLAGPort(cmd, nil, true) + assert.Error(t, err) + assert.Contains(t, err.Error(), "--yes is required to confirm a purchase when using --json or --json-file") +} + func TestValidatePort(t *testing.T) { cleanup := testutil.SetupLogin(func(c *megaport.Client) {}) defer cleanup() diff --git a/internal/commands/ports/ports_prompts_test.go b/internal/commands/ports/ports_prompts_test.go index c6ef3a93..dd6248cb 100644 --- a/internal/commands/ports/ports_prompts_test.go +++ b/internal/commands/ports/ports_prompts_test.go @@ -76,7 +76,7 @@ func TestPromptForPortDetails_InvalidTerm(t *testing.T) { _, err := promptForPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") } func TestPromptForPortDetails_InvalidPortSpeed(t *testing.T) { @@ -98,7 +98,7 @@ func TestPromptForPortDetails_InvalidLocationID(t *testing.T) { _, err := promptForPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") } func TestPromptForPortDetails_InvalidMarketplaceVisibility(t *testing.T) { @@ -234,7 +234,7 @@ func TestPromptForUpdatePortDetails_InvalidTermNotNumeric(t *testing.T) { _, err := promptForUpdatePortDetails("port-123", "", true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") assert.NotContains(t, err.Error(), "strconv") } @@ -246,7 +246,7 @@ func TestPromptForLAGPortDetails_InvalidTerm(t *testing.T) { _, err := promptForLAGPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid term") + assert.Contains(t, err.Error(), "Invalid term") assert.NotContains(t, err.Error(), "strconv") } @@ -258,6 +258,6 @@ func TestPromptForLAGPortDetails_InvalidLocationID(t *testing.T) { _, err := promptForLAGPortDetails(true) assert.Error(t, err) - assert.Contains(t, err.Error(), "invalid location ID") + assert.Contains(t, err.Error(), "Invalid location ID") assert.NotContains(t, err.Error(), "strconv") } diff --git a/internal/commands/users/users_actions_test.go b/internal/commands/users/users_actions_test.go index 22dd5aea..36540880 100644 --- a/internal/commands/users/users_actions_test.go +++ b/internal/commands/users/users_actions_test.go @@ -114,7 +114,7 @@ func TestGetUser(t *testing.T) { name: "invalid employee ID", args: []string{"abc"}, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, { name: "nil user", @@ -368,7 +368,7 @@ func TestUpdateUser(t *testing.T) { name: "invalid employee ID", args: []string{"abc"}, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, } @@ -478,7 +478,7 @@ func TestDeleteUser(t *testing.T) { args: []string{"abc"}, force: true, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, } @@ -572,7 +572,7 @@ func TestDeactivateUser(t *testing.T) { args: []string{"abc"}, force: true, setupMock: func(m *MockUserManagementService) {}, - expectedError: "invalid employee ID", + expectedError: "Invalid employee ID", }, } diff --git a/internal/commands/vxc/vxc_actions.go b/internal/commands/vxc/vxc_actions.go index 981a8be1..01027453 100644 --- a/internal/commands/vxc/vxc_actions.go +++ b/internal/commands/vxc/vxc_actions.go @@ -276,6 +276,11 @@ func BuyVXC(cmd *cobra.Command, args []string, noColor bool) error { return err } + yes, err := utils.RequireYesForJSONBuy(cmd) + if err != nil { + return err + } + noWait, _ := cmd.Flags().GetBool("no-wait") // Only the order submission is wrapped in WithOrderOnceRetry below, so the SDK // must not also poll for provisioning: a 429 raised during polling would @@ -292,10 +297,7 @@ func BuyVXC(cmd *cobra.Command, args []string, noColor bool) error { return err } - jsonStr, _ := cmd.Flags().GetString("json") - jsonFile, _ := cmd.Flags().GetString("json-file") - yes, _ := cmd.Flags().GetBool("yes") - if !yes && jsonStr == "" && jsonFile == "" { + if !yes { details := []utils.BuyConfirmDetail{ {Key: "Name", Value: req.VXCName}, {Key: "Term", Value: fmt.Sprintf("%d months", req.Term)}, diff --git a/internal/commands/vxc/vxc_actions_test.go b/internal/commands/vxc/vxc_actions_test.go index 608b40e6..94ccaa4e 100644 --- a/internal/commands/vxc/vxc_actions_test.go +++ b/internal/commands/vxc/vxc_actions_test.go @@ -79,6 +79,7 @@ func TestBuyVXC_NilResponse(t *testing.T) { testutil.SetFlags(t, cmd, map[string]string{ "json": `{"portUid":"port-aaa-111","vxcName":"JSON VXC","rateLimit":500,"term":12,"bEndConfiguration":{"productUID":"port-bbb-222"}}`, + "yes": "true", }) var err error @@ -2548,11 +2549,17 @@ func TestBuyVXC_Confirmation(t *testing.T) { expectedContains: "vxc-uid-123", }, { - name: "json input skips confirmation", + name: "json input with yes skips confirmation", jsonInput: `{"portUid":"dcc-12345","vxcName":"JSON VXC","rateLimit":500,"term":12,"aEndConfiguration":{"vlan":100},"bEndConfiguration":{"productUID":"dcc-67890","vlan":200}}`, + yesFlag: true, expectPromptCalled: false, expectedContains: "vxc-uid-123", }, + { + name: "json input without yes is a usage error", + jsonInput: `{"portUid":"dcc-12345","vxcName":"JSON VXC","rateLimit":500,"term":12,"aEndConfiguration":{"vlan":100},"bEndConfiguration":{"productUID":"dcc-67890","vlan":200}}`, + expectedError: "--yes is required to confirm a purchase when using --json or --json-file", + }, } for _, tt := range tests { diff --git a/internal/utils/utils.go b/internal/utils/utils.go index 6b65c13e..7970524c 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -337,6 +337,20 @@ func WrapOutputFormatRunE(fn func(cmd *cobra.Command, args []string, noColor boo } } +// RequireYesForJSONBuy enforces that a JSON-mode purchase (--json or +// --json-file) also passes --yes, since JSON mode has no interactive +// confirmation prompt to fall back on. Returns the resolved --yes value so +// callers can reuse it for their own confirmation-prompt branch. +func RequireYesForJSONBuy(cmd *cobra.Command) (bool, error) { + jsonStr, _ := cmd.Flags().GetString("json") + jsonFile, _ := cmd.Flags().GetString("json-file") + yes, _ := cmd.Flags().GetBool("yes") + if !yes && (jsonStr != "" || jsonFile != "") { + return false, exitcodes.NewUsageError(fmt.Errorf("--yes is required to confirm a purchase when using --json or --json-file")) + } + return yes, nil +} + // classifyError inspects an error message to determine the appropriate exit code. func classifyError(err error) int { // Preserve exit codes already set by action functions diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index f3004aa0..f43ebb46 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -481,6 +481,56 @@ func TestWrapOutputFormatRunE(t *testing.T) { }) } +func TestRequireYesForJSONBuy(t *testing.T) { + newCmd := func() *cobra.Command { + cmd := &cobra.Command{Use: "buy"} + cmd.Flags().String("json", "", "") + cmd.Flags().String("json-file", "", "") + cmd.Flags().Bool("yes", false, "") + return cmd + } + + t.Run("json without yes is a usage error", func(t *testing.T) { + cmd := newCmd() + require.NoError(t, cmd.Flags().Set("json", `{"name":"test"}`)) + + yes, err := RequireYesForJSONBuy(cmd) + assert.False(t, yes) + require.Error(t, err) + assert.Contains(t, err.Error(), "--yes is required to confirm a purchase when using --json or --json-file") + var cliErr *exitcodes.CLIError + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, exitcodes.Usage, cliErr.Code) + }) + + t.Run("json-file without yes is a usage error", func(t *testing.T) { + cmd := newCmd() + require.NoError(t, cmd.Flags().Set("json-file", "./config.json")) + + yes, err := RequireYesForJSONBuy(cmd) + assert.False(t, yes) + require.Error(t, err) + }) + + t.Run("json with yes succeeds", func(t *testing.T) { + cmd := newCmd() + require.NoError(t, cmd.Flags().Set("json", `{"name":"test"}`)) + require.NoError(t, cmd.Flags().Set("yes", "true")) + + yes, err := RequireYesForJSONBuy(cmd) + assert.True(t, yes) + assert.NoError(t, err) + }) + + t.Run("no json and no yes succeeds, leaving confirmation to the caller", func(t *testing.T) { + cmd := newCmd() + + yes, err := RequireYesForJSONBuy(cmd) + assert.False(t, yes) + assert.NoError(t, err) + }) +} + func TestClassifyError(t *testing.T) { tests := []struct { name string diff --git a/internal/validation/common_test.go b/internal/validation/common_test.go index bbab5d47..20e754ed 100644 --- a/internal/validation/common_test.go +++ b/internal/validation/common_test.go @@ -229,7 +229,7 @@ func TestValidateMACAddress(t *testing.T) { {"Valid colon-separated uppercase", "AA:BB:CC:DD:EE:FF", false, ""}, {"Valid colon-separated mixed case", "aA:bB:cC:dD:eE:fF", false, ""}, {"Valid hyphen-separated", "00-11-22-33-44-55", false, ""}, - {"Invalid empty", "", true, "Invalid MAC address: - cannot be empty"}, + {"Invalid empty", "", true, "Invalid MAC address: \"\" - cannot be empty"}, {"Invalid too short", "00:11:22:33:44", true, "Invalid MAC address: 00:11:22:33:44 - must be a valid MAC address (e.g. 00:11:22:33:44:55)"}, {"Invalid too long", "00:11:22:33:44:55:66", true, "Invalid MAC address: 00:11:22:33:44:55:66 - must be a valid MAC address (e.g. 00:11:22:33:44:55)"}, {"Invalid EUI-64", "00:11:22:33:44:55:66:77", true, "Invalid MAC address: 00:11:22:33:44:55:66:77 - must be a 6-byte (EUI-48) MAC address"}, @@ -264,7 +264,7 @@ func TestValidateMVEProductSize(t *testing.T) { {"Valid LARGE", "LARGE", false, ""}, {"Invalid lowercase", "small", true, fmt.Sprintf("Invalid product size: small - must be one of: %v", ValidMVEProductSizes)}, {"Invalid value", "XLARGE", true, fmt.Sprintf("Invalid product size: XLARGE - must be one of: %v", ValidMVEProductSizes)}, - {"Empty value", "", true, fmt.Sprintf("Invalid product size: - must be one of: %v", ValidMVEProductSizes)}, + {"Empty value", "", true, fmt.Sprintf("Invalid product size: \"\" - must be one of: %v", ValidMVEProductSizes)}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/validation/errors.go b/internal/validation/errors.go index 8c69bfa3..248c91f4 100644 --- a/internal/validation/errors.go +++ b/internal/validation/errors.go @@ -1,6 +1,9 @@ package validation -import "fmt" +import ( + "fmt" + "strings" +) // ValidationError represents an error that occurs during validation of user input. // It contains information about the field being validated, its value, and the reason for validation failure. @@ -22,5 +25,10 @@ func NewValidationError(field string, value interface{}, reason string) *Validat // Error implements the error interface for ValidationError. // Returns a formatted string with field name, value, and reason for the validation error. func (e *ValidationError) Error() string { + // An empty or whitespace-only string value renders unambiguously when + // quoted; every other value keeps the plain %v rendering. + if s, ok := e.Value.(string); ok && strings.TrimSpace(s) == "" { + return fmt.Sprintf("Invalid %s: %q - %s", e.Field, s, e.Reason) + } return fmt.Sprintf("Invalid %s: %v - %s", e.Field, e.Value, e.Reason) } diff --git a/internal/validation/errors_test.go b/internal/validation/errors_test.go index a0ff0554..c174a44a 100644 --- a/internal/validation/errors_test.go +++ b/internal/validation/errors_test.go @@ -23,7 +23,7 @@ func TestValidationErrorError(t *testing.T) { }{ {"int value", "rate limit", 0, "must be positive", "Invalid rate limit: 0 - must be positive"}, {"string value", "name", "x", "too short", "Invalid name: x - too short"}, - {"empty string value", "name", "", "cannot be empty", "Invalid name: - cannot be empty"}, + {"empty string value", "name", "", "cannot be empty", "Invalid name: \"\" - cannot be empty"}, {"nil value", "peer ASN", nil, "is required", "Invalid peer ASN: - is required"}, {"bool value", "enabled", true, "bad", "Invalid enabled: true - bad"}, {"slice value", "terms", []int{1, 12}, "bad", "Invalid terms: [1 12] - bad"}, diff --git a/internal/validation/mcr_test.go b/internal/validation/mcr_test.go index 43d86b71..205f980a 100644 --- a/internal/validation/mcr_test.go +++ b/internal/validation/mcr_test.go @@ -68,7 +68,7 @@ func TestValidateMCRRequest(t *testing.T) { LocationID: 100, }, wantErr: true, - errText: "Invalid MCR name: - cannot be empty", // Use ValidationError format + errText: "Invalid MCR name: \"\" - cannot be empty", // Use ValidationError format }, { name: "Invalid term", @@ -229,7 +229,7 @@ func TestValidatePrefixFilterListRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid description: - cannot be empty", + errText: "Invalid description: \"\" - cannot be empty", }, { name: "Invalid address family", @@ -259,7 +259,7 @@ func TestValidatePrefixFilterListRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid address family: - cannot be empty", + errText: "Invalid address family: \"\" - cannot be empty", }, { name: "Empty entries", diff --git a/internal/validation/mve_test.go b/internal/validation/mve_test.go index d83754a8..ed35d421 100644 --- a/internal/validation/mve_test.go +++ b/internal/validation/mve_test.go @@ -30,7 +30,7 @@ func TestValidateMVERequest(t *testing.T) { term: 12, locationID: 123, wantErr: true, - errText: "Invalid MVE name: - cannot be empty", + errText: "Invalid MVE name: \"\" - cannot be empty", }, { name: "Invalid term", @@ -105,7 +105,7 @@ func TestValidateMVEVendor(t *testing.T) { name: "Empty vendor", vendor: "", wantErr: true, - errText: fmt.Sprintf("Invalid MVE vendor: - must be one of: %v", ValidMVEVendors), // Adjusted for empty value check if ValidateStringOneOf handles it + errText: fmt.Sprintf("Invalid MVE vendor: \"\" - must be one of: %v", ValidMVEVendors), // Adjusted for empty value check if ValidateStringOneOf handles it }, } @@ -176,7 +176,7 @@ func TestValidateMVENetworkInterfaces(t *testing.T) { {Description: ""}, }, wantErr: true, - errText: "Invalid network interface 2: - description cannot be empty", // Adjusted expected error + errText: "Invalid network interface 2: \"\" - description cannot be empty", // Adjusted expected error }, } @@ -249,7 +249,7 @@ func TestValidateMVENetworkInterfacesTyped(t *testing.T) { {Description: ""}, }, wantErr: true, - errText: "Invalid network interface 2: - description cannot be empty", + errText: "Invalid network interface 2: \"\" - description cannot be empty", }, } @@ -308,7 +308,7 @@ func TestValidateBuyMVERequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid MVE name: - cannot be empty", + errText: "Invalid MVE name: \"\" - cannot be empty", }, { name: "Missing location", @@ -477,7 +477,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ProductSize: "MEDIUM", }, wantErr: true, - errText: "Invalid SSH public key: - cannot be empty", + errText: "Invalid SSH public key: \"\" - cannot be empty", }, // aruba - valid { @@ -503,7 +503,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SystemTag: "test-tag", }, wantErr: true, - errText: "Invalid account name: - cannot be empty", + errText: "Invalid account name: \"\" - cannot be empty", }, // aviatrix - valid { @@ -525,7 +525,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ProductSize: "MEDIUM", }, wantErr: true, - errText: "Invalid cloud init: - cannot be empty", + errText: "Invalid cloud init: \"\" - cannot be empty", }, // cisco - valid { @@ -551,7 +551,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ManageLocally: true, }, wantErr: true, - errText: "Invalid admin SSH public key: - cannot be empty", + errText: "Invalid admin SSH public key: \"\" - cannot be empty", }, // fortinet - valid { @@ -577,7 +577,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SSHPublicKey: "ssh-rsa AAAA...", }, wantErr: true, - errText: "Invalid license data: - cannot be empty", + errText: "Invalid license data: \"\" - cannot be empty", }, // paloalto - valid { @@ -603,7 +603,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { LicenseData: "license-data", }, wantErr: true, - errText: "Invalid admin password / admin password hash: - at least one of admin password or admin password hash must be provided", + errText: "Invalid admin password / admin password hash: \"\" - at least one of admin password or admin password hash must be provided", }, // paloalto - valid with plaintext admin password only { @@ -631,7 +631,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { LicenseData: "license-data", }, wantErr: true, - errText: "Invalid admin password / admin password hash: - only one of admin password or admin password hash may be provided, not both", + errText: "Invalid admin password / admin password hash: \"\" - only one of admin password or admin password hash may be provided, not both", }, // prisma - valid { @@ -655,7 +655,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SecretKey: "secret-key", }, wantErr: true, - errText: "Invalid ION key: - cannot be empty", + errText: "Invalid ION key: \"\" - cannot be empty", }, // versa - valid { @@ -685,7 +685,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { SerialNumber: "SN123456", }, wantErr: true, - errText: "Invalid director address: - cannot be empty", + errText: "Invalid director address: \"\" - cannot be empty", }, // vmware - valid { @@ -713,7 +713,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { VcoActivationCode: "activation-code", }, wantErr: true, - errText: "Invalid VCO address: - cannot be empty", + errText: "Invalid VCO address: \"\" - cannot be empty", }, // meraki - valid { @@ -735,7 +735,7 @@ func TestValidateMVEVendorConfig_AllVendors(t *testing.T) { ProductSize: "MEDIUM", }, wantErr: true, - errText: "Invalid token: - cannot be empty", + errText: "Invalid token: \"\" - cannot be empty", }, } @@ -784,7 +784,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { // SSHPublicKey missing (zero value "") }, wantErr: true, - errText: "Invalid SSH public key: - cannot be empty", // Adjusted error + errText: "Invalid SSH public key: \"\" - cannot be empty", // Adjusted error }, { name: "Valid Cisco config", @@ -813,7 +813,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { ManageLocally: false, // Requires FMC fields (zero value "") }, wantErr: true, - errText: "Invalid FMC IP address: - cannot be empty when not managing locally", // Adjusted error + errText: "Invalid FMC IP address: \"\" - cannot be empty when not managing locally", // Adjusted error }, { name: "Invalid product size", @@ -839,7 +839,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { ManageLocally: true, // Avoid FMC errors }, wantErr: true, - errText: "Invalid SSH public key: - cannot be empty", // Updated based on actual test failure output + errText: "Invalid SSH public key: \"\" - cannot be empty", // Updated based on actual test failure output }, { name: "Missing image ID (zero value)", // Renamed test @@ -861,7 +861,7 @@ func TestValidateMVEVendorConfig(t *testing.T) { ManageLocally: true, // Avoid FMC errors }, wantErr: true, - errText: fmt.Sprintf("Invalid product size: - must be one of: %v", ValidMVEProductSizes), // Updated error message prefix and adjusted for empty string check + errText: fmt.Sprintf("Invalid product size: \"\" - must be one of: %v", ValidMVEProductSizes), // Updated error message prefix and adjusted for empty string check }, } diff --git a/internal/validation/parse.go b/internal/validation/parse.go index 3dc3ac21..7ef24b5e 100644 --- a/internal/validation/parse.go +++ b/internal/validation/parse.go @@ -1,21 +1,17 @@ package validation import ( - "fmt" "strconv" ) // ParseInt converts a user-supplied string into an int. On failure it returns a -// friendly error naming the field and the offending value, instead of leaking -// strconv internals like `strconv.Atoi: parsing "x": invalid syntax`. -// -// The message keeps the lowercase "invalid " prefix so exit-code -// classification still tags it as a usage error. For ID arguments the field -// should contain "ID" (e.g. "location ID") so that classification holds. +// friendly, typed *ValidationError naming the field and the offending value, +// instead of leaking strconv internals like `strconv.Atoi: parsing "x": invalid +// syntax` or relying on exit-code classification matching the message text. func ParseInt(field, value string) (int, error) { n, err := strconv.Atoi(value) if err != nil { - return 0, fmt.Errorf("invalid %s: %q is not a valid whole number", field, value) + return 0, NewValidationError(field, value, "is not a valid whole number") } return n, nil } diff --git a/internal/validation/parse_test.go b/internal/validation/parse_test.go index 88e1a25f..8142db46 100644 --- a/internal/validation/parse_test.go +++ b/internal/validation/parse_test.go @@ -1,10 +1,11 @@ package validation import ( - "strings" + "errors" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParseInt(t *testing.T) { @@ -25,21 +26,21 @@ func TestParseInt(t *testing.T) { value: "bad-location", wantErr: true, // Friendly message: names the field and value, no strconv internals. - errSubstr: []string{"invalid location ID", "bad-location"}, + errSubstr: []string{"location ID", "bad-location", "not a valid whole number"}, }, { name: "empty string", field: "employee ID", value: "", wantErr: true, - errSubstr: []string{"invalid employee ID"}, + errSubstr: []string{"employee ID", "not a valid whole number"}, }, { name: "float string", field: "term", value: "1.5", wantErr: true, - errSubstr: []string{"invalid term"}, + errSubstr: []string{"term", "not a valid whole number"}, }, } @@ -61,11 +62,12 @@ func TestParseInt(t *testing.T) { } } -// classifyError keys off a lowercase "invalid" + "ID" substring, so the helper -// must keep that wording for ID fields to preserve the Usage exit code. -func TestParseIntPreservesUsageClassification(t *testing.T) { +// TestParseIntReturnsTypedValidationError verifies ParseInt's failure is a +// typed *ValidationError, which is what actually drives the Usage exit code +// in classifyError (not a substring match against the message text). +func TestParseIntReturnsTypedValidationError(t *testing.T) { _, err := ParseInt("location ID", "abc") - assert.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "invalid")) - assert.True(t, strings.Contains(err.Error(), "ID")) + require.Error(t, err) + var validationErr *ValidationError + assert.True(t, errors.As(err, &validationErr), "expected a typed *ValidationError, got %T: %v", err, err) } diff --git a/internal/validation/port_test.go b/internal/validation/port_test.go index a8a18feb..1cc99ccd 100644 --- a/internal/validation/port_test.go +++ b/internal/validation/port_test.go @@ -34,7 +34,7 @@ func TestValidatePortRequest(t *testing.T) { portSpeed: 10000, locationID: 100, wantErr: true, - errText: "Invalid port name: - cannot be empty", + errText: "Invalid port name: \"\" - cannot be empty", }, { name: "Invalid term", @@ -146,7 +146,7 @@ func TestValidateLAGPortRequest(t *testing.T) { Term: 12, }, wantErr: true, - errText: "Invalid port name: - cannot be empty", + errText: "Invalid port name: \"\" - cannot be empty", }, { name: "Invalid port speed for LAG", diff --git a/internal/validation/vxc_test.go b/internal/validation/vxc_test.go index d3220fd6..a38c4e09 100644 --- a/internal/validation/vxc_test.go +++ b/internal/validation/vxc_test.go @@ -345,7 +345,7 @@ func TestValidateIPsecTunnelConfig(t *testing.T) { PreSharedKey: "psk", }, wantErr: true, - errText: "Invalid vRouter interface [0] IPsec tunnel source IP address: - cannot be empty", + errText: "Invalid vRouter interface [0] IPsec tunnel source IP address: \"\" - cannot be empty", }, { name: "Source IP not IPv4", @@ -364,7 +364,7 @@ func TestValidateIPsecTunnelConfig(t *testing.T) { PreSharedKey: "psk", }, wantErr: true, - errText: "Invalid vRouter interface [0] IPsec tunnel destination IP address: - cannot be empty", + errText: "Invalid vRouter interface [0] IPsec tunnel destination IP address: \"\" - cannot be empty", }, { name: "Missing pre-shared key", @@ -683,7 +683,7 @@ func TestValidateVXCRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid VXC name: - cannot be empty", + errText: "Invalid VXC name: \"\" - cannot be empty", }, { name: "Invalid term", @@ -725,7 +725,7 @@ func TestValidateVXCRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid A-End UID (PortUID): - cannot be empty", + errText: "Invalid A-End UID (PortUID): \"\" - cannot be empty", }, { name: "Empty B-End UID without partner config", @@ -739,7 +739,7 @@ func TestValidateVXCRequest(t *testing.T) { }, }, wantErr: true, - errText: "Invalid B-End UID: - cannot be empty when no partner configuration is provided", + errText: "Invalid B-End UID: \"\" - cannot be empty when no partner configuration is provided", }, } @@ -818,7 +818,7 @@ func TestValidateAWSPartnerConfig(t *testing.T) { ownerAccount: "123456789012", asn: 65000, wantErr: true, - errText: "Invalid AWS connect type: - cannot be empty", + errText: "Invalid AWS connect type: \"\" - cannot be empty", }, { connectType: "INVALID", @@ -833,7 +833,7 @@ func TestValidateAWSPartnerConfig(t *testing.T) { ownerAccount: "", asn: 65000, wantErr: true, - errText: "Invalid AWS owner account: - cannot be empty", + errText: "Invalid AWS owner account: \"\" - cannot be empty", }, { name: "Invalid customer IP CIDR", @@ -951,7 +951,7 @@ func TestValidateGooglePartnerConfig(t *testing.T) { errText string }{ {"Valid Google config", "google-pairing-key", false, ""}, - {"Empty pairing key", "", true, "Invalid Google pairing key: - cannot be empty"}, + {"Empty pairing key", "", true, "Invalid Google pairing key: \"\" - cannot be empty"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -979,7 +979,7 @@ func TestValidateOraclePartnerConfig(t *testing.T) { errText string }{ {"Valid Oracle config", "ocid1.virtualcircuit.oc1..example", false, ""}, - {"Empty virtual circuit ID", "", true, "Invalid Oracle virtual circuit ID: - cannot be empty"}, + {"Empty virtual circuit ID", "", true, "Invalid Oracle virtual circuit ID: \"\" - cannot be empty"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1025,7 +1025,7 @@ func TestValidateIBMPartnerConfig(t *testing.T) { name: "Empty account ID", accountID: "", wantErr: true, - errText: "Invalid IBM account ID: - cannot be empty", + errText: "Invalid IBM account ID: \"\" - cannot be empty", }, { name: "Account ID too short", @@ -1179,7 +1179,7 @@ func TestValidateVXCPartnerConfig(t *testing.T) { name: "Missing config details (handled by specific validators)", config: &megaport.VXCPartnerConfigAWS{}, // Empty AWS config wantErr: true, - errText: "Invalid AWS connect type: - cannot be empty", // Error from ValidateAWSPartnerConfig + errText: "Invalid AWS connect type: \"\" - cannot be empty", // Error from ValidateAWSPartnerConfig }, { name: "Invalid AWS config details", @@ -1188,7 +1188,7 @@ func TestValidateVXCPartnerConfig(t *testing.T) { ASN: 65000, }, wantErr: true, - errText: "Invalid AWS connect type: - cannot be empty", + errText: "Invalid AWS connect type: \"\" - cannot be empty", }, { name: "Invalid Azure config details", @@ -1196,7 +1196,7 @@ func TestValidateVXCPartnerConfig(t *testing.T) { ServiceKey: "", // Invalid service key }, wantErr: true, - errText: "Invalid Azure service key: - cannot be empty", + errText: "Invalid Azure service key: \"\" - cannot be empty", }, { name: "Invalid vRouter config details", @@ -1339,7 +1339,7 @@ func TestValidateAzurePartnerConfig(t *testing.T) { ServiceKey: "", }, wantErr: true, - errText: "Invalid Azure service key: - cannot be empty", + errText: "Invalid Azure service key: \"\" - cannot be empty", }, { name: "Nil config",