From 539b370e2c1ee25c5c4a96da767184a7ab56be3a Mon Sep 17 00:00:00 2001 From: Alexandre Bergere Date: Wed, 14 Oct 2026 00:00:52 +0200 Subject: [PATCH 1/6] fix: print YAML output as the document JSON output prints YAML is the default output of every command that returns one resource: `get`, `create`, `usage get`, `usage report`, `tokens create`. It was not usable there. The API's types describe themselves in JSON only, through json tags and MarshalJSON methods, and the YAML encoder reads neither: - a field was named after the Go one, lowercased: `externalcustomerid`, `createdat`, `limitcapexceededoveragepercent`; - a typed value printed as an empty mapping, so `licenses entitlements get` and `instances usage get` showed `value: {}` and `limit: {}`; - every absent optional field was printed, as `null`. YAML output is now built from the JSON encoding of the value: the same keys, in the same order, with the same values. A number is carried over as JSON writes it, and a string that would read as another type (a date, "true", "1024") is quoted so that it stays a string. The CLI's own documents (`doctor`, `config view`, the message of a mutation) already carried matching json and yaml tags and print as before. Signed-off-by: Alexandre Bergere Co-Authored-By: Claude Opus 5.5 --- internal/cmd/common_output_test.go | 88 +++++++++++++++++ internal/output/output.go | 6 +- internal/output/yaml.go | 69 +++++++++++++ internal/output/yaml_test.go | 150 +++++++++++++++++++++++++++++ 4 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 internal/output/yaml.go create mode 100644 internal/output/yaml_test.go diff --git a/internal/cmd/common_output_test.go b/internal/cmd/common_output_test.go index ed1f6f9..bdb59fe 100644 --- a/internal/cmd/common_output_test.go +++ b/internal/cmd/common_output_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "reflect" "strings" "testing" @@ -122,6 +123,93 @@ func TestWriteMessageFallsBackToABareLineForNonStructuredFormats(t *testing.T) { } } +// TestYAMLOutputCarriesTheDocumentJSONOutputCarries pins YAML, the default output of every +// command that returns one resource, to the document --output json prints. The API's types +// describe themselves in JSON only, so YAML used to name their fields after the Go ones +// (externalcustomerid) and to print a typed value -- a grant's, a usage counter's -- as an +// empty mapping. +func TestYAMLOutputCarriesTheDocumentJSONOutputCarries(t *testing.T) { + const usagePath = "/instances/sakura-tokyo-prod/entitlements/monthly-orders/usage" + api := givenRoutes(t, map[string]route{ + "GET /customers/sakura-tokyo": {status: http.StatusOK, body: `{ + "id": "cus_1", "name": "Sakura Tokyo", "slug": "sakura-tokyo", "externalCustomerId": "crm-1024", + "createdAt": "2026-10-01T09:00:00Z", "createdBy": {"id": "usr_1", "name": "Aiko"}}`}, + "GET /licenses/premium-v2/entitlements/monthly-orders": {status: http.StatusOK, body: `{ + "entitlementSlug": "monthly-orders", "entitlementType": "NUMBER", + "value": {"type": "number", "value": 1000}, "limitCapExceededOveragePercent": 20}`}, + "GET " + usagePath: {status: http.StatusOK, body: `{ + "entitlementId": "ent_1", "entitlementSlug": "monthly-orders", + "licenseId": "lic_1", "licenseSlug": "premium-v2", + "value": {"type": "number", "value": 730.5}, "limit": {"type": "number", "value": 1000}}`}, + }) + + tests := []struct { + name string + args []string + want string + }{ + { + name: "a resource keeps the API's field names", + args: []string{"customers", "get", "sakura-tokyo"}, + want: "externalCustomerId: crm-1024", + }, + { + name: "a grant keeps its typed value", + args: []string{"licenses", "entitlements", "get", "premium-v2", "monthly-orders"}, + want: "limitCapExceededOveragePercent: 20", + }, + { + name: "a usage counter keeps its value and its limit", + args: []string{"instances", "usage", "get", "sakura-tokyo-prod", "monthly-orders"}, + want: "value: 730.5", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + givenIsolatedConfig(t) + run := func(extra ...string) string { + return runSuccessfully(t, append(append([]string{"--base-url", api.URL}, extra...), test.args...)...) + } + + var want any + if err := json.Unmarshal([]byte(run("--output", "json")), &want); err != nil { + t.Fatalf("decode json output error = %v", err) + } + + // YAML is what the command prints when no format is selected, too. + for _, selected := range [][]string{nil, {"--output", "yaml"}} { + stdout := run(selected...) + expectOutputContains(t, stdout, test.want) + + var decoded any + if err := yaml.Unmarshal([]byte(stdout), &decoded); err != nil { + t.Fatalf("decode yaml output error = %v, stdout = %q", err, stdout) + } + if got := throughJSON(t, decoded); !reflect.DeepEqual(got, want) { + t.Fatalf("yaml output = %v, want the json document %v", got, want) + } + } + }) + } +} + +// throughJSON re-reads a decoded YAML document the way JSON types it, so that it compares +// with a decoded JSON document: every number a float64. +func throughJSON(t *testing.T, value any) any { + t.Helper() + + raw, err := json.Marshal(value) + if err != nil { + t.Fatalf("encode decoded yaml error = %v", err) + } + var out any + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatalf("decode re-encoded yaml error = %v", err) + } + return out +} + // givenCustomerAPI serves the customer and token endpoints the rendering tests exercise. func givenCustomerAPI(t *testing.T) *httptest.Server { t.Helper() diff --git a/internal/output/output.go b/internal/output/output.go index 9f54bce..ad0cac3 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -82,7 +82,11 @@ func writeJSON(w io.Writer, value any) error { } func writeYAML(w io.Writer, value any) error { - data, err := yaml.Marshal(value) + document, err := jsonDocument(value) + if err != nil { + return err + } + data, err := yaml.Marshal(document) if err != nil { return err } diff --git a/internal/output/yaml.go b/internal/output/yaml.go new file mode 100644 index 0000000..2a59e1d --- /dev/null +++ b/internal/output/yaml.go @@ -0,0 +1,69 @@ +package output + +import ( + "bytes" + "encoding/json" + + "gopkg.in/yaml.v3" +) + +// jsonDocument returns value as the YAML tree of its JSON encoding. The API's types describe +// themselves in JSON only: the names of their fields, the fields they leave out and the +// typed values they hold are spelled by json tags and MarshalJSON methods, none of which a +// YAML encoder reads. Left to itself it names a field after the Go one (externalcustomerid), +// prints every absent field as null and a typed value as an empty mapping. Going through +// JSON makes --output yaml the document --output json prints, key for key and in the same +// order. +func jsonDocument(value any) (*yaml.Node, error) { + raw, err := json.Marshal(value) + if err != nil { + return nil, err + } + + decoder := json.NewDecoder(bytes.NewReader(raw)) + // A number is carried over as written, never through a float64 that would round a + // large integer or respell a decimal. + decoder.UseNumber() + + return jsonNode(decoder) +} + +// jsonNode reads the next JSON value from decoder as a YAML node. +func jsonNode(decoder *json.Decoder) (*yaml.Node, error) { + token, err := decoder.Token() + if err != nil { + return nil, err + } + + switch typed := token.(type) { + case json.Delim: + node := &yaml.Node{Kind: yaml.MappingNode} + if typed == '[' { + node.Kind = yaml.SequenceNode + } + // An object's tokens alternate between a key and its value, which is the order a + // mapping node holds them in, so one loop reads an object and an array alike. + for decoder.More() { + child, err := jsonNode(decoder) + if err != nil { + return nil, err + } + node.Content = append(node.Content, child) + } + // The closing delimiter. + if _, err := decoder.Token(); err != nil { + return nil, err + } + return node, nil + case json.Number: + return &yaml.Node{Kind: yaml.ScalarNode, Value: typed.String()}, nil + default: + // A string, a boolean or null: the encoder knows how each is written, and which + // strings need quoting to stay strings. + node := &yaml.Node{} + if err := node.Encode(typed); err != nil { + return nil, err + } + return node, nil + } +} diff --git a/internal/output/yaml_test.go b/internal/output/yaml_test.go new file mode 100644 index 0000000..12317aa --- /dev/null +++ b/internal/output/yaml_test.go @@ -0,0 +1,150 @@ +package output + +import ( + "bytes" + "encoding/json" + "math" + "reflect" + "testing" + "time" + + "gopkg.in/yaml.v3" +) + +// apiResource stands for a type of the API client. It describes itself in JSON only: json +// tags, an optional field that is left out when absent, and a typed value that knows how to +// write itself as JSON and nothing else. +type apiResource struct { + ID string `json:"id"` + ExternalCustomerID *string `json:"externalCustomerId,omitempty"` + Domain *string `json:"domain,omitempty"` + CreatedAt time.Time `json:"createdAt"` + Value typedValue `json:"value"` +} + +type typedValue struct { + union json.RawMessage +} + +func (v typedValue) MarshalJSON() ([]byte, error) { + return v.union, nil +} + +// TestWriteYAMLPrintsTheJSONDocument pins what YAML output is made of: the keys, the values +// and the key order of the JSON encoding. A YAML encoder reading the Go type instead names +// the fields externalcustomerid and createdat, prints the absent domain as null and the +// typed value as an empty mapping. +func TestWriteYAMLPrintsTheJSONDocument(t *testing.T) { + t.Parallel() + + externalID := "crm-1024" + resource := apiResource{ + ID: "cus_1", + ExternalCustomerID: &externalID, + CreatedAt: time.Date(2026, time.October, 1, 9, 0, 0, 0, time.UTC), + Value: typedValue{union: json.RawMessage(`{"type":"number","value":1000}`)}, + } + + var out bytes.Buffer + if err := Write(&out, FormatYAML, resource, Table{}); err != nil { + t.Fatalf("Write() error = %v", err) + } + + const want = `id: cus_1 +externalCustomerId: crm-1024 +createdAt: "2026-10-01T09:00:00Z" +value: + type: number + value: 1000 +` + if out.String() != want { + t.Fatalf("Write() output =\n%s\nwant\n%s", out.String(), want) + } +} + +// TestWriteYAMLKeepsEveryValueWhatItIsInJSON checks the values one kind at a time: read +// back, the YAML document is the JSON one. A string that reads as another type stays a +// string, and a number is printed as JSON writes it. +func TestWriteYAMLKeepsEveryValueWhatItIsInJSON(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value any + want string + }{ + {name: "a plain string", value: "acme", want: "acme\n"}, + {name: "a string that reads as a boolean", value: "true", want: "\"true\"\n"}, + {name: "a string that reads as a number", value: "1024", want: "\"1024\"\n"}, + {name: "a string that reads as null", value: "null", want: "\"null\"\n"}, + {name: "an empty string", value: "", want: "\"\"\n"}, + {name: "a string of several lines", value: "first\nsecond", want: "|-\n first\n second\n"}, + {name: "a boolean", value: false, want: "false\n"}, + {name: "null", value: nil, want: "null\n"}, + {name: "an integer", value: 1000, want: "1000\n"}, + {name: "a decimal", value: 730.5, want: "730.5\n"}, + {name: "an integer too large for a float", value: uint64(math.MaxUint64), want: "18446744073709551615\n"}, + {name: "an empty object", value: map[string]any{}, want: "{}\n"}, + {name: "an empty list", value: []string{}, want: "[]\n"}, + {name: "an absent list", value: []string(nil), want: "null\n"}, + { + name: "a list of objects", + value: []map[string]any{{"slug": "monthly-orders", "limit": -1}}, + want: "- limit: -1\n slug: monthly-orders\n", + }, + { + name: "an object holding a list", + value: map[string]any{"scopes": []string{"read:instances", "write:instances"}}, + want: "scopes:\n - read:instances\n - write:instances\n", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + var out bytes.Buffer + if err := Write(&out, FormatYAML, test.value, Table{}); err != nil { + t.Fatalf("Write() error = %v", err) + } + if out.String() != test.want { + t.Fatalf("Write() output = %q, want %q", out.String(), test.want) + } + + var fromYAML any + if err := yaml.Unmarshal(out.Bytes(), &fromYAML); err != nil { + t.Fatalf("yaml.Unmarshal() error = %v; output = %q", err, out.String()) + } + if got, want := asJSON(t, fromYAML), asJSON(t, test.value); !reflect.DeepEqual(got, want) { + t.Fatalf("read back, the yaml document is %v, want %v", got, want) + } + }) + } +} + +// TestWriteYAMLReportsAValueJSONCannotEncode keeps a value with no JSON form an error +// rather than an empty document. +func TestWriteYAMLReportsAValueJSONCannotEncode(t *testing.T) { + t.Parallel() + + var out bytes.Buffer + if err := Write(&out, FormatYAML, map[string]any{"ratio": math.NaN()}, Table{}); err == nil { + t.Fatalf("Write() error = nil, want one; output = %q", out.String()) + } +} + +// asJSON returns value as JSON types it, so that a document read from YAML compares with +// the value it was written from. +func asJSON(t *testing.T, value any) any { + t.Helper() + + raw, err := json.Marshal(value) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + var out any + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatalf("json.Unmarshal() error = %v", err) + } + return out +} From 492d56a0e01ac46db902c05a4a234f695d3ab9cc Mon Sep 17 00:00:00 2001 From: Alexandre Bergere Date: Wed, 14 Oct 2026 00:01:14 +0200 Subject: [PATCH 2/6] fix: read YAML input as the JSON it stands for A `--file` or `--payload` written in YAML lost fields without an error. The request types name their fields for JSON only, so the YAML decoder filled the fields whose key is a single word and silently dropped every other one: - `customers create --file customer.yaml` sent no `externalCustomerId`; - an instance file lost `customerId`, `licenseId` and both license dates; - a feature flag went out with `"default_variant": null` and an empty `event_name`; - a grant value (`type: number`, `value: 2000`) decoded to nothing. Each command still exited 0, or failed on a request it had emptied itself. A YAML document is now decoded as the JSON document it stands for, so it makes the request the same document written in JSON makes: the same field names, typed values included. Where the two formats part ways on what a value is, the text wins, as it did before for single-word keys: a bare `version: 1.0` is the text "1.0" in a text field, and a date stays the text it is in JSON instead of being respelled as a timestamp. A YAML key the request has no field for is now an error that names the key, before any request is sent. JSON input is unchanged and still skips such a key, which is what lets a resource printed by `--output json` be edited and written back with its read-only fields still in it. Signed-off-by: Alexandre Bergere Co-Authored-By: Claude Opus 5.5 --- internal/cmd/input_sources_test.go | 121 +++++++++++++++ internal/input/input.go | 33 +++- internal/input/input_test.go | 234 +++++++++++++++++++++++++++++ internal/input/yaml.go | 171 +++++++++++++++++++++ 4 files changed, 551 insertions(+), 8 deletions(-) create mode 100644 internal/input/yaml.go diff --git a/internal/cmd/input_sources_test.go b/internal/cmd/input_sources_test.go index a2e581a..669444c 100644 --- a/internal/cmd/input_sources_test.go +++ b/internal/cmd/input_sources_test.go @@ -59,6 +59,127 @@ func TestInputSourcesBuildTheRequestBody(t *testing.T) { } } +// TestYAMLInputSendsTheRequestItsJSONSends pins a YAML --file or --payload to the request +// the same document makes in JSON. The fields of a request are named for JSON only, so YAML +// used to fill the ones of a single word and lose the rest without an error: a customer +// went out without its externalCustomerId, a feature flag with a null default_variant, and +// a grant with no value at all. +func TestYAMLInputSendsTheRequestItsJSONSends(t *testing.T) { + tests := []struct { + name string + args []string + route string + yaml string + json string + want map[string]any + }{ + { + name: "a key of several words", + args: []string{"customers", "create"}, + route: "POST /customers", + yaml: "name: Sakura Tokyo\nexternalCustomerId: crm-1024\n", + json: `{"name": "Sakura Tokyo", "externalCustomerId": "crm-1024"}`, + want: map[string]any{"name": "Sakura Tokyo", "externalCustomerId": "crm-1024"}, + }, + { + name: "a version that reads as a number stays the text it is", + args: []string{"components", "create"}, + route: "POST /components", + yaml: "name: api\nversion: 1.0\npreviousComponentId: cmp_0\n", + json: `{"name": "api", "version": "1.0", "previousComponentId": "cmp_0"}`, + want: map[string]any{"name": "api", "version": "1.0", "previousComponentId": "cmp_0"}, + }, + { + name: "dates and metadata", + args: []string{"instances", "create"}, + route: "POST /instances", + yaml: "name: Sakura Tokyo Prod\ndescription: Production\ncustomerId: cus_1\nlicenseId: lic_1\n" + + "startLicenseDate: 2026-01-01T00:00:00Z\nendLicenseDate: 2027-01-01T00:00:00Z\n" + + "metadata:\n region: kanto\n seats: 40\n", + json: `{"name": "Sakura Tokyo Prod", "description": "Production", "customerId": "cus_1", "licenseId": "lic_1", + "startLicenseDate": "2026-01-01T00:00:00Z", "endLicenseDate": "2027-01-01T00:00:00Z", + "metadata": {"region": "kanto", "seats": 40}}`, + want: map[string]any{ + "name": "Sakura Tokyo Prod", "description": "Production", "customerId": "cus_1", "licenseId": "lic_1", + "startLicenseDate": "2026-01-01T00:00:00Z", "endLicenseDate": "2027-01-01T00:00:00Z", + "metadata": map[string]any{"region": "kanto", "seats": float64(40)}, + }, + }, + { + name: "a grant's typed value", + args: []string{"licenses", "entitlements", "associate", "standard", "monthly-orders"}, + route: "POST /licenses/standard/entitlements", + yaml: "type: number\nvalue: 2000\n", + json: `{"type": "number", "value": 2000}`, + want: map[string]any{"entitlementSlug": "monthly-orders", "value": numberGrant(2000)}, + }, + { + name: "a feature flag's snake_case keys", + args: []string{"feature-flags", "create"}, + route: "POST /feature-flags", + yaml: "name: Delivery tracking\ntype: boolean\nenabled: true\nevent_name: delivery-tracking-enabled\n" + + "default_variant:\n key: \"off\"\n value: false\n", + json: `{"name": "Delivery tracking", "type": "boolean", "enabled": true, "event_name": "delivery-tracking-enabled", + "default_variant": {"key": "off", "value": false}}`, + want: map[string]any{ + "name": "Delivery tracking", "type": "boolean", "enabled": true, "event_name": "delivery-tracking-enabled", + "default_variant": map[string]any{"key": "off", "value": false}, + "description": nil, "metadata": nil, "targetings": nil, "variants": nil, + }, + }, + } + + for _, test := range tests { + sources := []struct { + name string + args func(t *testing.T) []string + }{ + {name: "a json file", args: func(t *testing.T) []string { return []string{"--file", givenInputFile(t, "input.json", test.json)} }}, + {name: "a yaml file", args: func(t *testing.T) []string { return []string{"--file", givenInputFile(t, "input.yaml", test.yaml)} }}, + {name: "a yaml payload", args: func(*testing.T) []string { return []string{"--payload", test.yaml} }}, + } + for _, source := range sources { + t.Run(test.name+"/"+source.name, func(t *testing.T) { + // Every create answers with a body the command only prints: an empty object will do. + api := givenRoutes(t, map[string]route{test.route: {status: http.StatusCreated, body: `{}`}}) + givenIsolatedConfig(t) + + args := append([]string{"--base-url", api.URL}, test.args...) + runSuccessfully(t, append(args, source.args(t)...)...) + + expectCalls(t, api.calls(), test.route) + expectBody(t, api.calls()[0].body, test.want) + }) + } + } +} + +// TestYAMLInputRefusesAKeyTheRequestHasNoFieldFor is the other half: a key that reaches no +// field stops the command before the request, naming the key, instead of being left out of +// a request that then succeeds. +func TestYAMLInputRefusesAKeyTheRequestHasNoFieldFor(t *testing.T) { + const document = "name: Sakura Tokyo\nexternal_customer_id: crm-1024\n" + + for name, source := range map[string]func(t *testing.T) []string{ + "a yaml file": func(t *testing.T) []string { return []string{"--file", givenInputFile(t, "customer.yaml", document)} }, + "a yaml payload": func(*testing.T) []string { return []string{"--payload", document} }, + } { + t.Run(name, func(t *testing.T) { + api := givenRoutes(t, map[string]route{}) + + args := append([]string{"--base-url", api.URL, "customers", "create"}, source(t)...) + _, err := executeForExitCode(t, args...) + + if err == nil || !strings.Contains(err.Error(), `unknown field "external_customer_id"`) { + t.Fatalf("Execute() error = %v, want it to name the unknown key", err) + } + if calls := api.calls(); len(calls) != 0 { + t.Fatalf("the API was called %d times, want the CLI to stop before the request", len(calls)) + } + }) + } +} + // TestOptionalFieldsAreOmittedWhenTheirFlagIsUnset pins the rule optionalString exists for: // a field the caller never mentioned must not reach the API at all, because an empty string // would overwrite whatever the record already holds. diff --git a/internal/input/input.go b/internal/input/input.go index 15cc4f5..b4805f9 100644 --- a/internal/input/input.go +++ b/internal/input/input.go @@ -8,8 +8,6 @@ import ( "os" "path/filepath" "strings" - - "gopkg.in/yaml.v3" ) // LoadFile reads and decodes the JSON or YAML file at path into a value of type T. A path of "-" reads from stdin. @@ -57,27 +55,46 @@ func read(path string) ([]byte, error) { func decode[T any](data []byte, formatHint string) (T, error) { var zero T - var out T switch strings.ToLower(formatHint) { case ".json": + var out T if err := json.Unmarshal(data, &out); err != nil { return zero, fmt.Errorf("decode JSON input: %w", err) } return out, nil case ".yaml", ".yml": - if err := yaml.Unmarshal(data, &out); err != nil { + document, err := parseYAML(data) + if err != nil { + return zero, fmt.Errorf("decode YAML input: %w", err) + } + out, err := decodeYAML[T](document) + if err != nil { return zero, fmt.Errorf("decode YAML input: %w", err) } return out, nil } - if err := json.Unmarshal(data, &out); err == nil { - return out, nil - } - if err := yaml.Unmarshal(data, &out); err == nil { + var out T + jsonErr := json.Unmarshal(data, &out) + if jsonErr == nil { return out, nil } + // Not JSON, or JSON that does not fit the request. Either is tried as YAML, where a text + // field takes a bare number, and the second is reported as the JSON it is if that fails. + jsonValid := json.Valid(data) + if document, err := parseYAML(data); err == nil { + out, err := decodeYAML[T](document) + if err == nil { + return out, nil + } + if !jsonValid { + return zero, fmt.Errorf("decode YAML input: %w", err) + } + } + if jsonValid { + return zero, fmt.Errorf("decode JSON input: %w", jsonErr) + } return zero, fmt.Errorf("decode input: expected JSON or YAML") } diff --git a/internal/input/input_test.go b/internal/input/input_test.go index c93f286..a25ec9d 100644 --- a/internal/input/input_test.go +++ b/internal/input/input_test.go @@ -1,10 +1,13 @@ package input import ( + "encoding/json" "os" "path/filepath" + "reflect" "strings" "testing" + "time" ) type component struct { @@ -171,6 +174,237 @@ func TestLoadString(t *testing.T) { } } +// instanceInput stands for an input type of the API client. Its fields carry no tags: a key +// reaches a field the way JSON matches one, by name and whatever the case. +type instanceInput struct { + Name string + Version string + CustomerID string + DeploymentZoneID *string + IsDefault bool + SaleUnitFactor *float64 + StartLicenseDate time.Time + GroupSlugs []string + Metadata map[string]any +} + +// flagInput stands for a type the API client generates: its keys are spelled by json tags. +type flagInput struct { + EventName string `json:"event_name"` + DefaultVariant typedValue `json:"default_variant"` +} + +// typedValue stands for a value discriminated by its type field. It reads itself from JSON +// and from nothing else. +type typedValue struct { + union json.RawMessage +} + +func (v *typedValue) UnmarshalJSON(data []byte) error { + v.union = append(v.union[:0], data...) + return nil +} + +// TestYAMLDecodesToWhatTheEquivalentJSONDecodesTo pins a YAML document to the request the +// same document written in JSON makes. The types of the API name their fields for JSON +// only, so a YAML decoder reading them matched the keys of one word and dropped every other +// without a word -- externalCustomerId, default_variant -- and read a typed value as nothing. +func TestYAMLDecodesToWhatTheEquivalentJSONDecodesTo(t *testing.T) { + t.Parallel() + + const instanceYAML = ` +name: Sakura Tokyo Prod +customerId: cus_1 +deploymentZoneId: zone_1 +isDefault: true +saleUnitFactor: 10 +startLicenseDate: 2026-01-01T00:00:00Z +groupSlugs: [restaurant-operations, delivery] +metadata: + region: kanto + seats: 40 + ratio: 0.5 + pilot: false + owner: null + opened: 2026-01-01 + tags: [a, b] +` + const instanceJSON = `{ + "name": "Sakura Tokyo Prod", "customerId": "cus_1", "deploymentZoneId": "zone_1", "isDefault": true, + "saleUnitFactor": 10, "startLicenseDate": "2026-01-01T00:00:00Z", + "groupSlugs": ["restaurant-operations", "delivery"], + "metadata": {"region": "kanto", "seats": 40, "ratio": 0.5, "pilot": false, "owner": null, + "opened": "2026-01-01", "tags": ["a", "b"]} +}` + expectSameDecoding[instanceInput](t, "an input type, its keys of several words included", instanceYAML, instanceJSON) + + expectSameDecoding[flagInput](t, "a generated type and the typed value it holds", + "event_name: delivery-tracking\ndefault_variant:\n type: boolean\n value: false\n", + `{"event_name": "delivery-tracking", "default_variant": {"type": "boolean", "value": false}}`) + + expectSameDecoding[typedValue](t, "a typed value on its own", + "type: number\nvalue: 2000\n", `{"type": "number", "value": 2000}`) + + expectSameDecoding[map[string]any](t, "a free-form object", + "max: 10\n1024: kept\nnested:\n enabled: true\n", `{"max": 10, "1024": "kept", "nested": {"enabled": true}}`) +} + +// TestYAMLKeepsTextAsWritten covers the two places where YAML and JSON part ways on what a +// value is. A text field takes what was written: version 1.0 is the text 1.0, not the +// number 1, which JSON would refuse there. And a date stays the text it is in JSON instead +// of becoming a time and coming back respelled. +func TestYAMLKeepsTextAsWritten(t *testing.T) { + t.Parallel() + + got, err := LoadString[instanceInput]("name: 2026\nversion: 1.0\ncustomerId: 2026-08-01\nmetadata:\n since: 2026-08-01\n") + if err != nil { + t.Fatalf("LoadString() error = %v", err) + } + + want := instanceInput{ + Name: "2026", + Version: "1.0", + CustomerID: "2026-08-01", + Metadata: map[string]any{"since": "2026-08-01"}, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("LoadString() = %+v, want %+v", got, want) + } +} + +// TestYAMLRejectsWhatItCannotPlace keeps a YAML document from going out incomplete: a key +// the request has no field for is an error that names it, and so is a value of the wrong +// kind. Neither is dropped. +func TestYAMLRejectsWhatItCannotPlace(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + content string + wantErr string + }{ + { + name: "a key the request has no field for", + content: "name: api\nexternal_customer_id: crm-1024\n", + wantErr: `decode YAML input: unknown field "external_customer_id"`, + }, + { + name: "a value of the wrong kind", + content: "name: api\nisDefault: sometimes\n", + wantErr: "decode YAML input: cannot unmarshal string", + }, + { + name: "a key that is a list", + content: "name: api\nmetadata:\n [a, b]: c\n", + wantErr: "decode YAML input: yaml: invalid map key", + }, + { + name: "a key tagged as a number", + content: "name: api\nmetadata:\n !!int 5: c\n", + wantErr: "decode YAML input: a key must be text", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // By extension and by content alike: the reason is not lost to the sniffing. + _, fileErr := LoadFile[instanceInput](givenFile(t, "instance.yaml", test.content)) + _, stringErr := LoadString[instanceInput](test.content) + + for source, err := range map[string]error{"LoadFile": fileErr, "LoadString": stringErr} { + if err == nil || !strings.Contains(err.Error(), test.wantErr) { + t.Errorf("%s() error = %v, want it to contain %q", source, err, test.wantErr) + } + } + }) + } +} + +// TestJSONIgnoresAKeyTheRequestHasNoFieldFor records that JSON input is left as it was: the +// keys a request has no field for are skipped, which is what lets a resource printed by +// --output json be edited and written back with the fields only the API sets still in it. +func TestJSONIgnoresAKeyTheRequestHasNoFieldFor(t *testing.T) { + t.Parallel() + + got, err := LoadString[instanceInput](`{"id": "ins_1", "name": "Sakura Tokyo Prod", "createdAt": "2026-10-01T09:00:00Z"}`) + if err != nil { + t.Fatalf("LoadString() error = %v", err) + } + if want := (instanceInput{Name: "Sakura Tokyo Prod"}); !reflect.DeepEqual(got, want) { + t.Fatalf("LoadString() = %+v, want %+v", got, want) + } +} + +// TestAnEmptyYAMLDocumentDecodesToNothing keeps an empty file what it was: no field set, +// and no error of its own -- the command reports what the request is missing. +func TestAnEmptyYAMLDocumentDecodesToNothing(t *testing.T) { + t.Parallel() + + got, err := LoadFile[instanceInput](givenFile(t, "instance.yaml", "# nothing yet\n")) + if err != nil { + t.Fatalf("LoadFile() error = %v", err) + } + if !reflect.DeepEqual(got, instanceInput{}) { + t.Fatalf("LoadFile() = %+v, want the zero value", got) + } +} + +// expectSameDecoding decodes one document written both ways and fails unless the two +// requests are the same, and the YAML one is not empty. +func expectSameDecoding[T any](t *testing.T, name, yamlDocument, jsonDocument string) { + t.Helper() + + fromJSON, err := LoadFile[T](givenFile(t, "document.json", jsonDocument)) + if err != nil { + t.Fatalf("%s: LoadFile(json) error = %v", name, err) + } + fromYAML, err := LoadFile[T](givenFile(t, "document.yaml", yamlDocument)) + if err != nil { + t.Fatalf("%s: LoadFile(yaml) error = %v", name, err) + } + sniffed, err := LoadString[T](yamlDocument) + if err != nil { + t.Fatalf("%s: LoadString(yaml) error = %v", name, err) + } + + var zero T + if reflect.DeepEqual(fromJSON, zero) { + t.Fatalf("%s: the json document decoded to nothing", name) + } + want := normalized(t, fromJSON) + if !reflect.DeepEqual(normalized(t, fromYAML), want) || !reflect.DeepEqual(normalized(t, sniffed), want) { + t.Fatalf("%s: yaml decoded to %+v (by extension) and %+v (by content), want what json decodes to, %+v", + name, fromYAML, sniffed, fromJSON) + } +} + +// normalized returns value with the raw JSON a typedValue holds decoded, so that two +// spellings of one document compare equal. +func normalized(t *testing.T, value any) any { + t.Helper() + + switch typed := value.(type) { + case typedValue: + return decodedJSON(t, typed.union) + case flagInput: + return [2]any{typed.EventName, decodedJSON(t, typed.DefaultVariant.union)} + default: + return value + } +} + +func decodedJSON(t *testing.T, raw json.RawMessage) any { + t.Helper() + + var decoded any + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("json.Unmarshal(%q) error = %v", raw, err) + } + return decoded +} + func givenFile(t *testing.T, name, content string) string { t.Helper() diff --git a/internal/input/yaml.go b/internal/input/yaml.go new file mode 100644 index 0000000..7f8d6e0 --- /dev/null +++ b/internal/input/yaml.go @@ -0,0 +1,171 @@ +package input + +import ( + "bytes" + "encoding/json" + "errors" + "reflect" + "strings" + + "gopkg.in/yaml.v3" +) + +const ( + yamlMergeTag = "!!merge" + yamlNullTag = "!!null" + yamlStringTag = "!!str" + yamlTimestampTag = "!!timestamp" +) + +// parseYAML reads data as one YAML document. It fails on what is not YAML at all, and on +// nothing else: whether the document fits the request is decodeYAML's to say. +func parseYAML(data []byte) (*yaml.Node, error) { + var document yaml.Node + if err := yaml.Unmarshal(data, &document); err != nil { + return nil, err + } + return &document, nil +} + +// decodeYAML decodes document into a value of type T as the JSON document it stands for. +// +// The types of the API describe themselves in JSON only: the names of their fields come +// from json tags or from matching a key whatever its case, and a typed value reads itself +// through UnmarshalJSON. A YAML decoder reads none of that. It matched the keys of one word +// by accident and dropped every other without an error -- externalCustomerId, default_variant +// -- so the request went out incomplete. Decoding the JSON the document stands for gives a +// YAML file the request the same file written in JSON makes. +// +// A key the request has no field for is an error rather than a drop, which is the failure +// this replaces. +func decodeYAML[T any](document *yaml.Node) (T, error) { + var zero T + + // An empty document has no node to decode, and stands for null. + var generic any + if document.Kind != 0 { + keepText(document, reflect.TypeFor[T]()) + if err := document.Decode(&generic); err != nil { + return zero, err + } + } + + raw, err := json.Marshal(generic) + if err != nil { + var unsupported *json.UnsupportedTypeError + if errors.As(err, &unsupported) { + // The one thing a YAML document holds that JSON cannot: a mapping whose keys are + // not text. + return zero, errors.New("a key must be text") + } + return zero, asYAMLError(err) + } + + var out T + decoder := json.NewDecoder(bytes.NewReader(raw)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&out); err != nil { + return zero, asYAMLError(err) + } + return out, nil +} + +// asYAMLError reports a failure to read the JSON a YAML document stands for without naming +// JSON, which the operator did not write. +func asYAMLError(err error) error { + return errors.New(strings.TrimPrefix(err.Error(), "json: ")) +} + +// keepText marks as text the plain scalars of node that reading the document as JSON would +// otherwise change, so that each reaches the request as it was written: +// +// - a key, since the keys of a JSON object are text: 1024 is the key "1024"; +// - a date or a time, which JSON has no type for and carries as text: left alone it +// becomes a time and comes back respelled, 2026-08-01 as 2026-08-01T00:00:00Z; +// - a value bound for a text field: version: 1.0 is the text 1.0 there, not the number 1 +// that JSON refuses for it. +// +// bound is the Go type the node is decoded into, or nil when nothing is known of it. A +// quoted or explicitly tagged scalar already says what it is and is left alone. +func keepText(node *yaml.Node, bound reflect.Type) { + for bound != nil && bound.Kind() == reflect.Pointer { + bound = bound.Elem() + } + if bound != nil && reflect.PointerTo(bound).Implements(reflect.TypeFor[json.Unmarshaler]()) { + // The type reads its own JSON: what it holds is not for its Go fields to tell. + bound = nil + } + + switch node.Kind { + case yaml.DocumentNode: + for _, content := range node.Content { + keepText(content, bound) + } + case yaml.SequenceNode: + for _, item := range node.Content { + keepText(item, elementOf(bound)) + } + case yaml.MappingNode: + for i := 0; i+1 < len(node.Content); i += 2 { + key, value := node.Content[i], node.Content[i+1] + if key.Kind == yaml.ScalarNode && key.Tag != yamlMergeTag && isPlain(key) { + key.Tag = yamlStringTag + } + keepText(value, memberOf(bound, key.Value)) + } + case yaml.ScalarNode: + if !isPlain(node) || node.Tag == yamlNullTag { + return + } + if node.Tag == yamlTimestampTag || (bound != nil && bound.Kind() == reflect.String) { + node.Tag = yamlStringTag + } + } +} + +// isPlain reports whether a scalar was written bare: neither quoted nor given a tag, which +// are the two ways a document says what a value is. +func isPlain(node *yaml.Node) bool { + const explicit = yaml.TaggedStyle | yaml.DoubleQuotedStyle | yaml.SingleQuotedStyle | yaml.LiteralStyle | yaml.FoldedStyle + return node.Style&explicit == 0 +} + +// elementOf returns the type of the items of a list type, or nil. +func elementOf(bound reflect.Type) reflect.Type { + if bound == nil { + return nil + } + if bound.Kind() == reflect.Slice || bound.Kind() == reflect.Array { + return bound.Elem() + } + return nil +} + +// memberOf returns the type of what key holds in a map or a struct type, or nil. For a +// struct it is the field JSON gives the key to: the one named by its json tag, or by its +// own name, whatever the case. +func memberOf(bound reflect.Type, key string) reflect.Type { + if bound == nil { + return nil + } + + switch bound.Kind() { + case reflect.Map: + return bound.Elem() + case reflect.Struct: + for i := range bound.NumField() { + field := bound.Field(i) + if !field.IsExported() { + continue + } + name, _, _ := strings.Cut(field.Tag.Get("json"), ",") + if name == "" { + name = field.Name + } + if strings.EqualFold(name, key) { + return field.Type + } + } + } + return nil +} From fe46311f78c3f9083fe483b9dfb5d42656dcf977 Mon Sep 17 00:00:00 2001 From: Alexandre Bergere Date: Wed, 14 Oct 2026 00:02:19 +0200 Subject: [PATCH 3/6] fix: offer --version on licenses update only, as a check `licenses create` required --version with inline flags and then never sent it. The API numbers the versions of a license itself and refuses a create that names one, so the request has no such field: the flag was mandatory and had no effect. --version is no longer a flag of `create`: passing it is an unknown flag and exits 2, and an inline create needs --name, --description, --type and --default. `licenses update` keeps the flag, where the API does act on it, but not as a setting: it accepts the version the license already has, refuses any other (422 UpdateLicense.VersionNotSettable, exit 5) and keeps the stored one when none is sent. So --version is no longer required there. Given, it is sent for the API to check; left out, the update applies whatever the version. The help of both commands says so. Signed-off-by: Alexandre Bergere Co-Authored-By: Claude Opus 5.5 --- internal/cmd/licenses.go | 26 +++++++++-- internal/cmd/licenses_test.go | 85 +++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/internal/cmd/licenses.go b/internal/cmd/licenses.go index c0003ef..3f8c576 100644 --- a/internal/cmd/licenses.go +++ b/internal/cmd/licenses.go @@ -81,7 +81,11 @@ func newLicensesCreateCommand() *cobra.Command { cmd := &cobra.Command{ Use: "create", Short: "Create a license from --file, --payload, or inline flags", - Args: cobra.NoArgs, + Long: "Create a license from --file, --payload, or inline flags. There is no --version: the\n" + + "API numbers the versions itself and refuses one it is sent. A license that opens a new\n" + + "family is version 1; one added to an existing family, named by familySlug or familyId\n" + + "in --file or --payload, takes the next number.", + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { inputValue, err := resolveInput(file, payload, inline.changed(cmd), inline.build(cmd)) if err != nil { @@ -112,7 +116,12 @@ func newLicensesUpdateCommand() *cobra.Command { cmd := &cobra.Command{ Use: "update ", Short: "Update a license from --file, --payload, or inline flags", - Args: cobra.ExactArgs(1), + Long: "Update a license from --file, --payload, or inline flags. The version of a license is\n" + + "assigned by the API when the license is created and never changes, so --version does\n" + + "not set it: it states the version the license is expected to have, and the API refuses\n" + + "the update (exit code 5) when the license has another one. Left out, the update is\n" + + "applied whatever the version.", + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { inputValue, err := resolveInput(file, payload, inline.changed(cmd), inline.build(cmd)) if err != nil { @@ -132,6 +141,7 @@ func newLicensesUpdateCommand() *cobra.Command { } addInputSourceFlags(cmd, &file, &payload, "license") inline.register(cmd) + inline.registerVersion(cmd) return cmd } @@ -346,11 +356,20 @@ func (f *licenseInputFlags) register(cmd *cobra.Command) { cmd.Flags().StringVar(&f.name, "name", "", "License name") cmd.Flags().StringVar(&f.description, "description", "", "License description") cmd.Flags().StringVar(&f.kind, "type", "", "License type") - cmd.Flags().StringVar(&f.version, "version", "", "License version") cmd.Flags().StringVar(&f.versionName, "version-name", "", "Human-readable license version name") cmd.Flags().BoolVar(&f.isDefault, "default", false, "Whether the license is the default") } +// registerVersion adds --version to update only, and as a check rather than a setting. The +// API numbers the versions of a license itself. Its create refuses any version it is sent, +// so the request the SDK builds there has no such field: on create the flag was required +// and then dropped. Its update accepts the version the license already has, refuses any +// other, and keeps the stored one when none is sent. +func (f *licenseInputFlags) registerVersion(cmd *cobra.Command) { + cmd.Flags().StringVar(&f.version, "version", "", + "Version the license is expected to have: the API checks it and never changes it") +} + // registerSlug adds --slug to create only. The API never renames a license: // its update accepts the slug already in the path and refuses any other, so the // flag could only restate the argument or be refused. @@ -368,7 +387,6 @@ func (f *licenseInputFlags) build(cmd *cobra.Command) func() (sdk.LicenseInput, {f.name, "name"}, {f.description, "description"}, {f.kind, "type"}, - {f.version, "version"}, } { if err := requiredInlineString(required.value, required.flag); err != nil { return sdk.LicenseInput{}, err diff --git a/internal/cmd/licenses_test.go b/internal/cmd/licenses_test.go index 54f12f1..1781795 100644 --- a/internal/cmd/licenses_test.go +++ b/internal/cmd/licenses_test.go @@ -2,6 +2,7 @@ package cmd import ( "net/http" + "strings" "testing" ) @@ -118,6 +119,90 @@ func TestLicensesEntitlementsAssociateSendsTheAllowance(t *testing.T) { } } +// TestLicensesCreateLeavesTheVersionToTheAPI pins create to what the API takes. It numbers +// the versions of a license itself and refuses any it is sent, so the request has no +// version field: --version used to be required there and then dropped. It is no longer a +// flag of create, so passing it is refused before anything is sent. +func TestLicensesCreateLeavesTheVersionToTheAPI(t *testing.T) { + api := givenRoutes(t, map[string]route{ + "POST /licenses": {status: http.StatusCreated, body: `{"name": "Premium", "slug": "premium", "version": "1"}`}, + }) + givenIsolatedConfig(t) + create := []string{ + "--base-url", api.URL, "licenses", "create", + "--name", "Premium", "--description", "Every feature.", "--type", "PAID", "--default=false", + } + + runSuccessfully(t, create...) + + expectCalls(t, api.calls(), "POST /licenses") + expectBody(t, api.calls()[0].body, map[string]any{ + "name": "Premium", "description": "Every feature.", "type": "PAID", "isDefault": false, + }) + + code, err := executeForExitCode(t, append(create, "--version", "2")...) + + if err == nil || !strings.Contains(err.Error(), "unknown flag: --version") { + t.Fatalf("Execute() error = %v, want --version refused as an unknown flag", err) + } + expectExitCode(t, code, exitUsage) + expectCalls(t, api.calls(), "POST /licenses") +} + +// TestLicensesUpdateSendsTheVersionAsACheck pins what is left of --version on update. The +// API accepts there the version the license already has and refuses any other, so the flag +// is sent as given for the API to check, and is no longer required: without it the version +// goes out empty, which the API reads as the one it holds. +func TestLicensesUpdateSendsTheVersionAsACheck(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + {name: "without --version the update names none", want: ""}, + {name: "with --version it is sent for the API to check", args: []string{"--version", "2"}, want: "2"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + api := givenRoutes(t, map[string]route{ + "PUT /licenses/premium-v2": {status: http.StatusNoContent}, + }) + givenIsolatedConfig(t) + + args := append([]string{ + "--base-url", api.URL, "licenses", "update", "premium-v2", + "--name", "Premium", "--description", "Every feature.", "--type", "PAID", "--default=false", + }, test.args...) + runSuccessfully(t, args...) + + expectCalls(t, api.calls(), "PUT /licenses/premium-v2") + expectBody(t, api.calls()[0].body, map[string]any{ + "name": "Premium", "description": "Every feature.", "type": "PAID", "isDefault": false, + "version": test.want, + }) + }) + } +} + +// TestLicensesUpdateReportsARefusedVersion checks the other end of the check: the API's +// refusal of a version the license does not have reaches the operator as a refused request. +func TestLicensesUpdateReportsARefusedVersion(t *testing.T) { + api := givenRoutes(t, map[string]route{ + "PUT /licenses/premium-v2": {status: http.StatusUnprocessableEntity, body: `{ + "status": 422, "title": "Unprocessable Entity", "code": "UpdateLicense.VersionNotSettable", + "detail": "License \"premium-v2\" is version 2; the version is assigned by the server and cannot be changed"}`}, + }) + + code, err := executeForExitCode(t, "--base-url", api.URL, "licenses", "update", "premium-v2", + "--name", "Premium", "--description", "Every feature.", "--type", "PAID", "--default=false", "--version", "3") + + if err == nil || !strings.Contains(err.Error(), "UpdateLicense.VersionNotSettable") { + t.Fatalf("Execute() error = %v, want the API's refusal", err) + } + expectExitCode(t, code, exitRejected) +} + func numberGrant(value float64) map[string]any { return map[string]any{"type": "number", "value": value} } From 9235c477a9c9e0891ee502626fb0a352e4ac31c9 Mon Sep 17 00:00:00 2001 From: Alexandre Bergere Date: Wed, 14 Oct 2026 00:04:50 +0200 Subject: [PATCH 4/6] fix: drop three flags that were accepted and did nothing Each of these parsed, exited 0 and changed nothing: - `components update --previous-component-id`. The link to the version a component follows is set once, at creation: the API's update refuses the field (422 UpdateComponent.PreviousComponentNotSettable) and the request built there never carried it. The flag stays on `create`. - `instances audit-trails --offset`. The audit trail is paged by cursor and the API declares no offset parameter, so none was sent. --limit still caps how many entries come back. - `instances list --include-deleted`. It was sent as include_deleted, which the list has no parameter for and ignores: the result was the same with and without it. All three are now unknown flags and exit 2, as --slug already is on the updates that cannot rename. Signed-off-by: Alexandre Bergere Co-Authored-By: Claude Opus 5.5 --- internal/cmd/components.go | 10 ++++- internal/cmd/components_test.go | 44 +++++++++++++++++++++ internal/cmd/entitlements_test.go | 10 +++-- internal/cmd/instances.go | 9 +---- internal/cmd/instances_test.go | 63 +++++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 12 deletions(-) create mode 100644 internal/cmd/components_test.go create mode 100644 internal/cmd/instances_test.go diff --git a/internal/cmd/components.go b/internal/cmd/components.go index c63510b..45332ad 100644 --- a/internal/cmd/components.go +++ b/internal/cmd/components.go @@ -102,6 +102,7 @@ func newComponentsCreateCommand() *cobra.Command { } addInputSourceFlags(cmd, &file, &payload, "component") inline.register(cmd) + inline.registerPreviousComponentID(cmd) return cmd } @@ -173,10 +174,17 @@ func (f *componentInputFlags) register(cmd *cobra.Command) { cmd.Flags().StringVar(&f.name, "name", "", "Component name") cmd.Flags().StringVar(&f.version, "version", "", "Component version") cmd.Flags().StringVar(&f.description, "description", "", "Component description") - cmd.Flags().StringVar(&f.previousComponentID, "previous-component-id", "", "Previous component ID") cmd.Flags().StringVar(&f.slug, "slug", "", "Component slug") } +// registerPreviousComponentID adds --previous-component-id to create only. The link to the +// version a component follows is set once, when it is created: the API's update refuses +// the field (422 UpdateComponent.PreviousComponentNotSettable) and the request the SDK +// builds there does not carry it, so on update the flag was accepted and dropped. +func (f *componentInputFlags) registerPreviousComponentID(cmd *cobra.Command) { + cmd.Flags().StringVar(&f.previousComponentID, "previous-component-id", "", "ID of the component this one is the next version of") +} + func (f *componentInputFlags) changed(cmd *cobra.Command) bool { return anyFlagChanged(cmd, "name", "version", "description", "previous-component-id", "slug") } diff --git a/internal/cmd/components_test.go b/internal/cmd/components_test.go new file mode 100644 index 0000000..8ab9a02 --- /dev/null +++ b/internal/cmd/components_test.go @@ -0,0 +1,44 @@ +package cmd + +import ( + "net/http" + "strings" + "testing" +) + +// TestPreviousComponentIDIsOfferedOnCreateOnly pins --previous-component-id to the command +// where it does something. The link to the version a component follows is set once, at +// creation: the API's update refuses the field, and the request built there never carried +// it, so `components update --previous-component-id` printed the component and exited 0 +// with the link untouched. Create still sends it; update no longer takes the flag. +func TestPreviousComponentIDIsOfferedOnCreateOnly(t *testing.T) { + const component = `{"id": "cmp_2", "name": "kitchen-display", "slug": "kitchen-display-1-3-0", "version": "1.3.0"}` + api := givenRoutes(t, map[string]route{ + "POST /components": {status: http.StatusCreated, body: component}, + "PUT /components/kitchen-display-1-3-0": {status: http.StatusOK, body: component}, + }) + givenIsolatedConfig(t) + + runSuccessfully(t, "--base-url", api.URL, "components", "create", + "--name", "kitchen-display", "--version", "1.3.0", "--previous-component-id", "cmp_1") + runSuccessfully(t, "--base-url", api.URL, "components", "update", "kitchen-display-1-3-0", + "--name", "kitchen-display", "--version", "1.3.1") + + calls := api.calls() + expectCalls(t, calls, "POST /components", "PUT /components/kitchen-display-1-3-0") + expectBody(t, calls[0].body, map[string]any{ + "name": "kitchen-display", "version": "1.3.0", "previousComponentId": "cmp_1", + }) + expectBody(t, calls[1].body, map[string]any{"name": "kitchen-display", "version": "1.3.1"}) + + code, err := executeForExitCode(t, "--base-url", api.URL, "components", "update", "kitchen-display-1-3-0", + "--name", "kitchen-display", "--version", "1.3.1", "--previous-component-id", "cmp_1") + + if err == nil || !strings.Contains(err.Error(), "unknown flag: --previous-component-id") { + t.Fatalf("Execute() error = %v, want --previous-component-id refused as an unknown flag", err) + } + expectExitCode(t, code, exitUsage) + if got := len(api.calls()); got != len(calls) { + t.Fatalf("the API was called %d more times, want the CLI to stop before the request", got-len(calls)) + } +} diff --git a/internal/cmd/entitlements_test.go b/internal/cmd/entitlements_test.go index 932b2c3..90eb89b 100644 --- a/internal/cmd/entitlements_test.go +++ b/internal/cmd/entitlements_test.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "reflect" "sync" "testing" @@ -137,7 +138,7 @@ type route struct { } // routedAPI is a fake API that answers each "METHOD path" with its route and records every -// call, in order, with its decoded body. +// call, in order, with its query and its decoded body. type routedAPI struct { *httptest.Server @@ -146,8 +147,9 @@ type routedAPI struct { } type recordedCall struct { - call string - body map[string]any + call string + query url.Values + body map[string]any } func (a *routedAPI) calls() []recordedCall { @@ -170,7 +172,7 @@ func givenRoutes(t *testing.T, routes map[string]route) *routedAPI { } } api.mu.Lock() - api.recorded = append(api.recorded, recordedCall{call: call, body: body}) + api.recorded = append(api.recorded, recordedCall{call: call, query: r.URL.Query(), body: body}) api.mu.Unlock() answer, ok := routes[call] diff --git a/internal/cmd/instances.go b/internal/cmd/instances.go index 5847d74..db0283f 100644 --- a/internal/cmd/instances.go +++ b/internal/cmd/instances.go @@ -29,7 +29,6 @@ func newInstancesCommand() *cobra.Command { } func newInstancesListCommand() *cobra.Command { - var includeDeleted bool cmd := &cobra.Command{ Use: "list", Short: "List instances", @@ -41,7 +40,7 @@ func newInstancesListCommand() *cobra.Command { } ctx, cancel := commandContext(cmd) defer cancel() - items, err := client.Instances.List(ctx, &sdk.InstancesListOptions{IncludeDeleted: includeDeleted}) + items, err := client.Instances.List(ctx, nil) if err != nil { return err } @@ -60,7 +59,6 @@ func newInstancesListCommand() *cobra.Command { }) }, } - cmd.Flags().BoolVar(&includeDeleted, "include-deleted", false, "Include soft-deleted instances") return cmd } @@ -174,7 +172,6 @@ func newInstancesAuditTrailsCommand() *cobra.Command { var after string var before string var limit int32 - var offset int32 cmd := &cobra.Command{ Use: "audit-trails ", @@ -201,7 +198,6 @@ func newInstancesAuditTrailsCommand() *cobra.Command { After: afterTime, Before: beforeTime, Limit: optionalInt32(limit), - Offset: optionalInt32(offset), }) if err != nil { return err @@ -226,7 +222,6 @@ func newInstancesAuditTrailsCommand() *cobra.Command { cmd.Flags().StringVar(&after, "after", "", "Only include events after this RFC3339 timestamp") cmd.Flags().StringVar(&before, "before", "", "Only include events before this RFC3339 timestamp") cmd.Flags().Int32Var(&limit, "limit", 0, "Limit results") - cmd.Flags().Int32Var(&offset, "offset", 0, "Offset results") return cmd } @@ -373,7 +368,7 @@ func loadUsageMetadata(metadataFile, metadataPayload string) (map[string]any, er } // optionalInt32 omits a paging bound the caller left at its zero value: the API applies its -// own default for an absent limit or offset, which sending 0 would override. +// own default for an absent limit, which sending 0 would override. func optionalInt32(value int32) *int32 { if value == 0 { return nil diff --git a/internal/cmd/instances_test.go b/internal/cmd/instances_test.go new file mode 100644 index 0000000..817ec49 --- /dev/null +++ b/internal/cmd/instances_test.go @@ -0,0 +1,63 @@ +package cmd + +import ( + "net/http" + "strings" + "testing" +) + +const emptyPage = `{"items": [], "nextCursor": null, "hasMore": false}` + +// TestInstancesReadsOfferOnlyTheFiltersTheAPIReads pins `instances list` and `instances +// audit-trails` to the query parameters the API declares. Two flags were accepted and +// changed nothing: --include-deleted was sent as include_deleted, which the list takes no +// such parameter for and ignores, and --offset was not sent at all, the audit trail being +// paged by cursor. Neither is a flag any more, and neither parameter is sent. +func TestInstancesReadsOfferOnlyTheFiltersTheAPIReads(t *testing.T) { + const auditTrails = "GET /instances/sakura-tokyo-prod/audit-trails" + api := givenRoutes(t, map[string]route{ + "GET /instances": {status: http.StatusOK, body: emptyPage}, + auditTrails: {status: http.StatusOK, body: emptyPage}, + }) + givenIsolatedConfig(t) + + runSuccessfully(t, "--base-url", api.URL, "instances", "list") + runSuccessfully(t, "--base-url", api.URL, "instances", "audit-trails", "sakura-tokyo-prod", + "--event-name", "INSTANCE_UPDATED", "--after", "2026-10-01T00:00:00Z", "--limit", "10") + + calls := api.calls() + expectCalls(t, calls, "GET /instances", auditTrails) + for _, call := range calls { + for _, parameter := range []string{"include_deleted", "offset"} { + if call.query.Has(parameter) { + t.Errorf("%s was sent %s=%q, which the API does not read", call.call, parameter, call.query.Get(parameter)) + } + } + } + // The filters the API does read still go out. + for parameter, want := range map[string]string{ + "event_name": "INSTANCE_UPDATED", "after": "2026-10-01T00:00:00Z", "limit": "10", + } { + if got := calls[1].query.Get(parameter); got != want { + t.Errorf("%s was sent %s=%q, want %q", calls[1].call, parameter, got, want) + } + } + + for _, refused := range []struct { + flag string + args []string + }{ + {flag: "--include-deleted", args: []string{"instances", "list", "--include-deleted"}}, + {flag: "--offset", args: []string{"instances", "audit-trails", "sakura-tokyo-prod", "--offset", "5"}}, + } { + code, err := executeForExitCode(t, append([]string{"--base-url", api.URL}, refused.args...)...) + + if err == nil || !strings.Contains(err.Error(), "unknown flag: "+refused.flag) { + t.Fatalf("Execute(%v) error = %v, want %s refused as an unknown flag", refused.args, err, refused.flag) + } + expectExitCode(t, code, exitUsage) + } + if got := len(api.calls()); got != len(calls) { + t.Fatalf("the API was called %d more times, want the CLI to stop before the request", got-len(calls)) + } +} From ce1a8813a3f9771ebad291fe5dd2f1108189db30 Mon Sep 17 00:00:00 2001 From: Alexandre Bergere Date: Wed, 14 Oct 2026 00:05:56 +0200 Subject: [PATCH 5/6] fix: keep an instance's metadata on an inline update `instances update` with inline flags and no --metadata-json sent `"metadata": {}`. The API replaces an instance's metadata with what it is sent, so an update that only meant to change a description or a license date cleared the instance's metadata, and exited 0. The request cannot leave the metadata out, so the command now reads the instance first when the flags name none, and sends its metadata back. --metadata-json is sent as given, without the read, and `'{}'` clears the metadata on purpose. If the instance cannot be read, nothing is sent. --file and --payload are still sent as written. The deployment zone and the slug needed nothing: the API already keeps them when the request leaves them out. The command's help now says what each optional flag does when absent. Signed-off-by: Alexandre Bergere Co-Authored-By: Claude Opus 5.5 --- internal/cmd/instances.go | 20 +++++- internal/cmd/instances_test.go | 112 +++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 1 deletion(-) diff --git a/internal/cmd/instances.go b/internal/cmd/instances.go index db0283f..ec15a9f 100644 --- a/internal/cmd/instances.go +++ b/internal/cmd/instances.go @@ -119,7 +119,13 @@ func newInstancesUpdateCommand() *cobra.Command { cmd := &cobra.Command{ Use: "update ", Short: "Update an instance from --file, --payload, or inline flags", - Args: cobra.ExactArgs(1), + Long: "Update an instance from --file, --payload, or inline flags. The API replaces the\n" + + "instance with what it is sent, so inline flags name every required field again. What\n" + + "the optional flags leave out is kept: the deployment zone and the slug by the API, and\n" + + "the metadata by the command, which reads the instance first and sends its metadata\n" + + "back unless --metadata-json gives another -- '{}' to clear it. --file and --payload\n" + + "are sent as written.", + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { inputValue, err := resolveInput(file, payload, inline.changed(cmd), inline.build(cmd)) if err != nil { @@ -131,6 +137,18 @@ func newInstancesUpdateCommand() *cobra.Command { } ctx, cancel := commandContext(cmd) defer cancel() + // The API replaces the metadata with what it is sent and the request cannot + // leave it out, so flags that name none would wipe it with the empty object + // they build. They carry back what the instance holds instead. + if inline.changed(cmd) && !flagChanged(cmd, "metadata-json") { + current, err := client.Instances.Get(ctx, args[0]) + if err != nil { + return err + } + if current.Metadata != nil { + inputValue.Metadata = *current.Metadata + } + } if err := client.Instances.Update(ctx, args[0], inputValue); err != nil { return err } diff --git a/internal/cmd/instances_test.go b/internal/cmd/instances_test.go index 817ec49..500568b 100644 --- a/internal/cmd/instances_test.go +++ b/internal/cmd/instances_test.go @@ -61,3 +61,115 @@ func TestInstancesReadsOfferOnlyTheFiltersTheAPIReads(t *testing.T) { t.Fatalf("the API was called %d more times, want the CLI to stop before the request", got-len(calls)) } } + +// TestInstancesUpdateKeepsTheMetadataItDoesNotName pins what an inline update does with an +// instance's metadata. The API replaces the metadata with what it is sent, and the request +// cannot leave it out, so an update whose flags named none used to send an empty object and +// wipe it. The command now reads the instance first and sends its metadata back, unless +// --metadata-json gives the metadata to store -- an empty object included, which is how it +// is cleared on purpose. --file and --payload are still sent as written. +func TestInstancesUpdateKeepsTheMetadataItDoesNotName(t *testing.T) { + const withMetadata = `{ + "id": "ins_1", "name": "Sakura Tokyo Prod", "slug": "sakura-tokyo-prod", "description": "Production", + "customerId": "cus_1", "licenseId": "lic_1", + "startLicenseDate": "2026-01-01T00:00:00Z", "endLicenseDate": "2027-01-01T00:00:00Z", + "metadata": {"region": "kanto", "seats": 40}}` + const withoutMetadata = `{ + "id": "ins_1", "name": "Sakura Tokyo Prod", "slug": "sakura-tokyo-prod", "description": "Production", + "customerId": "cus_1", "licenseId": "lic_1", + "startLicenseDate": "2026-01-01T00:00:00Z", "endLicenseDate": "2027-01-01T00:00:00Z"}` + inline := []string{ + "--name", "Sakura Tokyo Prod", "--description", "Production, Shibuya", "--customer-id", "cus_1", "--license-id", "lic_1", + "--start-license-date", "2026-01-01T00:00:00Z", "--end-license-date", "2027-01-01T00:00:00Z", + } + + tests := []struct { + name string + current string + args []string + wantCalls []string + want map[string]any + }{ + { + name: "flags that name no metadata keep the instance's", + current: withMetadata, + args: inline, + wantCalls: []string{"GET", "PUT"}, + want: map[string]any{"region": "kanto", "seats": float64(40)}, + }, + { + name: "--metadata-json is sent as given, without reading the instance", + current: withMetadata, + args: append(append([]string{}, inline...), "--metadata-json", `{"region": "kansai"}`), + wantCalls: []string{"PUT"}, + want: map[string]any{"region": "kansai"}, + }, + { + name: "an empty --metadata-json clears the metadata", + current: withMetadata, + args: append(append([]string{}, inline...), "--metadata-json", `{}`), + wantCalls: []string{"PUT"}, + want: map[string]any{}, + }, + { + name: "an instance without metadata still sends an object", + current: withoutMetadata, + args: inline, + wantCalls: []string{"GET", "PUT"}, + want: map[string]any{}, + }, + { + name: "a payload is sent as written", + current: withMetadata, + args: []string{"--payload", `{ + "name": "Sakura Tokyo Prod", "description": "Production, Shibuya", "customerId": "cus_1", "licenseId": "lic_1", + "startLicenseDate": "2026-01-01T00:00:00Z", "endLicenseDate": "2027-01-01T00:00:00Z", + "metadata": {"region": "kansai"}}`}, + wantCalls: []string{"PUT"}, + want: map[string]any{"region": "kansai"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + const path = "/instances/sakura-tokyo-prod" + api := givenRoutes(t, map[string]route{ + "GET " + path: {status: http.StatusOK, body: test.current}, + "PUT " + path: {status: http.StatusNoContent}, + }) + givenIsolatedConfig(t) + + runSuccessfully(t, append([]string{"--base-url", api.URL, "instances", "update", "sakura-tokyo-prod"}, test.args...)...) + + calls := api.calls() + wantCalls := make([]string, len(test.wantCalls)) + for i, method := range test.wantCalls { + wantCalls[i] = method + " " + path + } + expectCalls(t, calls, wantCalls...) + expectBody(t, calls[len(calls)-1].body, map[string]any{ + "name": "Sakura Tokyo Prod", "description": "Production, Shibuya", "customerId": "cus_1", "licenseId": "lic_1", + "startLicenseDate": "2026-01-01T00:00:00Z", "endLicenseDate": "2027-01-01T00:00:00Z", + "metadata": test.want, + }) + }) + } +} + +// TestInstancesUpdateStopsWhenTheInstanceCannotBeRead keeps the read a precondition: an +// update that cannot learn the metadata to keep sends nothing rather than an empty object. +func TestInstancesUpdateStopsWhenTheInstanceCannotBeRead(t *testing.T) { + api := givenRoutes(t, map[string]route{ + "GET /instances/gone": {status: http.StatusNotFound, body: `{"status": 404, "title": "Not Found"}`}, + }) + + code, err := executeForExitCode(t, "--base-url", api.URL, "instances", "update", "gone", + "--name", "Gone", "--description", "Removed", "--customer-id", "cus_1", "--license-id", "lic_1", + "--start-license-date", "2026-01-01T00:00:00Z", "--end-license-date", "2027-01-01T00:00:00Z") + + if err == nil { + t.Fatal("Execute() error = nil, want the failed read reported") + } + expectExitCode(t, code, exitNotFound) + expectCalls(t, api.calls(), "GET /instances/gone") +} From 5f5a57d5c90142ddca37bc49963ce10b682b6312 Mon Sep 17 00:00:00 2001 From: Alexandre Bergere Date: Wed, 14 Oct 2026 00:08:45 +0200 Subject: [PATCH 6/6] docs: describe output, input and updates as the commands behave The README said things the code does not do, and left out things the previous commits changed. - `--output` was said to default to `table`. That holds for lists only: a single resource prints YAML, with or without `--output table`, and a command the API answers with no body prints one line. JSON and YAML now carry the same document, which is stated. - `--file` and `--payload` were said to be sent as written by both update commands. That is `entitlements update`. `licenses entitlements update` reads the grant and keeps its allowance whenever `--overage-percent` is absent, whatever carries the value, and a file or a payload holds only the value there. Each is described on its own. - The exit codes of a destructive command were missing: 2 when it cannot prompt and was not given `--yes`, 1 when the prompt is declined. - New: what a file or a payload is made of, how YAML is read and what it refuses, what an inline `instances update` keeps, and where a license's version comes from. - The audit-trail example filtered on `instance.updated`, a name no event has: the API matches the exact event name, INSTANCE_UPDATED. The Installation and Development sections are left as they are. Signed-off-by: Alexandre Bergere Co-Authored-By: Claude Opus 5.5 --- README.md | 56 ++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 733bcd2..54d5e7a 100644 --- a/README.md +++ b/README.md @@ -113,10 +113,19 @@ accepted. kaiten customers list kaiten licenses get team-v1 --output yaml kaiten licenses entitlements list team-v1 -kaiten instances audit-trails acme-production --event-name instance.updated --after 2026-01-01T00:00:00Z --limit 100 +kaiten instances audit-trails acme-production --event-name INSTANCE_UPDATED --after 2026-01-01T00:00:00Z --limit 100 ``` -`--output` (or `KAITEN_OUTPUT`) selects `table` (the default), `json` or `yaml`. +`--output` (or `KAITEN_OUTPUT`) selects `table`, `json` or `yaml`. + +Without it, the format follows what the command returns. A list prints a table. A single +resource (`get`, `create`, a usage report, a new token) prints YAML, and so does +`--output table` there, a single resource having no columns. A command the API answers +with no body (`delete`, most `update`s) prints one line, or a document with a `message` +key under `--output json` and `--output yaml`. + +JSON and YAML output carry the same document: the API's field names and values, in the +same order. ### Writing @@ -137,6 +146,26 @@ kaiten components create --payload '{"name": "api", "version": "1.4.0"}' Feature flags carry targeting rules and variants, so `feature-flags create` and `update` take `--file` or `--payload` only. +A file or a payload uses the API's field names, in JSON or in YAML, and both make the +same request: + +```yaml +# customer.yaml +name: Acme Corp +externalCustomerId: crm-4711 +``` + +A YAML document is read as the JSON it stands for. A value written bare for a text field +stays text (`version: 1.0` is the version `1.0`), and a YAML key the request has no field +for is an error that names the key, before anything is sent. JSON skips such a key +instead, so a resource printed with `--output json` can be edited and written back with +the fields only the API sets (`id`, `createdAt`, ...) still in it; printed as YAML, it has +to lose them first. + +With inline flags, `instances update` keeps what its optional flags leave out: the +deployment zone, the slug, and the metadata, which the command reads from the instance and +sends back unless `--metadata-json` gives another (`--metadata-json '{}'` clears it). + ### Licenses and entitlements ```shell @@ -154,11 +183,21 @@ kaiten licenses entitlements update team-v1 monthly-orders --number 1500 ``` The API replaces an entitlement or a grant with what it is sent, resetting what the request -leaves out. So `entitlements update` and `licenses entitlements update` read the resource -first when given inline flags, and send back everything the flags do not change: a -periodic entitlement keeps its window, which the API refuses to see dropped, and a grant -keeps its allowance unless `--overage-percent` says otherwise. `--file` and `--payload` are -sent as written. +leaves out. So both update commands read the resource first and send back what the +invocation does not change, each in its own way: + +- `entitlements update` does it with inline flags: every field the flags do not name is + sent back as it was read, so a periodic entitlement keeps its window, which the API + refuses to see dropped. `--file` and `--payload` are sent as written there, and carry + every field the entitlement keeps. +- `licenses entitlements update` does it whenever `--overage-percent` is absent, whatever + carries the value: `--number`, `--boolean`, an object, or `--file` and `--payload`, which + hold the value and nothing else. The grant keeps its allowance when the new value is a + finite number. `--overage-percent` sets another, and the grant is then not read. + +The API numbers the versions of a license itself. `licenses create` has no `--version`, +and on `licenses update` the flag does not set the version: it states the one the license +is expected to have, and the API refuses the update when the license has another. ### Usage metering @@ -191,6 +230,9 @@ prompting, so scripts and CI pipelines pass `--yes`: kaiten customers delete acme --yes ``` +Without `--yes` (`-y`), a run that cannot prompt changes nothing and exits with code `2`. +At the prompt, any answer but `y` or `yes` cancels the command, which exits with code `1`. + ## Exit codes Every command reports the kind of failure through its exit code, so a script can