fix: rm the dummpy bad case guard for catalog inputs

.github/pull_request_template.md

@@ -0,0 +1 @@
+<!-- Title: Conventional Commits, e.g. "fix(compose): refuse an unanswered input". Below: what changed, why, and how it was tested. -->

AGENTS.md

@@ -4,6 +4,13 @@
 
 swiss is the backend server and cli client, changelog are made from that perspective, UI things left to console, not here.
 
+## Before calling a change done
+
+- **New feature:** what real problem does it solve that the current code can't? If you can't name one, don't add it.
+- **Scope:** is it over-designed, or guarding edge cases nobody asked about? Prefer the simplest change, and let an existing layer or check handle the rest.
+- **API:** is every API-level change (endpoint, request or response field, site profile key) synced in `docs/openapi.yaml`?
+- **Compatibility:** could it break an older console or an older catalog? swissd runs against both across versions.
+
 ## Comments
 
 - Minimal. Only where the *why* cannot be read off the code.

docs/openapi.yaml

@@ -1798,8 +1798,8 @@ components:
             Answers to variants' `inputs`, by each input's key. Unlike `extra`, an
             answer reaches only a variant that declares the key, in the `derived`
             layer. A string may use `model.pathTemplate`'s placeholders
-            (`{{hf}}`, `{{org}}`, `{{name}}`, `{{model}}`). A key `extra` also sets is
-            refused.
+            (`{{hf}}`, `{{org}}`, `{{name}}`, `{{model}}`). Where `extra` sets the
+            same key, `extra` wins.
           additionalProperties: { type: [string, number, boolean] }
           examples:
             - storageKVOffload.mooncake.master: mooncake-master.svc:50051

internal/catalog/inputs.go

