From a6b459536b995d1d267fe8020e1fab03bf9c8625 Mon Sep 17 00:00:00 2001 From: riddhibhagwat-db Date: Tue, 4 Aug 2026 17:17:34 +0000 Subject: [PATCH 1/4] [air] Document run config fields via `air run -h config.` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `air run` takes a YAML config with ~35 fields across 9 nested structs, and the only way to learn a field was to read runconfig.go or trip its validation error. Add schema help addressed by dotted path: air run -h config # list top-level fields air run -h config.compute # list a section's fields air run -h config.compute.accelerator_type air run -h compute.accelerator_type # the config. prefix is optional Descriptions live in `help:` struct tags beside the `yaml:` tags, so they sit next to the validation rules that enforce them. Required-ness is a string rather than a bool because validate() enforces it contextually: docker_image.url is required only once docker_image is present. The path must be a separate argument. cobra hardcodes a bool read of the help flag in execute(), so a string -h is rejected outright and -h= cannot work; the Long text says so rather than leaving it to be discovered. Cobra returns flag.ErrHelp before Args and required-flag validation, which is why this needs no --file. A reflection test fails if any schema field lacks a `help:` tag, so a new field cannot merge undocumented — the same guard annotations.yml gets from its PLACEHOLDER convention. --- .../air/config-help/out.test.toml | 3 + .../experimental/air/config-help/output.txt | 110 ++++++ .../experimental/air/config-help/script | 31 ++ experimental/air/cmd/compute.go | 4 +- experimental/air/cmd/run.go | 32 +- experimental/air/cmd/runconfig.go | 365 ++++++++++++++++-- experimental/air/cmd/runconfig_test.go | 218 +++++++++++ 7 files changed, 727 insertions(+), 36 deletions(-) create mode 100644 acceptance/experimental/air/config-help/out.test.toml create mode 100644 acceptance/experimental/air/config-help/output.txt create mode 100644 acceptance/experimental/air/config-help/script diff --git a/acceptance/experimental/air/config-help/out.test.toml b/acceptance/experimental/air/config-help/out.test.toml new file mode 100644 index 00000000000..d6187dcb046 --- /dev/null +++ b/acceptance/experimental/air/config-help/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/experimental/air/config-help/output.txt b/acceptance/experimental/air/config-help/output.txt new file mode 100644 index 00000000000..2d7dffc135d --- /dev/null +++ b/acceptance/experimental/air/config-help/output.txt @@ -0,0 +1,110 @@ + +=== command help +>>> [CLI] experimental air run --help +Submit a training workload to Databricks serverless GPU compute. + +The workload is described by a YAML config file (see --file). + +To look up a config field, pass its path to -h: + + databricks experimental air run -h config + databricks experimental air run -h config.compute + databricks experimental air run -h config.compute.accelerator_type + +The path must be a separate argument: cobra reserves -h as a boolean, so +-h=config.compute and -hconfig.compute are not accepted. + +Usage: + databricks experimental air run [flags] + +Flags: + --dry-run Validate the config without submitting + -f, --file string Path to the workload YAML config + -h, --help help for run + --idempotency-key string Return the existing run if this key was already used + --override stringArray Override a YAML field, e.g. compute.num_accelerators=8 (repeatable) + --watch Stream logs until the run completes + +Global Flags: + --debug enable debug logging + -o, --output type output type: text or json (default text) + -p, --profile string ~/.databrickscfg profile + -t, --target string bundle target to use (if applicable) + +=== schema overview +>>> [CLI] experimental air run -h config +config + The run YAML schema. Pass a field path for details, e.g. config.compute.accelerator_type. + + Fields: + experiment_name (required) Name of the experiment. + compute (required) Which accelerators to run on and how many. + environment Python dependencies, or a custom Docker image, for the run's runtime. + command (required) Shell command that starts the workload. + env_variables Plain environment variables, as NAME: value. + secrets Environment variables sourced from secrets, as NAME: scope/key. + code_source Local code to upload and make available to the run. + max_retries How many times to retry a failed run. + timeout_minutes Wall-clock limit for the run in minutes. + idempotency_token Reuse token: a repeat submission with the same token returns the existing run instead of starting another. + parameters Free-form values passed through to the workload. + mlflow_run_name Name for the MLflow run. + mlflow_experiment_directory Workspace directory holding the MLflow experiment. + permissions Who may view or manage the run, as a list of principal plus level grants. + usage_policy_name Usage policy to bill the run to, by name. + usage_policy_id Usage policy to bill the run to, by id. + +Use "-h config." for details on a field. + +=== nested object lists its fields +>>> [CLI] experimental air run -h config.compute +config.compute + Which accelerators to run on and how many. + + Fields: + num_accelerators Total number of GPUs to allocate. + accelerator_type Which accelerator to run on, e.g. GPU_1xA10. + +Use "-h config.compute." for details on a field. + +=== leaf field +>>> [CLI] experimental air run -h config.compute.accelerator_type +config.compute.accelerator_type + Which accelerator to run on, e.g. GPU_1xA10. See https://docs.databricks.com/aws/en/machine-learning/ai-runtime/cli/yaml-config#reference for the current list of supported GPU types. Matched case-sensitively. + + Type: string + Required: no + +=== conditionally required leaf +>>> [CLI] experimental air run -h config.environment.docker_image.url +config.environment.docker_image.url + Fully qualified image URL, e.g. myregistry.io/team/train:v3. + + Type: string + Required: when environment.docker_image is set + +=== union field reports both accepted shapes +>>> [CLI] experimental air run -h config.code_source.snapshot.git.remote +config.code_source.snapshot.git.remote + No longer supported: the snapshot archives your local copy. Only false is accepted; use commit to pin a revision. + + Type: bool or string + Required: no + +=== the config. prefix is optional +>>> [CLI] experimental air run -h compute.num_accelerators +config.compute.num_accelerators + Total number of GPUs to allocate. Must be a positive multiple of the accelerator type's per-node GPU count. See https://docs.databricks.com/aws/en/machine-learning/ai-runtime/cli/yaml-config#reference for supported GPU types. + + Type: int + Required: no + +=== unknown field suggests the near match +>>> [CLI] experimental air run -h config.compute.acclerator_type +Error: unknown config field "config.compute.acclerator_type"; did you mean "accelerator_type"? + +fields under "config.compute" are: accelerator_type, num_accelerators + +=== free-form map keys are not schema fields +>>> [CLI] experimental air run -h config.parameters.learning_rate +Error: "config.parameters" holds user-defined keys, so "learning_rate" is not part of the schema; see "config.parameters" instead diff --git a/acceptance/experimental/air/config-help/script b/acceptance/experimental/air/config-help/script new file mode 100644 index 00000000000..d1c25be1100 --- /dev/null +++ b/acceptance/experimental/air/config-help/script @@ -0,0 +1,31 @@ +# Pin the rendered config field help. Any change to a `help:` struct tag on the +# run YAML schema shows up as a diff here. + +# Plain -h must keep documenting the command itself, and advertise the config +# path syntax so the feature is discoverable. +title "command help" +trace $CLI experimental air run --help + +title "schema overview" +trace $CLI experimental air run -h config + +title "nested object lists its fields" +trace $CLI experimental air run -h config.compute + +title "leaf field" +trace $CLI experimental air run -h config.compute.accelerator_type + +title "conditionally required leaf" +trace $CLI experimental air run -h config.environment.docker_image.url + +title "union field reports both accepted shapes" +trace $CLI experimental air run -h config.code_source.snapshot.git.remote + +title "the config. prefix is optional" +trace $CLI experimental air run -h compute.num_accelerators + +title "unknown field suggests the near match" +trace $CLI experimental air run -h config.compute.acclerator_type + +title "free-form map keys are not schema fields" +trace $CLI experimental air run -h config.parameters.learning_rate diff --git a/experimental/air/cmd/compute.go b/experimental/air/cmd/compute.go index 07013c53906..e7b69d470be 100644 --- a/experimental/air/cmd/compute.go +++ b/experimental/air/cmd/compute.go @@ -54,8 +54,8 @@ func gpusPerNode(g gpuType) (int, error) { // computeConfig is the `compute` block of the run YAML: which accelerators to // use and how many. type computeConfig struct { - NumAccelerators int `yaml:"num_accelerators"` - AcceleratorType string `yaml:"accelerator_type"` + NumAccelerators int `yaml:"num_accelerators" help:"Total number of GPUs to allocate. Must be a positive multiple of the accelerator type's per-node GPU count. See https://docs.databricks.com/aws/en/machine-learning/ai-runtime/cli/yaml-config#reference for supported GPU types."` + AcceleratorType string `yaml:"accelerator_type" help:"Which accelerator to run on, e.g. GPU_1xA10. See https://docs.databricks.com/aws/en/machine-learning/ai-runtime/cli/yaml-config#reference for the current list of supported GPU types. Matched case-sensitively."` } // validate checks the compute block against the backend's constraints. diff --git a/experimental/air/cmd/run.go b/experimental/air/cmd/run.go index ea00368679f..aff2c1db8f5 100644 --- a/experimental/air/cmd/run.go +++ b/experimental/air/cmd/run.go @@ -36,9 +36,39 @@ func newRunCommand() *cobra.Command { Short: "Submit a training workload from a YAML config", Long: `Submit a training workload to Databricks serverless GPU compute. -The workload is described by a YAML config file (see --file).`, +The workload is described by a YAML config file (see --file). + +To look up a config field, pass its path to -h: + + databricks experimental air run -h config + databricks experimental air run -h config.compute + databricks experimental air run -h config.compute.accelerator_type + +The path must be a separate argument: cobra reserves -h as a boolean, so +-h=config.compute and -hconfig.compute are not accepted.`, } + // Document a config field instead of the command when -h is given a path. + // cobra hands the help function the positional args it collected, and it does + // so before Args and required-flag validation, so -f is not needed here. + cmd.SetHelpFunc(func(c *cobra.Command, args []string) { + fields := c.Flags().Args() + if len(fields) == 0 { + // Resolved through the parent rather than captured up front, so this + // picks up the inherited help function instead of cobra's default. + // A detached command (as in unit tests) has no parent to inherit from. + if parent := c.Parent(); parent != nil { + parent.HelpFunc()(c, args) + return + } + _ = c.Usage() + return + } + if err := writeConfigFieldHelp(c.OutOrStdout(), fields[0]); err != nil { + c.PrintErrln("Error:", err) + } + }) + cmd.Flags().StringVarP(&file, "file", "f", "", "Path to the workload YAML config") cmd.Flags().BoolVar(&watch, "watch", false, "Stream logs until the run completes") cmd.Flags().StringArrayVar(&overrides, "override", nil, "Override a YAML field, e.g. compute.num_accelerators=8 (repeatable)") diff --git a/experimental/air/cmd/runconfig.go b/experimental/air/cmd/runconfig.go index 4cfbf3736d1..580a51c3b8d 100644 --- a/experimental/air/cmd/runconfig.go +++ b/experimental/air/cmd/runconfig.go @@ -3,7 +3,9 @@ package aircmd import ( "errors" "fmt" + "io" "maps" + "reflect" "regexp" "slices" "strings" @@ -39,24 +41,24 @@ var uuidRe = regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[ // runConfig is the top-level run YAML schema: experiment_name + compute / // environment / code_source plus the command and run options. type runConfig struct { - ExperimentName string `yaml:"experiment_name"` - Compute *computeConfig `yaml:"compute"` - Environment *environmentConfig `yaml:"environment"` - Command *string `yaml:"command"` - EnvVariables map[string]string `yaml:"env_variables"` - Secrets map[string]string `yaml:"secrets"` - CodeSource *codeSourceConfig `yaml:"code_source"` + ExperimentName string `yaml:"experiment_name" help:"Name of the experiment. Becomes the Jobs API task key: max 100 characters, alphanumerics, hyphens, and underscores only." required:"yes"` + Compute *computeConfig `yaml:"compute" help:"Which accelerators to run on and how many." required:"yes"` + Environment *environmentConfig `yaml:"environment" help:"Python dependencies, or a custom Docker image, for the run's runtime."` + Command *string `yaml:"command" help:"Shell command that starts the workload. Max 1000 lines; move longer logic into a script under code_source." required:"yes"` + EnvVariables map[string]string `yaml:"env_variables" help:"Plain environment variables, as NAME: value. A name here cannot also appear in secrets."` + Secrets map[string]string `yaml:"secrets" help:"Environment variables sourced from secrets, as NAME: scope/key."` + CodeSource *codeSourceConfig `yaml:"code_source" help:"Local code to upload and make available to the run."` // MaxRetries defaults to 3 when unset; default-filling is a normalization // concern handled at launch, so a nil pointer is left as-is here. - MaxRetries *int `yaml:"max_retries"` - TimeoutMinutes *int `yaml:"timeout_minutes"` - IdempotencyToken *string `yaml:"idempotency_token"` - Parameters map[string]any `yaml:"parameters"` - MLflowRunName *string `yaml:"mlflow_run_name"` - MLflowExperimentDirectory *string `yaml:"mlflow_experiment_directory"` - Permissions []permission `yaml:"permissions"` - UsagePolicyName *string `yaml:"usage_policy_name"` - UsagePolicyID *string `yaml:"usage_policy_id"` + MaxRetries *int `yaml:"max_retries" help:"How many times to retry a failed run. Must be >= 0. Defaults to 3 when unset."` + TimeoutMinutes *int `yaml:"timeout_minutes" help:"Wall-clock limit for the run in minutes. Must be >= 1."` + IdempotencyToken *string `yaml:"idempotency_token" help:"Reuse token: a repeat submission with the same token returns the existing run instead of starting another. Max 64 characters."` + Parameters map[string]any `yaml:"parameters" help:"Free-form values passed through to the workload. Any nested structure is allowed."` + MLflowRunName *string `yaml:"mlflow_run_name" help:"Name for the MLflow run. Max 100 characters, alphanumerics, hyphens, and underscores only."` + MLflowExperimentDirectory *string `yaml:"mlflow_experiment_directory" help:"Workspace directory holding the MLflow experiment. Must start with /Workspace."` + Permissions []permission `yaml:"permissions" help:"Who may view or manage the run, as a list of principal plus level grants."` + UsagePolicyName *string `yaml:"usage_policy_name" help:"Usage policy to bill the run to, by name. Max 127 characters. Mutually exclusive with usage_policy_id."` + UsagePolicyID *string `yaml:"usage_policy_id" help:"Usage policy to bill the run to, by id. Mutually exclusive with usage_policy_name."` } // validate runs structural validation over the whole config, returning the first @@ -227,9 +229,9 @@ func validateSecretRefs(secrets map[string]string) error { // environmentConfig is the `environment` block: dependencies and/or a custom // docker image. type environmentConfig struct { - Dependencies dependencies `yaml:"dependencies"` - Version stringOrInt `yaml:"version"` - DockerImage *dockerImageConfig `yaml:"docker_image"` + Dependencies dependencies `yaml:"dependencies" help:"Inline list of packages to install. Not allowed alongside docker_image."` + Version stringOrInt `yaml:"version" help:"Client image version to pin. Only valid alongside inline dependencies."` + DockerImage *dockerImageConfig `yaml:"docker_image" help:"Custom image supplying the whole runtime. Not allowed alongside dependencies or version."` } func (e *environmentConfig) validate() error { @@ -304,7 +306,7 @@ func (s *stringOrInt) UnmarshalYAML(node *yaml.Node) error { // dockerImageConfig is environment.docker_image. type dockerImageConfig struct { - URL string `yaml:"url"` + URL string `yaml:"url" help:"Fully qualified image URL, e.g. myregistry.io/team/train:v3." required:"when environment.docker_image is set"` } func (d *dockerImageConfig) validate() error { @@ -316,8 +318,8 @@ func (d *dockerImageConfig) validate() error { // codeSourceConfig is the `code_source` block. Only the "snapshot" type exists. type codeSourceConfig struct { - Type string `yaml:"type"` - Snapshot *snapshotSourceConfig `yaml:"snapshot"` + Type string `yaml:"type" help:"Kind of code source. Must be \"snapshot\", the only supported type." required:"when code_source is set"` + Snapshot *snapshotSourceConfig `yaml:"snapshot" help:"Which local directory to archive and upload." required:"when code_source.type is \"snapshot\""` } func (c *codeSourceConfig) validate() error { @@ -332,10 +334,10 @@ func (c *codeSourceConfig) validate() error { // snapshotSourceConfig describes a local directory to tar and upload. type snapshotSourceConfig struct { - RootPath string `yaml:"root_path"` - RemoteVolume *string `yaml:"remote_volume"` - Git *gitRef `yaml:"git"` - IncludePaths []string `yaml:"include_paths"` + RootPath string `yaml:"root_path" help:"Local directory to archive, relative or absolute." required:"when code_source.snapshot is set"` + RemoteVolume *string `yaml:"remote_volume" help:"Volume to upload the archive to. Must start with /Volumes/."` + Git *gitRef `yaml:"git" help:"Pin the snapshot to a specific git revision."` + IncludePaths []string `yaml:"include_paths" help:"Restrict the archive to these paths, relative to root_path and without \"..\". Omit to include everything."` } func (s *snapshotSourceConfig) validate() error { @@ -374,9 +376,9 @@ func (s *snapshotSourceConfig) validate() error { // gitRef pins a snapshot to a specific git ref. branch and commit are mutually // exclusive; remote is only meaningful with branch. type gitRef struct { - Branch *string `yaml:"branch"` - Commit *string `yaml:"commit"` - Remote gitRemote `yaml:"remote"` + Branch *string `yaml:"branch" help:"Branch to pin to, resolved to its local HEAD. Mutually exclusive with commit." required:"one of branch or commit"` + Commit *string `yaml:"commit" help:"Commit to pin to. Mutually exclusive with branch." required:"one of branch or commit"` + Remote gitRemote `yaml:"remote" help:"No longer supported: the snapshot archives your local copy. Only false is accepted; use commit to pin a revision."` } func (g *gitRef) validate() error { @@ -434,12 +436,12 @@ func (r *gitRemote) truthy() bool { // permission is a DABs-compatible permission grant: exactly one principal plus a // level. type permission struct { - UserName *string `yaml:"user_name"` - GroupName *string `yaml:"group_name"` - ServicePrincipalName *string `yaml:"service_principal_name"` + UserName *string `yaml:"user_name" help:"Grant to this user, by email. Exactly one principal field per grant." required:"one principal per grant"` + GroupName *string `yaml:"group_name" help:"Grant to this group, by name. Exactly one principal field per grant." required:"one principal per grant"` + ServicePrincipalName *string `yaml:"service_principal_name" help:"Grant to this service principal, by name. Exactly one principal field per grant." required:"one principal per grant"` // Level is a databricks PermissionLevel (e.g. CAN_VIEW, CAN_MANAGE). Enum // membership is validated server-side; here we only require it to be set. - Level string `yaml:"level"` + Level string `yaml:"level" help:"Permission level to grant, e.g. CAN_VIEW or CAN_MANAGE. Validated server-side." required:"when a grant is listed"` } func (p *permission) validate() error { @@ -471,3 +473,300 @@ func (p *permission) validate() error { } return nil } + +// The remainder of this file implements `air run -h config.`, which +// documents the schema above from the struct tags themselves so the docs sit +// next to the validation rules that enforce them. + +// configHelpRoot is the prefix that addresses the run YAML schema in help +// output. Paths may also be given bare (`compute.url`). +const configHelpRoot = "config" + +// writeConfigFieldHelp resolves a dotted config path and writes its docs. +func writeConfigFieldHelp(w io.Writer, path string) error { + field, err := resolveConfigField(path) + if err != nil { + return err + } + renderConfigField(w, field) + return nil +} + +// freeFormConfigFields hold free-form maps, so path resolution stops at them: +// their keys are chosen by the user, not the schema. +var freeFormConfigFields = map[string]bool{ + "parameters": true, + "env_variables": true, + "secrets": true, +} + +// configTypeNames labels the polymorphic union types, whose accepted YAML shapes +// reflection cannot see: each is a struct of unexported fields populated by a +// custom UnmarshalYAML. +var configTypeNames = map[reflect.Type]string{ + reflect.TypeFor[dependencies](): "list of strings", + reflect.TypeFor[stringOrInt](): "string or int", + reflect.TypeFor[gitRemote](): "bool or string", +} + +// configField is one resolved schema field: where it sits, what it accepts, and +// what it is for. +type configField struct { + path string + typeName string + required string + help string + // children is non-empty for an object, whose sub-fields are listed instead + // of a type/description pair. + children []configField +} + +// resolveConfigField resolves a dotted YAML path against the run config schema. +// The leading "config." is optional; an empty path describes the whole schema. +func resolveConfigField(path string) (configField, error) { + trimmed := strings.TrimPrefix(strings.TrimPrefix(path, configHelpRoot), ".") + root := configField{ + path: configHelpRoot, + help: "The run YAML schema. Pass a field path for details, e.g. " + configHelpRoot + ".compute.accelerator_type.", + children: describeStruct(reflect.TypeFor[runConfig](), configHelpRoot), + } + if trimmed == "" { + return root, nil + } + + current := root + for i, part := range strings.Split(trimmed, ".") { + if len(current.children) == 0 { + // A free-form map's keys are chosen by the user, so the map itself is + // the most specific thing the schema can describe. Say so, rather + // than implying the key is misspelled. + if freeFormConfigFields[configLeafName(current.path)] { + return configField{}, fmt.Errorf("%q holds user-defined keys, so %q is not part of the schema; see %q instead", current.path, part, current.path) + } + return configField{}, fmt.Errorf("%q is not an object, so it has no field %q", current.path, part) + } + child, ok := findConfigChild(current.children, part) + if !ok { + return configField{}, unknownConfigFieldError(current, part, strings.Split(trimmed, ".")[:i+1]) + } + current = child + } + return current, nil +} + +// findConfigChild looks up an immediate child by its YAML name. +func findConfigChild(children []configField, name string) (configField, bool) { + for _, c := range children { + if configLeafName(c.path) == name { + return c, true + } + } + return configField{}, false +} + +// configLeafName returns the last segment of a dotted path. +func configLeafName(path string) string { + _, leaf, found := cutLast(path, ".") + if !found { + return path + } + return leaf +} + +// cutLast splits s around the final instance of sep. +func cutLast(s, sep string) (before, after string, found bool) { + i := strings.LastIndex(s, sep) + if i < 0 { + return s, "", false + } + return s[:i], s[i+len(sep):], true +} + +// unknownConfigFieldError reports an unresolvable segment, naming the valid +// siblings and, when one is close enough, a suggestion. +func unknownConfigFieldError(parent configField, part string, matched []string) error { + names := make([]string, 0, len(parent.children)) + for _, c := range parent.children { + names = append(names, configLeafName(c.path)) + } + slices.Sort(names) + + msg := fmt.Sprintf("unknown config field %q", configHelpRoot+"."+strings.Join(matched, ".")) + if suggestion, ok := closestConfigField(part, names); ok { + msg += fmt.Sprintf("; did you mean %q?", suggestion) + } + return fmt.Errorf("%s\n\nfields under %q are: %s", msg, parent.path, strings.Join(names, ", ")) +} + +// closestConfigField picks the nearest candidate by edit distance, requiring the +// match to be close enough that the suggestion is more helpful than noise. +func closestConfigField(name string, candidates []string) (string, bool) { + best, bestDist := "", 0 + for _, c := range candidates { + d := configEditDistance(name, c) + // Allow roughly a third of the name to differ, and always accept a + // single edit so short names still get a suggestion. + limit := max(len(c)/3, 1) + if d <= limit && (best == "" || d < bestDist) { + best, bestDist = c, d + } + } + return best, best != "" +} + +// configEditDistance computes the Levenshtein distance between two strings. +func configEditDistance(a, b string) int { + prev := make([]int, len(b)+1) + curr := make([]int, len(b)+1) + for j := range prev { + prev[j] = j + } + for i := 1; i <= len(a); i++ { + curr[0] = i + for j := 1; j <= len(b); j++ { + cost := 1 + if a[i-1] == b[j-1] { + cost = 0 + } + curr[j] = min(min(curr[j-1]+1, prev[j]+1), prev[j-1]+cost) + } + prev, curr = curr, prev + } + return prev[len(b)] +} + +// describeStruct reads a struct's yaml/help/required tags into configFields, +// recursing into nested objects. Fields are returned in declaration order, which +// is the order the schema and its validation errors already use. +func describeStruct(t reflect.Type, prefix string) []configField { + var out []configField + for f := range t.Fields() { + tag := f.Tag.Get("yaml") + if tag == "" || tag == "-" { + continue + } + name, _, _ := strings.Cut(tag, ",") + if name == "" || name == "-" { + continue + } + + field := configField{ + path: prefix + "." + name, + typeName: configTypeName(f.Type), + required: f.Tag.Get("required"), + help: f.Tag.Get("help"), + } + if nested := underlyingConfigStruct(f.Type); nested != nil && !freeFormConfigFields[name] { + field.children = describeStruct(nested, field.path) + } + out = append(out, field) + } + return out +} + +// configTypeName renders a field's YAML-facing type. +func configTypeName(t reflect.Type) string { + if name, ok := configTypeNames[t]; ok { + return name + } + for t.Kind() == reflect.Pointer { + t = t.Elem() + } + switch t.Kind() { + case reflect.String: + return "string" + case reflect.Int: + return "int" + case reflect.Bool: + return "bool" + case reflect.Slice: + return "list of " + configTypeName(t.Elem()) + case reflect.Map: + return fmt.Sprintf("map of %s to %s", configTypeName(t.Key()), configTypeName(t.Elem())) + case reflect.Struct: + return "object" + case reflect.Interface: + return "any" + default: + return t.Kind().String() + } +} + +// underlyingConfigStruct unwraps pointer/slice indirection and returns the struct +// type a field decodes into, or nil if it is not a struct. The polymorphic unions +// are excluded: they are structs, but their YAML shape is scalar or list. +func underlyingConfigStruct(t reflect.Type) reflect.Type { + if _, ok := configTypeNames[t]; ok { + return nil + } + for t.Kind() == reflect.Pointer || t.Kind() == reflect.Slice { + t = t.Elem() + } + if t.Kind() == reflect.Struct { + return t + } + return nil +} + +// renderConfigField writes a resolved field's documentation. An object lists its +// immediate children; a leaf gets its type, required-ness, and description. +func renderConfigField(w io.Writer, f configField) { + fmt.Fprintf(w, "%s\n", f.path) + if f.help != "" { + fmt.Fprintf(w, " %s\n", f.help) + } + + if len(f.children) == 0 { + fmt.Fprintf(w, "\n Type: %s\n", f.typeName) + required := f.required + if required == "" { + required = "no" + } + fmt.Fprintf(w, " Required: %s\n", required) + return + } + + // Pad the name column so the summaries line up. + width := 0 + for _, c := range f.children { + width = max(width, len(configLeafName(c.path))) + } + fmt.Fprintf(w, "\n Fields:\n") + for _, c := range f.children { + fmt.Fprintf(w, " %-*s %s\n", width, configLeafName(c.path), configFieldSummary(c)) + } + fmt.Fprintf(w, "\nUse \"-h %s.\" for details on a field.\n", f.path) +} + +// configFieldSummary is the one-line description used in a field listing: the +// first sentence of the help text, annotated when the field is required. +func configFieldSummary(f configField) string { + summary := firstSentence(f.help) + if f.required == "yes" { + summary = "(required) " + summary + } + return summary +} + +// sentenceAbbreviations end in a period that does not close a sentence, so +// firstSentence must not break on them. +var sentenceAbbreviations = []string{"e.g.", "i.e.", "etc.", "vs.", "cf."} + +// firstSentence returns s up to and including the first sentence-ending period, +// i.e. the first ". " boundary not immediately preceded by a known abbreviation. +// Returns s unchanged when it holds a single sentence. +func firstSentence(s string) string { + for i := 0; i+1 < len(s); i++ { + if s[i] != '.' || s[i+1] != ' ' { + continue + } + candidate := s[:i+1] + if slices.ContainsFunc(sentenceAbbreviations, func(a string) bool { + return strings.HasSuffix(candidate, a) + }) { + continue + } + return candidate + } + return s +} diff --git a/experimental/air/cmd/runconfig_test.go b/experimental/air/cmd/runconfig_test.go index 45cae7cd0ae..d4d4c6bc47e 100644 --- a/experimental/air/cmd/runconfig_test.go +++ b/experimental/air/cmd/runconfig_test.go @@ -414,3 +414,221 @@ func TestLoadRunConfig_FileErrors(t *testing.T) { assert.Contains(t, err.Error(), "is empty") }) } + +func TestResolveConfigField(t *testing.T) { + tests := []struct { + name string + path string + wantPath string + wantType string + wantRequired string + }{ + {"leaf", "config.compute.accelerator_type", "config.compute.accelerator_type", "string", ""}, + {"bare path", "compute.accelerator_type", "config.compute.accelerator_type", "string", ""}, + {"top-level required", "config.experiment_name", "config.experiment_name", "string", "yes"}, + {"int leaf", "config.max_retries", "config.max_retries", "int", ""}, + {"conditionally required", "config.environment.docker_image.url", "config.environment.docker_image.url", "string", "when environment.docker_image is set"}, + {"through a slice", "config.permissions.level", "config.permissions.level", "string", "when a grant is listed"}, + {"free-form map", "config.parameters", "config.parameters", "map of string to any", ""}, + {"deeply nested", "config.code_source.snapshot.root_path", "config.code_source.snapshot.root_path", "string", "when code_source.snapshot is set"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + field, err := resolveConfigField(tt.path) + require.NoError(t, err) + assert.Equal(t, tt.wantPath, field.path) + assert.Equal(t, tt.wantType, field.typeName) + assert.Equal(t, tt.wantRequired, field.required) + assert.NotEmpty(t, field.help) + assert.Empty(t, field.children) + }) + } +} + +// The union types are structs of unexported fields, so their type label comes +// from configTypeNames rather than reflection, and they must not be walked into. +func TestResolveConfigField_PolymorphicTypes(t *testing.T) { + tests := []struct{ path, wantType string }{ + {"config.environment.dependencies", "list of strings"}, + {"config.environment.version", "string or int"}, + {"config.code_source.snapshot.git.remote", "bool or string"}, + } + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + field, err := resolveConfigField(tt.path) + require.NoError(t, err) + assert.Equal(t, tt.wantType, field.typeName) + // A union must not be walked into as an object: its fields are unexported. + assert.Empty(t, field.children) + }) + } +} + +func TestResolveConfigField_Containers(t *testing.T) { + root, err := resolveConfigField("config") + require.NoError(t, err) + assert.Equal(t, "config", root.path) + // Children are listed in declaration order, matching the schema. + assert.Equal(t, "experiment_name", configLeafName(root.children[0].path)) + assert.Equal(t, "compute", configLeafName(root.children[1].path)) + + // An empty path describes the whole schema, so `-h config` and a bare + // prefix agree. + bare, err := resolveConfigField("") + require.NoError(t, err) + assert.Equal(t, root.children, bare.children) + + compute, err := resolveConfigField("config.compute") + require.NoError(t, err) + assert.Equal(t, "object", compute.typeName) + require.Len(t, compute.children, 2) +} + +func TestResolveConfigField_Errors(t *testing.T) { + tests := []struct { + name string + path string + wantParts []string + }{ + { + name: "typo suggests the near match", + path: "config.compute.acclerator_type", + wantParts: []string{`unknown config field "config.compute.acclerator_type"`, `did you mean "accelerator_type"?`, "accelerator_type, num_accelerators"}, + }, + { + name: "unknown top-level field lists siblings", + path: "config.bogus", + wantParts: []string{`unknown config field "config.bogus"`, "experiment_name"}, + }, + { + name: "no suggestion when nothing is close", + path: "config.zzzzzzzzzzzz", + wantParts: []string{`unknown config field "config.zzzzzzzzzzzz"`}, + }, + { + name: "free-form keys are not schema fields", + path: "config.parameters.learning_rate", + wantParts: []string{`"config.parameters" holds user-defined keys`, `"learning_rate" is not part of the schema`}, + }, + { + name: "scalar has no sub-fields", + path: "config.command.foo", + wantParts: []string{`"config.command" is not an object`, `no field "foo"`}, + }, + { + name: "nested typo reports the resolved prefix", + path: "config.code_source.snapshot.rootpath", + wantParts: []string{`unknown config field "config.code_source.snapshot.rootpath"`, `did you mean "root_path"?`}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := resolveConfigField(tt.path) + require.Error(t, err) + for _, part := range tt.wantParts { + assert.Contains(t, err.Error(), part) + } + }) + } + + // A distant name must not attract a suggestion. + _, err := resolveConfigField("config.zzzzzzzzzzzz") + require.Error(t, err) + assert.NotContains(t, err.Error(), "did you mean") +} + +func TestWriteConfigFieldHelp(t *testing.T) { + var leaf strings.Builder + require.NoError(t, writeConfigFieldHelp(&leaf, "config.compute.num_accelerators")) + assert.Contains(t, leaf.String(), "config.compute.num_accelerators") + assert.Contains(t, leaf.String(), "Type: int") + assert.Contains(t, leaf.String(), "Required: no") + + var container strings.Builder + require.NoError(t, writeConfigFieldHelp(&container, "config.compute")) + assert.Contains(t, container.String(), "Fields:") + assert.Contains(t, container.String(), "num_accelerators") + // A container lists its fields rather than printing a type/required pair. + assert.NotContains(t, container.String(), "Required:") + assert.Contains(t, container.String(), `Use "-h config.compute." for details`) + + // Required top-level fields are flagged in a listing. + var root strings.Builder + require.NoError(t, writeConfigFieldHelp(&root, "config")) + assert.Contains(t, root.String(), "(required) Which accelerators to run on") + + require.Error(t, writeConfigFieldHelp(&strings.Builder{}, "config.nope")) +} + +// Every schema field must document itself, so a new field cannot be added +// without help text. This is the same guard the bundle schema gets from its +// PLACEHOLDER convention in annotations.yml. +func TestConfigFieldsAllDocumented(t *testing.T) { + root, err := resolveConfigField("config") + require.NoError(t, err) + + var walk func(fields []configField) + walk = func(fields []configField) { + for _, f := range fields { + assert.NotEmpty(t, f.help, "%s is missing a help: struct tag", f.path) + assert.NotEmpty(t, f.typeName, "%s has no type name", f.path) + walk(f.children) + } + } + walk(root.children) +} + +func TestFirstSentence(t *testing.T) { + tests := []struct{ in, want string }{ + {"One sentence with no period", "One sentence with no period"}, + {"First. Second.", "First."}, + {"Which accelerator, e.g. GPU_1xA10. See the docs.", "Which accelerator, e.g. GPU_1xA10."}, + {"Use numpy, torch, etc. Then run.", "Use numpy, torch, etc. Then run."}, + {"Prod i.e. production. Details.", "Prod i.e. production."}, + {"Ends with abbreviation etc.", "Ends with abbreviation etc."}, + } + for _, tt := range tests { + assert.Equal(t, tt.want, firstSentence(tt.in)) + } +} + +// Plain -h must fall back to command help. A detached command has no parent to +// inherit a help function from, so this also covers that nil case. +func TestRunCommandHelp_NoConfigPath(t *testing.T) { + var out strings.Builder + cmd := newRunCommand() + cmd.SetOut(&out) + cmd.SetArgs([]string{"-h"}) + require.NoError(t, cmd.Execute()) + assert.Contains(t, out.String(), "Usage:") + assert.NotContains(t, out.String(), "Fields:") +} + +// -h with a config path documents the field instead of the command, and does so +// without the otherwise-required --file. +func TestRunCommandHelp_ConfigPath(t *testing.T) { + var out strings.Builder + cmd := newRunCommand() + cmd.SetOut(&out) + cmd.SetArgs([]string{"-h", "config.compute.accelerator_type"}) + require.NoError(t, cmd.Execute()) + assert.Contains(t, out.String(), "config.compute.accelerator_type") + assert.Contains(t, out.String(), "Type: string") + assert.NotContains(t, out.String(), "Usage:") +} + +// An unresolvable path reports to stderr and still exits cleanly, matching how +// cobra treats the help path. +func TestRunCommandHelp_UnknownConfigPath(t *testing.T) { + var out, errOut strings.Builder + cmd := newRunCommand() + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs([]string{"-h", "config.nope"}) + require.NoError(t, cmd.Execute()) + assert.Contains(t, errOut.String(), `unknown config field "config.nope"`) + assert.Empty(t, out.String()) +} From 56583f1415fde673ff6164dfe3d17ea4e42b0292 Mon Sep 17 00:00:00 2001 From: riddhibhagwat-db Date: Wed, 5 Aug 2026 00:09:49 +0000 Subject: [PATCH 2/4] [air] Sync config-help engine matrix with the air test tree --- acceptance/experimental/air/config-help/out.test.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/experimental/air/config-help/out.test.toml b/acceptance/experimental/air/config-help/out.test.toml index d6187dcb046..e90b6d5d1ba 100644 --- a/acceptance/experimental/air/config-help/out.test.toml +++ b/acceptance/experimental/air/config-help/out.test.toml @@ -1,3 +1,3 @@ Local = true Cloud = false -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] From ba8b42be54b2f281b5d1f9fe3536e39a7b9288c6 Mon Sep 17 00:00:00 2001 From: riddhibhagwat-db Date: Mon, 10 Aug 2026 17:57:01 +0000 Subject: [PATCH 3/4] [air] Unify the --override and config-help schema walkers `--override` path validation (runconfig_override.go) and `-h config.` (runconfig.go) each walked runConfig by reflection to resolve a dotted path. Collapse them onto one walk: describeStruct now records whether a field is a free-form map, and checkOverridePath resolves against the shared configSchema() tree instead of its own yamlFields/underlyingStruct/freeFormFields helpers, which are deleted. Behavior is unchanged. --override keeps its own error voice (the messages its tests pin), and a new test asserts both features resolve against the same schema so they can't drift apart again. --- experimental/air/cmd/runconfig.go | 31 +++++++--- experimental/air/cmd/runconfig_override.go | 72 ++++++---------------- experimental/air/cmd/runconfig_test.go | 28 +++++++++ 3 files changed, 68 insertions(+), 63 deletions(-) diff --git a/experimental/air/cmd/runconfig.go b/experimental/air/cmd/runconfig.go index 580a51c3b8d..8d05f6fd254 100644 --- a/experimental/air/cmd/runconfig.go +++ b/experimental/air/cmd/runconfig.go @@ -516,33 +516,43 @@ type configField struct { typeName string required string help string + // freeForm marks a map whose keys are user-defined (parameters, secrets, + // env_variables). It has no children, but a sub-path into it is still valid. + freeForm bool // children is non-empty for an object, whose sub-fields are listed instead // of a type/description pair. children []configField } -// resolveConfigField resolves a dotted YAML path against the run config schema. -// The leading "config." is optional; an empty path describes the whole schema. -func resolveConfigField(path string) (configField, error) { - trimmed := strings.TrimPrefix(strings.TrimPrefix(path, configHelpRoot), ".") - root := configField{ +// configSchema describes the whole run YAML schema as a tree of configFields. +// It is the single reflection walk over runConfig; both `-h config.` and +// --override path validation resolve against it. +func configSchema() configField { + return configField{ path: configHelpRoot, help: "The run YAML schema. Pass a field path for details, e.g. " + configHelpRoot + ".compute.accelerator_type.", children: describeStruct(reflect.TypeFor[runConfig](), configHelpRoot), } +} + +// resolveConfigField resolves a dotted YAML path against the run config schema. +// The leading "config." is optional; an empty path describes the whole schema. +func resolveConfigField(path string) (configField, error) { + trimmed := strings.TrimPrefix(strings.TrimPrefix(path, configHelpRoot), ".") + root := configSchema() if trimmed == "" { return root, nil } current := root for i, part := range strings.Split(trimmed, ".") { - if len(current.children) == 0 { + if current.freeForm { // A free-form map's keys are chosen by the user, so the map itself is // the most specific thing the schema can describe. Say so, rather // than implying the key is misspelled. - if freeFormConfigFields[configLeafName(current.path)] { - return configField{}, fmt.Errorf("%q holds user-defined keys, so %q is not part of the schema; see %q instead", current.path, part, current.path) - } + return configField{}, fmt.Errorf("%q holds user-defined keys, so %q is not part of the schema; see %q instead", current.path, part, current.path) + } + if len(current.children) == 0 { return configField{}, fmt.Errorf("%q is not an object, so it has no field %q", current.path, part) } child, ok := findConfigChild(current.children, part) @@ -655,8 +665,9 @@ func describeStruct(t reflect.Type, prefix string) []configField { typeName: configTypeName(f.Type), required: f.Tag.Get("required"), help: f.Tag.Get("help"), + freeForm: freeFormConfigFields[name], } - if nested := underlyingConfigStruct(f.Type); nested != nil && !freeFormConfigFields[name] { + if nested := underlyingConfigStruct(f.Type); nested != nil && !field.freeForm { field.children = describeStruct(nested, field.path) } out = append(out, field) diff --git a/experimental/air/cmd/runconfig_override.go b/experimental/air/cmd/runconfig_override.go index a00a178603b..e8f8930b5b0 100644 --- a/experimental/air/cmd/runconfig_override.go +++ b/experimental/air/cmd/runconfig_override.go @@ -3,8 +3,6 @@ package aircmd import ( "context" "fmt" - "maps" - "reflect" "slices" "strings" @@ -16,14 +14,6 @@ import ( // the parsed YAML map (not the typed runConfig) before re-decode, so one pipeline // covers path existence, type coercion, and the semantic validate() rules. -// freeFormFields hold free-form maps, so path validation stops at them: any -// sub-path is valid. -var freeFormFields = map[string]bool{ - "parameters": true, - "env_variables": true, - "secrets": true, -} - // parseOverrides parses --override KEY=VALUE arguments, preserving order. func parseOverrides(overrides []string) ([]overrideEntry, error) { entries := make([]overrideEntry, 0, len(overrides)) @@ -57,66 +47,42 @@ type overrideEntry struct { // before mutation, so an error names the exact --override key rather than the // re-decode's Go-type language. func validateOverridePaths(entries []overrideEntry) error { + schema := configSchema() for _, e := range entries { - if err := checkOverridePath(strings.Split(e.path, "."), reflect.TypeFor[runConfig](), e.path); err != nil { + if err := checkOverridePath(strings.Split(e.path, "."), schema, e.path); err != nil { return err } } return nil } -// checkOverridePath recursively validates one dotted path against a struct type -// whose fields carry `yaml:` tags. -func checkOverridePath(parts []string, t reflect.Type, fullPath string) error { - field := parts[0] - fields := yamlFields(t) - sub, ok := fields[field] +// checkOverridePath validates one dotted path against a resolved schema node. +// It shares configSchema()'s reflection walk with `-h config.` but keeps +// the --override error voice, which names the offending flag. +func checkOverridePath(parts []string, node configField, fullPath string) error { + name := parts[0] + child, ok := findConfigChild(node.children, name) if !ok { + names := make([]string, 0, len(node.children)) + for _, c := range node.children { + names = append(names, configLeafName(c.path)) + } + slices.Sort(names) return fmt.Errorf("invalid --override %q: %q is not a known field; available fields are: %s", - fullPath, field, strings.Join(slices.Sorted(maps.Keys(fields)), ", ")) + fullPath, name, strings.Join(names, ", ")) } if len(parts) == 1 { return nil } - if freeFormFields[field] { + // A free-form map's keys are user-defined, so any sub-path into it is valid. + if child.freeForm { return nil } - subStruct := underlyingStruct(sub) - if subStruct == nil { + if len(child.children) == 0 { return fmt.Errorf("invalid --override %q: %q is not a nested object; cannot address sub-field %q", - fullPath, field, strings.Join(parts[1:], ".")) + fullPath, name, strings.Join(parts[1:], ".")) } - return checkOverridePath(parts[1:], subStruct, fullPath) -} - -// yamlFields maps a struct's yaml tag names to their field types, skipping -// fields without a yaml tag (or tagged "-"). -func yamlFields(t reflect.Type) map[string]reflect.Type { - out := map[string]reflect.Type{} - for f := range t.Fields() { - tag := f.Tag.Get("yaml") - if tag == "" || tag == "-" { - continue - } - name, _, _ := strings.Cut(tag, ",") - if name == "" || name == "-" { - continue - } - out[name] = f.Type - } - return out -} - -// underlyingStruct unwraps pointer/slice indirection and returns the struct type -// a field decodes into, or nil if the field is not a struct (a scalar/map/etc.). -func underlyingStruct(t reflect.Type) reflect.Type { - for t.Kind() == reflect.Pointer || t.Kind() == reflect.Slice { - t = t.Elem() - } - if t.Kind() == reflect.Struct { - return t - } - return nil + return checkOverridePath(parts[1:], child, fullPath) } // applyOverrides walks each dotted path into the parsed YAML map and sets the diff --git a/experimental/air/cmd/runconfig_test.go b/experimental/air/cmd/runconfig_test.go index d4d4c6bc47e..b36828b3d8f 100644 --- a/experimental/air/cmd/runconfig_test.go +++ b/experimental/air/cmd/runconfig_test.go @@ -632,3 +632,31 @@ func TestRunCommandHelp_UnknownConfigPath(t *testing.T) { assert.Contains(t, errOut.String(), `unknown config field "config.nope"`) assert.Empty(t, out.String()) } + +// Both `-h config.` and --override path validation must resolve against +// the one configSchema() walk, so a field valid for one is valid for the other. +// This guards against the two features drifting apart again. +func TestConfigSchemaSharedByHelpAndOverride(t *testing.T) { + paths := []string{ + "compute.num_accelerators", + "environment.docker_image.url", + "code_source.snapshot.root_path", + "env_variables.MY_VAR", // free-form sub-path + } + for _, p := range paths { + t.Run(p, func(t *testing.T) { + require.NoError(t, validateOverridePaths([]overrideEntry{{path: p, raw: "x"}})) + // The help path accepts the same field (free-form sub-paths resolve to + // the map itself, which is the schema's most specific node). + _, err := resolveConfigField(p) + if !strings.Contains(p, "env_variables") { + require.NoError(t, err) + } + }) + } + + // A field unknown to one is unknown to the other. + require.Error(t, validateOverridePaths([]overrideEntry{{path: "compute.bogus", raw: "x"}})) + _, err := resolveConfigField("compute.bogus") + require.Error(t, err) +} From 512365f47ba02c6d57e161dd9dbda535c7d70f42 Mon Sep 17 00:00:00 2001 From: riddhibhagwat-db Date: Tue, 11 Aug 2026 21:21:48 +0000 Subject: [PATCH 4/4] [air] Tighten config-help comments --- experimental/air/cmd/run.go | 9 ++---- experimental/air/cmd/runconfig.go | 42 ++++++++++---------------- experimental/air/cmd/runconfig_test.go | 4 +-- 3 files changed, 20 insertions(+), 35 deletions(-) diff --git a/experimental/air/cmd/run.go b/experimental/air/cmd/run.go index aff2c1db8f5..c982d06fa7b 100644 --- a/experimental/air/cmd/run.go +++ b/experimental/air/cmd/run.go @@ -48,15 +48,12 @@ The path must be a separate argument: cobra reserves -h as a boolean, so -h=config.compute and -hconfig.compute are not accepted.`, } - // Document a config field instead of the command when -h is given a path. - // cobra hands the help function the positional args it collected, and it does - // so before Args and required-flag validation, so -f is not needed here. + // cobra passes -h's positional args to the help func before Args/required-flag + // validation, so a config path documents a field without needing -f. cmd.SetHelpFunc(func(c *cobra.Command, args []string) { fields := c.Flags().Args() if len(fields) == 0 { - // Resolved through the parent rather than captured up front, so this - // picks up the inherited help function instead of cobra's default. - // A detached command (as in unit tests) has no parent to inherit from. + // Parent() is nil for a detached command (unit tests). if parent := c.Parent(); parent != nil { parent.HelpFunc()(c, args) return diff --git a/experimental/air/cmd/runconfig.go b/experimental/air/cmd/runconfig.go index 8d05f6fd254..5a1d082abb3 100644 --- a/experimental/air/cmd/runconfig.go +++ b/experimental/air/cmd/runconfig.go @@ -474,12 +474,11 @@ func (p *permission) validate() error { return nil } -// The remainder of this file implements `air run -h config.`, which -// documents the schema above from the struct tags themselves so the docs sit -// next to the validation rules that enforce them. +// Below: `air run -h config.`, which documents the schema above from its +// yaml/help/required struct tags. -// configHelpRoot is the prefix that addresses the run YAML schema in help -// output. Paths may also be given bare (`compute.url`). +// configHelpRoot is the optional leading segment of a help path (`config.compute` +// or bare `compute`). const configHelpRoot = "config" // writeConfigFieldHelp resolves a dotted config path and writes its docs. @@ -500,33 +499,28 @@ var freeFormConfigFields = map[string]bool{ "secrets": true, } -// configTypeNames labels the polymorphic union types, whose accepted YAML shapes -// reflection cannot see: each is a struct of unexported fields populated by a -// custom UnmarshalYAML. +// configTypeNames labels the polymorphic unions, whose YAML shape reflection +// can't see (unexported fields filled by a custom UnmarshalYAML). var configTypeNames = map[reflect.Type]string{ reflect.TypeFor[dependencies](): "list of strings", reflect.TypeFor[stringOrInt](): "string or int", reflect.TypeFor[gitRemote](): "bool or string", } -// configField is one resolved schema field: where it sits, what it accepts, and -// what it is for. +// configField is one resolved node of the run config schema. type configField struct { path string typeName string required string help string - // freeForm marks a map whose keys are user-defined (parameters, secrets, - // env_variables). It has no children, but a sub-path into it is still valid. + // freeForm marks a user-keyed map (parameters/secrets/env_variables): no + // children, yet any sub-path into it is valid. freeForm bool - // children is non-empty for an object, whose sub-fields are listed instead - // of a type/description pair. - children []configField + children []configField // nil for a leaf } -// configSchema describes the whole run YAML schema as a tree of configFields. -// It is the single reflection walk over runConfig; both `-h config.` and -// --override path validation resolve against it. +// configSchema is the single reflection walk over runConfig; both +// `-h config.` and --override path validation resolve against it. func configSchema() configField { return configField{ path: configHelpRoot, @@ -547,9 +541,7 @@ func resolveConfigField(path string) (configField, error) { current := root for i, part := range strings.Split(trimmed, ".") { if current.freeForm { - // A free-form map's keys are chosen by the user, so the map itself is - // the most specific thing the schema can describe. Say so, rather - // than implying the key is misspelled. + // Keys are user-defined, so the map is the most specific node. return configField{}, fmt.Errorf("%q holds user-defined keys, so %q is not part of the schema; see %q instead", current.path, part, current.path) } if len(current.children) == 0 { @@ -608,8 +600,8 @@ func unknownConfigFieldError(parent configField, part string, matched []string) return fmt.Errorf("%s\n\nfields under %q are: %s", msg, parent.path, strings.Join(names, ", ")) } -// closestConfigField picks the nearest candidate by edit distance, requiring the -// match to be close enough that the suggestion is more helpful than noise. +// closestConfigField returns the nearest candidate by edit distance, if one is +// close enough to be worth suggesting. func closestConfigField(name string, candidates []string) (string, bool) { best, bestDist := "", 0 for _, c := range candidates { @@ -646,8 +638,7 @@ func configEditDistance(a, b string) int { } // describeStruct reads a struct's yaml/help/required tags into configFields, -// recursing into nested objects. Fields are returned in declaration order, which -// is the order the schema and its validation errors already use. +// recursing into nested objects. Declaration order matches validate()'s errors. func describeStruct(t reflect.Type, prefix string) []configField { var out []configField for f := range t.Fields() { @@ -737,7 +728,6 @@ func renderConfigField(w io.Writer, f configField) { return } - // Pad the name column so the summaries line up. width := 0 for _, c := range f.children { width = max(width, len(configLeafName(c.path))) diff --git a/experimental/air/cmd/runconfig_test.go b/experimental/air/cmd/runconfig_test.go index b36828b3d8f..fce77aacf03 100644 --- a/experimental/air/cmd/runconfig_test.go +++ b/experimental/air/cmd/runconfig_test.go @@ -563,9 +563,7 @@ func TestWriteConfigFieldHelp(t *testing.T) { require.Error(t, writeConfigFieldHelp(&strings.Builder{}, "config.nope")) } -// Every schema field must document itself, so a new field cannot be added -// without help text. This is the same guard the bundle schema gets from its -// PLACEHOLDER convention in annotations.yml. +// Guards against adding a schema field without a help: tag. func TestConfigFieldsAllDocumented(t *testing.T) { root, err := resolveConfigField("config") require.NoError(t, err)