feat(config): read YAML lists and maps, and refuse shapes that do not…

cmd/kosli/root.go

@@ -562,6 +562,97 @@ func initialize(cmd *cobra.Command, out, errOut io.Writer) error {
 	return nil
 }
 
+// scalarString renders one decoded config value as the text a flag would have
+// been given on the command line, and refuses a value that is itself a
+// structure. A flag holds values, so a nested sequence or mapping has nowhere to
+// go; rendering it would produce a plausible-looking string rather than an
+// error.
+func scalarString(value any) (string, error) {
+	switch value.(type) {
+	case []interface{}:
+		return "", fmt.Errorf("expects values, but one of them is a list")
+	case map[string]interface{}:
+		return "", fmt.Errorf("expects values, but one of them is a set of key/value pairs")
+	default:
+		return fmt.Sprintf("%v", value), nil
+	}
+}
+
+// scalarStrings renders every element of a decoded sequence, refusing the whole
+// sequence if any element is itself a structure.
+func scalarStrings(values []interface{}) ([]string, error) {
+	rendered := make([]string, len(values))
+	for i, value := range values {
+		element, err := scalarString(value)
+		if err != nil {
+			return nil, err
+		}
+		rendered[i] = element
+	}
+	return rendered, nil
+}
+
+// applyConfigValue writes a value taken from the config file or the environment
+// onto its flag, and refuses one whose shape the flag cannot hold.
+//
+// A decoded YAML value is one of three things: a sequence, a mapping, or a
+// scalar. A flag holds values, not structures, so only two of those shapes have
+// anywhere to go - a sequence onto a multi-value flag, a mapping onto a
+// key=value flag - and only when their contents are scalars.
+//
+// Anything else is refused rather than rendered. Rendering is what makes this
+// worth stating: %v turns any shape into a plausible-looking string, so a list
+// of mappings would become the attestation name "map[name:coverage]" instead of
+// an error. The shapes someone can write are unbounded, so the check is on what
+// fits rather than on a list of known mistakes: the four ways to arrive here
+// without a home are a sequence on a flag that holds one value, a sequence whose
+// elements are not scalars, a mapping on a flag that is not key=value, and a
+// mapping whose values are not scalars.
+func applyConfigValue(flags *pflag.FlagSet, flag *pflag.Flag, value any) error {
+	switch shaped := value.(type) {
+	case []interface{}:
+		sliceValue, isSlice := flag.Value.(pflag.SliceValue)
+		if !isSlice {
+			return fmt.Errorf("expects a single value, but a list was given")
+		}
+		elements, err := scalarStrings(shaped)
+		if err != nil {
+			return err
+		}
+		if err := sliceValue.Replace(elements); err != nil {
+			return err
+		}
+
+	case map[string]interface{}:
+		// The pairs are applied one at a time, in the form the flag accepts,
+		// exactly as repeating the flag on the command line would. The first Set
+		// replaces whatever the flag held and later ones merge, so the random
+		// order of a Go map range does not affect the result.
+		if flag.Value.Type() != "stringToString" {
+			return fmt.Errorf("expects a single value, but a set of key/value pairs was given")
+		}
+		for key, item := range shaped {
+			if _, err := scalarString(item); err != nil {
+				return err
+			}
+			if err := flags.Set(flag.Name, fmt.Sprintf("%v=%v", key, item)); err != nil {
+				return err
+			}
+		}
+		return nil
+
+	default:
+		return flags.Set(flag.Name, fmt.Sprintf("%v", value))
+	}
+
+	// Replace writes the value directly, bypassing FlagSet.Set, so the flag must
+	// be marked as set here. Required-flag validation and every Changed() check
+	// read this field.
+	flag.Changed = true
+
+	return nil
+}
+
 // configValueSource names where a flag's value came from, so that a failure to
 // apply it points at the thing the user has to edit. viper reads a value from
 // the environment when the bound variable holds one, and from the config file
@@ -638,7 +729,7 @@ func bindFlags(cmd *cobra.Command, v *viper.Viper) error {
 				}
 			}
 
-			if err := cmd.Flags().Set(f.Name, fmt.Sprintf("%v", val)); err != nil {
+			if err := applyConfigValue(cmd.Flags(), f, val); err != nil {
 				bindErr = errors.Join(bindErr, fmt.Errorf("failed to set flag '--%s' from %s: %v", f.Name, configValueSource(f.Name), err))
 			}
 		}

cmd/kosli/root_test.go

@@ -93,6 +93,95 @@ func (suite *RootCommandTestSuite) TestConfigProcessing() {
 	runTestCmd(suite.T(), tests)
 }
 