@@ -34,8 +34,7 @@ const (
 
 var inputKey = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_-]*(\.[A-Za-z_][A-Za-z0-9_-]*)*$`)
 
-// ValidInputKey is shared with the site profile, which keys its answers the same way.
-func ValidInputKey(k string) bool { return inputKey.MatchString(k) }
+func validInputKey(k string) bool { return inputKey.MatchString(k) }
 
 func (i Input) TypeOrDefault() string {
 	if i.Type == "" {
@@ -94,13 +93,13 @@ func isInteger(v any) bool {
 	return false
 }
 
-// checkInputs: values leaves every input to compose, which answers it in the derived layer.
+// checkInputs: a value values gives an input's key is the base compose's answer overrides.
 func checkInputs(inputs []Input, vals values.Tree) error {
 	declared := map[string]bool{}
 	for _, in := range inputs {
 		where := "inputs " + in.Key
 		switch {
-		case !ValidInputKey(in.Key):
+		case !validInputKey(in.Key):
 			return fmt.Errorf("inputs: key %q is not a dotted chart key", in.Key)
 		case in.Key == "model.localPath":
 			return fmt.Errorf("%s: swiss answers it from the site's model.pathTemplate, so it is never an input", where)
@@ -112,8 +111,8 @@ func checkInputs(inputs []Input, vals values.Tree) error {
 			return fmt.Errorf("%s: pattern is only for a string input, and this one is %s", where, in.TypeOrDefault())
 		}
 		declared[in.Key] = true
-		if at := setAt(vals, strings.Split(in.Key, ".")); at != "" {
-			return fmt.Errorf("%s: values.%s sets it; give the value as the input's default", where, at)
+		if at := setAt(vals, strings.Split(in.Key, ".")); at != "" && at != in.Key {
+			return fmt.Errorf("%s: values.%s is not a map, so the answer has no place to go", where, at)
 		}
 		if in.Pattern != "" {
 			if _, err := in.pattern(); err != nil {

internal/catalog/inputs_test.go

@@ -38,14 +38,20 @@ func TestAnInputsDefaultAndExampleAreAnswers(t *testing.T) {
 			t.Errorf("default %v: %v", in.Default, err)
 		}
 	}
+	// values may set an input's key: it is the base an answer overrides.
+	for _, base := range []any{"mooncake-master.example:50051", ""} {
+		ok := inputVariant([]Input{mooncakeMaster}, values.Tree{"storageKVOffload": map[string]any{"mooncake": map[string]any{"master": base}}})
+		if err := ok.validate(); err != nil {
+			t.Errorf("values base %q: %v", base, err)
+		}
+	}
 	one := func(in Input) []Input { return []Input{in} }
 	at := func(in Input, edit func(*Input)) Input { edit(&in); return in }
 	for name, c := range map[string]struct {
 		v    Variant
 		want string
 	}{
-		"values sets it":      {inputVariant(one(mooncakeMaster), values.Tree{"storageKVOffload": map[string]any{"mooncake": map[string]any{"master": "mc.svc:50051"}}}), "values.storageKVOffload.mooncake.master sets it; give the value as the input's default"},
-		"a non-map above it":  {inputVariant(one(mooncakeMaster), values.Tree{"storageKVOffload": map[string]any{"mooncake": "off"}}), "values.storageKVOffload.mooncake sets it"},
+		"a non-map above it":  {inputVariant(one(mooncakeMaster), values.Tree{"storageKVOffload": map[string]any{"mooncake": "off"}}), "values.storageKVOffload.mooncake is not a map"},
 		"default off pattern": {inputVariant(one(at(mooncakeMaster, func(i *Input) { i.Default = "no-port" })), nil), `default "no-port" does not match`},
 		"a blank default":     {inputVariant(one(at(mooncakeMaster, func(i *Input) { i.Default = "" })), nil), `default "" does not match`},
 		"default of a type":   {inputVariant(one(Input{Key: "a.b", Title: "t", Type: InputInteger, Example: 1, Default: "8"}), nil), "default 8 is not an integer"},

internal/compose/compose.go

@@ -385,8 +385,8 @@ func applyDefaults(site, derived, base values.Tree, in Input) error {
 	return nil
 }
 
-// applyInputs puts each input's answer in the derived layer, under the form's
-// overrides: the site's, else the input's default.
+// applyInputs puts each input's answer in the derived layer, over the variant's
+// values and under the form's overrides: the site's, else the input's default.
 func applyInputs(derived, site values.Tree, in Input) error {
 	for _, input := range in.Resolved.Variant.Inputs {
 		answer, ok, err := in.Profile.InputAnswer(input.Key, in.Resolved.Source.HF, in.Resolved.Model)
@@ -400,7 +400,7 @@ func applyInputs(derived, site values.Tree, in Input) error {
 			continue
 		}
 		if _, set := values.Get(site, input.Key); set {
-			continue
+			continue // extra wins over an input answer
 		}
 		if err := values.Set(derived, input.Key, answer); err != nil {
 			return err

internal/compose/inputs_test.go

@@ -106,6 +106,56 @@ func TestTheVariantsDefaultIsTheLowestAnswer(t *testing.T) {
 	}
 }
 
+// A value the variant's values give the key stands when nobody answers, and
+// any answer overrides it.
+func TestTheVariantsValueIsTheBaseAnAnswerOverrides(t *testing.T) {
+	for name, c := range map[string]struct {
+		site  map[string]any
+		def   any
+		want  string
+		layer string
+	}{
+		"nobody answers":      {want: "mooncake-master.example:50051", layer: values.LayerCatalog},
+		"the site answers":    {site: map[string]any{masterKey: "site.svc:50051"}, want: "site.svc:50051", layer: values.LayerDerived},
+		"the default answers": {def: "tuned.svc:50051", want: "tuned.svc:50051", layer: values.LayerDerived},
+	} {
+		in := inputsInput()
+		in.Resolved.Variant.Values["storageKVOffload"].(map[string]any)["mooncake"].(map[string]any)["master"] = "mooncake-master.example:50051"
+		in.Resolved.Variant.Inputs[0].Default = c.def
+		in.Profile.Inputs = c.site
+		p, err := Compose(in)
+		if err != nil {
+			t.Fatalf("%s: %v", name, err)
+		}
+		if got, _ := values.Get(p.Values(), masterKey); got != c.want || p.LayerOf(masterKey) != c.layer {
+			t.Errorf("%s: %s = %v from %s, want %s from %s", name, masterKey, got, p.LayerOf(masterKey), c.want, c.layer)
+		}
+	}
+}
+
+// extra sets the key too: extra wins, and the input answer does nothing.
+func TestExtraOverridesTheSitesInputAnswer(t *testing.T) {
+	in := inputsInput()
+	in.Profile.Inputs = map[string]any{masterKey: "site.svc:50051"}
+	in.Profile.Extra = values.Tree{"storageKVOffload": map[string]any{"mooncake": map[string]any{"master": "extra.svc:50051"}}}
+	p, err := Compose(in)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got, _ := values.Get(p.Values(), masterKey); got != "extra.svc:50051" || p.LayerOf(masterKey) != values.LayerSite {
+		t.Errorf("%s = %v from %s, want extra's", masterKey, got, p.LayerOf(masterKey))
+	}
+}
+
+// The profile takes inputs as written; compose refuses one it cannot fill.
+func TestASiteAnswerWithAnUnknownPlaceholderRefusesThePlan(t *testing.T) {
+	in := inputsInput()
+	in.Profile.Inputs = map[string]any{masterKey: "{{release}}.svc:50051"}
+	if _, err := Compose(in); err == nil || !strings.Contains(err.Error(), "unknown placeholder") {
+		t.Fatalf("want a refusal naming the placeholder, got %v", err)
+	}
+}
+
 func TestAnAnswerTheInputDoesNotTakeRefusesThePlan(t *testing.T) {
 	in := inputsInput()
 	in.Profile.Inputs = map[string]any{masterKey: "no-port"}

internal/site/inputs_test.go

@@ -43,17 +43,3 @@ func TestATemplateNamingTheRepoNeedsASource(t *testing.T) {
 		t.Errorf("{{model}} needs no source: %v, %v", got, err)
 	}
 }
-
-func TestAnAnswerThatCouldNeverBeUsedIsRefused(t *testing.T) {
-	for name, c := range map[string]struct{ yaml, want string }{
-		"unknown placeholder": {"inputs:\n  a.b: /x/{{release}}\n", "unknown placeholder"},
-		"not a scalar":        {"inputs:\n  a.b: {host: h}\n", "string, integer or boolean"},
-		"no value":            {"inputs:\n  a.b:\n", "string, integer or boolean"},
-		"not a key":           {"inputs:\n  a..b: x\n", "not a dotted chart key"},
-		"extra answers too":   {"extra:\n  a:\n    b: y\ninputs:\n  a.b: x\n", "extra sets the same key"},
-	} {
-		if _, err := Parse([]byte(inputsHead+c.yaml), "test"); err == nil || !strings.Contains(err.Error(), c.want) {
-			t.Errorf("%s: want an error containing %q, got %v", name, c.want, err)
-		}
-	}
-}

internal/site/profile.go

@@ -7,9 +7,7 @@ package site
 
 import (
 	"fmt"
-	"maps"
 	"os"
-	"slices"
 	"strings"
 	"time"
 
@@ -208,29 +206,6 @@ func (f FeatureDefaults) Resolve() map[string]bool {
 	}
 }
 
-// checkInputs refuses an answer that could never be used, or a second answer from extra.
-func checkInputs(inputs map[string]any, extra values.Tree) error {
-	for _, key := range slices.Sorted(maps.Keys(inputs)) {
-		where := fmt.Sprintf("inputs[%q]", key)
-		if !catalog.ValidInputKey(key) {
-			return fmt.Errorf("%s: not a dotted chart key", where)
-		}
-		switch v := inputs[key].(type) {
-		case string:
-			if _, err := fill(v, "org/name", "model"); err != nil {
-				return fmt.Errorf("%s: %w", where, err)
-			}
-		case bool, int, int64, uint64, float64:
-		default:
-			return fmt.Errorf("%s: an answer is a string, integer or boolean, not %T", where, v)
-		}
-		if _, set := values.Get(extra, key); set {
-			return fmt.Errorf("%s: extra sets the same key; give the one answer here", where)
-		}
-	}
-	return nil
-}
-
 // InputAnswer is the site's answer to an input, placeholders filled for this model.
 func (p Profile) InputAnswer(key, hf, model string) (answer any, ok bool, err error) {
 	v, ok := p.Inputs[key]
@@ -553,9 +528,6 @@ func Parse(raw []byte, origin string) (*Profile, error) {
 	if err := p.Nodes.checkGPUs(); err != nil {
 		return nil, fmt.Errorf("%s: %w", origin, err)
 	}
-	if err := checkInputs(p.Inputs, p.Extra); err != nil {
-		return nil, fmt.Errorf("%s: %w", origin, err)
-	}
 	return &p, nil
 }