+// TestConfigFileYamlListReachesSliceFlagAsElements pins that a list written the
+// YAML way reaches a multi-value flag as separate elements, which is the
+// spelling anyone writing a YAML config file reaches for first.
+//
+// attach-policy is used because it reads --environment without mutating it, so
+// the assertion sees only what parsing produced.
+func (suite *RootCommandTestSuite) TestConfigFileYamlListReachesSliceFlagAsElements() {
+	c, _, _, _, err := executeCommandC(
+		"attach-policy mypolicy --config-file testdata/config/yaml-list-value.yaml --dry-run")
+	suite.Require().NoError(err)
+
+	environments, err := c.Flags().GetStringSlice("environment")
+	suite.Require().NoError(err)
+	suite.Equal([]string{"prod", "staging"}, environments)
+}
+
+// TestConfigFileYamlMapReachesMapFlagAsPairs pins that a mapping written the
+// YAML way reaches a map-valued flag as its pairs, the counterpart of the list
+// case and the other spelling a YAML config file invites.
+func (suite *RootCommandTestSuite) TestConfigFileYamlMapReachesMapFlagAsPairs() {
+	c, _, _, _, err := executeCommandC(
+		"attest generic --config-file testdata/config/yaml-map-value.yaml " +
+			"--fingerprint 0000000000000000000000000000000000000000000000000000000000000001 " +
+			"--name foo --flow f --trail t --dry-run")
+	suite.Require().NoError(err)
+
+	externalURLs, err := c.Flags().GetStringToString("external-url")
+	suite.Require().NoError(err)
+	suite.Equal(map[string]string{"docs": "https://example.com/docs"}, externalURLs)
+}
+
+// TestConfigFileYamlListWithEmptyElementIsRejected pins that reading a YAML list
+// does not become a way past the refusal of empty elements. The flag types that
+// refuse an empty element do so in Set and in Replace, and a config file list
+// arrives through Replace, so the two have to meet here rather than each being
+// correct alone.
+func (suite *RootCommandTestSuite) TestConfigFileYamlListWithEmptyElementIsRejected() {
+	_, _, _, _, err := executeCommandC(
+		"attest generic --config-file testdata/config/yaml-list-with-empty-element.yaml " +
+			"--fingerprint 0000000000000000000000000000000000000000000000000000000000000001 " +
+			"--name foo --flow f --trail t --dry-run")
+
+	suite.Require().Error(err)
+	suite.ErrorContains(err, "attachments")
+	suite.ErrorContains(err, "empty values are not allowed")
+}
+
+// TestConfigFileValueThatDoesNotFitItsFlagIsRejected is the counterpart to the
+// two tests above: the shapes a flag has nowhere to put, and the flag each
+// message must name.
+//
+// A flag holds values, not structures, so a value fits only when it is a scalar,
+// a list of scalars on a multi-value flag, or a mapping of scalars on a
+// key=value flag. Everything else is refused rather than rendered, which is the
+// point: rendering turns any shape into a plausible-looking string, so a list of
+// mappings would become the attestation name "map[name:coverage]" and a list on
+// --description would become the literal "[a b]" - both the kind of thing
+// noticed long after the run that produced it.
+//
+// The shapes someone can write are unbounded, so the rule is what fits rather
+// than a list of known mistakes. New shapes belong here as rows.
+func (suite *RootCommandTestSuite) TestConfigFileValueThatDoesNotFitItsFlagIsRejected() {
+	cases := []struct {
+		name     string
+		file     string
+		wantFlag string
+	}{
+		{
+			name:     "list on a flag that holds one value",
+			file:     "yaml-list-on-scalar-flag.yaml",
+			wantFlag: "description",
+		},
+		{
+			name:     "list whose elements are mappings",
+			file:     "yaml-list-of-mappings.yaml",
+			wantFlag: "template",
+		},
+	}
+	for _, tc := range cases {
+		suite.Run(tc.name, func() {
+			_, _, _, _, err := executeCommandC(
+				"create flow myflow --config-file testdata/config/" + tc.file + " --dry-run")
+
+			suite.Require().Error(err)
+			suite.ErrorContains(err, tc.wantFlag)
+		})
+	}
+}
+
 // TestEmptyApiTokenEnvVarStillDecryptsConfigToken pins that KOSLI_API_TOKEN set
 // to an empty string does not suppress decryption of a config-file token.
 // bindFlags decides the token came from the environment with os.LookupEnv, where

cmd/kosli/testdata/config/invalid-flag-value.yaml

@@ -1,4 +1,3 @@
 org: demo
 api-token: DRY_RUN
-link:
-  docs: https://example.com/docs
+link: notkeyvalue

cmd/kosli/testdata/config/two-invalid-flag-values.yaml

@@ -1,5 +1,4 @@
 org: demo
 api-token: DRY_RUN
-link:
-  docs: https://example.com/docs
+link: notkeyvalue
 max-api-retries: not-a-number

cmd/kosli/testdata/config/yaml-list-of-mappings.yaml

@@ -0,0 +1,5 @@
+org: demo
+api-token: DRY_RUN
+template:
+  - name: coverage
+  - name: unit-test

cmd/kosli/testdata/config/yaml-list-on-scalar-flag.yaml

@@ -0,0 +1,5 @@
+org: demo
+api-token: DRY_RUN
+description:
+  - a
+  - b

cmd/kosli/testdata/config/yaml-list-value.yaml

@@ -0,0 +1,5 @@
+org: demo
+api-token: DRY_RUN
+environment:
+  - prod
+  - staging

cmd/kosli/testdata/config/yaml-list-with-empty-element.yaml

@@ -0,0 +1,5 @@
+org: demo
+api-token: DRY_RUN
+attachments:
+  - ""
+  - testdata/file1

cmd/kosli/testdata/config/yaml-map-value.yaml

@@ -0,0 +1,4 @@
+org: demo
+api-token: DRY_RUN
+external-url:
+  docs: https://example.com/docs