diff --git a/appview/pages/templates/repo/pipelines/workflow.html b/appview/pages/templates/repo/pipelines/workflow.html index 4d6895e5..84d20014 100644 --- a/appview/pages/templates/repo/pipelines/workflow.html +++ b/appview/pages/templates/repo/pipelines/workflow.html @@ -26,6 +26,14 @@ >Cancel {{ end }} + {{ with (index .Pipeline.Statuses .Workflow).Latest }} + {{ if .Error }} +
+ {{ i "triangle-alert" "size-4 shrink-0 mt-0.5" }} + {{- .Error -}} +
+ {{ end }} + {{ end }} {{ block "logs" . }} {{ end }} diff --git a/spindle/engine/manifest.go b/spindle/engine/manifest.go new file mode 100644 index 00000000..2cde8694 --- /dev/null +++ b/spindle/engine/manifest.go @@ -0,0 +1,232 @@ +package engine + +import ( + "errors" + "fmt" + "reflect" + "strconv" + "strings" + + "gopkg.in/yaml.v3" + "tangled.org/core/workflow" +) + +// how many lines of context to show on above / below of an offending line. +const frameContext = 3 + +type manifestError struct { + line int + msg string +} + +func (e *manifestError) Error() string { return e.msg } + +// codeFrame renders the lines around `line` with a gutter and a `>` marker on +// the offending line, eg. +// +// 4 | image: alpine +// > 5 | registre: +// 6 | nixpkgs: github:nixos/nixpkgs/nixos-unstable +func codeFrame(raw string, line int) string { + lines := strings.Split(raw, "\n") + if line < 1 || line > len(lines) { + return "" + } + start := max(line-frameContext, 1) + end := min(line+frameContext, len(lines)) + width := len(strconv.Itoa(end)) + + var b strings.Builder + for n := start; n <= end; n++ { + marker := " " + if n == line { + marker = "> " + } + fmt.Fprintf(&b, "%s%*d | %s\n", marker, width, n, lines[n-1]) + } + return strings.TrimRight(b.String(), "\n") +} + +var genericWorkflowKeys = ignoredKeys(reflect.TypeFor[workflow.Workflow]()) + +// ignoredKeys is the set of yaml keys we ignore on field checks for a struct. +// real, parseable keys come straight from the tags (via fieldsByYAMLName); on +// top of those we tolerate `yaml:"-"` fields by their conventional spelling. +// those have no yaml key of their own (the program fills them in itself, eg. +// `name` from the filename, `raw` from the file bytes), but users sometimes +// write one in the body anyway, and that's harmless rather than a typo. +func ignoredKeys(t reflect.Type) map[string]bool { + if t.Kind() == reflect.Pointer { + t = t.Elem() + } + keys := make(map[string]bool) + for k := range fieldsByYAMLName(t) { + keys[k] = true + } + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if tag, _, _ := strings.Cut(f.Tag.Get("yaml"), ","); tag == "-" { + keys[strings.ToLower(f.Name)] = true + } + } + return keys +} + +// this exists because yaml.v3 reports mismatches as "cannot unmarshal !!seq into +// map[string]interface {}", which is kind of confusing, even if it outputs a line. +// so we use reflection, walk the node tree alongside the schema type, and point +// at the field that's actually mis-shaped. +// +// returns nil when nothing is structurally wrong. +func DescribeManifestError(raw string, schema any) error { + var doc yaml.Node + if err := yaml.Unmarshal([]byte(raw), &doc); err != nil { + return nil + } + if len(doc.Content) == 0 { + return nil + } + err := checkNode(doc.Content[0], reflect.TypeOf(schema), "", genericWorkflowKeys) + var me *manifestError + if !errors.As(err, &me) { + return err // nil + } + if frame := codeFrame(raw, me.line); frame != "" { + return fmt.Errorf("%s\n\n%s", me.msg, frame) + } + return errors.New(me.msg) +} + +// checkNode walks a yaml node against the type it's expected to decode into, +// recursing through structs, maps and slices. allowExtra names keys that are +// valid at this level despite not being in the struct (only the root uses it). +func checkNode(node *yaml.Node, t reflect.Type, path string, allowExtra map[string]bool) error { + if node.Kind == yaml.AliasNode && node.Alias != nil { + node = node.Alias + } + if t == nil { + return nil + } + if t.Kind() == reflect.Pointer { + t = t.Elem() + } + // `any` accepts anything (eg. registry values) so we can't check more + if t.Kind() == reflect.Interface { + return nil + } + // an empty value (eg. `registry:` with nothing under it) is harmless + if node.Kind == yaml.ScalarNode && (node.Tag == "!!null" || node.Value == "") { + return nil + } + + want, ok := yamlKindForType(t) + if !ok { + return nil + } + if node.Kind != want { + return &manifestError{line: node.Line, msg: fmt.Sprintf( + "%s must be %s, but got %s (line %d)", + describePath(path), yamlKindName(want), yamlKindName(node.Kind), node.Line)} + } + + switch t.Kind() { + case reflect.Struct: + fields := fieldsByYAMLName(t) + for i := 0; i+1 < len(node.Content); i += 2 { + key, val := node.Content[i], node.Content[i+1] + ft, ok := fields[key.Value] + if !ok { + // a struct has a fixed set of fields, so anything else is a typo. + // (maps, take arbitrary user-defined keys and don't count) + if allowExtra[key.Value] { + continue + } + return &manifestError{line: key.Line, msg: fmt.Sprintf( + "unknown field %s (line %d)", + describePath(joinKey(path, key.Value)), key.Line)} + } + if err := checkNode(val, ft, joinKey(path, key.Value), nil); err != nil { + return err + } + } + case reflect.Map: + for i := 0; i+1 < len(node.Content); i += 2 { + key, val := node.Content[i], node.Content[i+1] + if err := checkNode(val, t.Elem(), joinKey(path, key.Value), nil); err != nil { + return err + } + } + case reflect.Slice, reflect.Array: + for idx, val := range node.Content { + if err := checkNode(val, t.Elem(), fmt.Sprintf("%s[%d]", path, idx), nil); err != nil { + return err + } + } + } + return nil +} + +// fieldsByYAMLName maps a struct's yaml keys to their field types, mirroring how +// yaml.v3 resolves keys: explicit tag name, else the lowercased field name. +func fieldsByYAMLName(t reflect.Type) map[string]reflect.Type { + fields := make(map[string]reflect.Type) + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + name, _, _ := strings.Cut(f.Tag.Get("yaml"), ",") + if name == "-" { + continue + } + if name == "" { + name = strings.ToLower(f.Name) + } + fields[name] = f.Type + } + return fields +} + +func joinKey(path, key string) string { + if path == "" { + return key + } + return path + "." + key +} + +func describePath(path string) string { + if path == "" { + return "the manifest" + } + return "`" + path + "`" +} + +func yamlKindForType(t reflect.Type) (yaml.Kind, bool) { + switch t.Kind() { + case reflect.Pointer: + return yamlKindForType(t.Elem()) + case reflect.Map, reflect.Struct: + return yaml.MappingNode, true + case reflect.Slice, reflect.Array: + return yaml.SequenceNode, true + case reflect.String, reflect.Bool, + reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return yaml.ScalarNode, true + default: + return 0, false + } +} + +func yamlKindName(k yaml.Kind) string { + switch k { + case yaml.MappingNode: + return "a mapping" + case yaml.SequenceNode: + return "a list" + case yaml.ScalarNode: + return "a scalar value" + case yaml.AliasNode: + return "an alias" + default: + return "an unknown value" + } +} diff --git a/spindle/engine/manifest_test.go b/spindle/engine/manifest_test.go new file mode 100644 index 00000000..cb086bf0 --- /dev/null +++ b/spindle/engine/manifest_test.go @@ -0,0 +1,112 @@ +package engine + +import ( + "strings" + "testing" +) + +type testManifest struct { + Image string `yaml:"image"` + Registry map[string]any `yaml:"registry"` + Dependencies []string `yaml:"dependencies"` + Nested map[string][]string `yaml:"nested"` + Steps []struct { + Name string `yaml:"name"` + } `yaml:"steps"` +} + +func TestDescribeManifestError(t *testing.T) { + cases := []struct { + name string + raw string + want []string // substrings the message must contain + }{ + { + name: "map field written as list", + raw: "registry:\n - nixpkgs: github:nixos/nixpkgs\n", + want: []string{"registry", "a mapping", "a list"}, + }, + { + name: "list field written as scalar", + raw: "dependencies: bun\n", + want: []string{"dependencies", "a list", "a scalar value"}, + }, + { + name: "scalar field written as mapping", + raw: "image:\n name: nixos\n", + want: []string{"image", "a scalar value", "a mapping"}, + }, + { + name: "nested map value mis-shaped", + raw: "nested:\n foo: bar\n", // foo should be a list of strings + want: []string{"nested.foo", "a list", "a scalar value"}, + }, + { + name: "field inside a list element mis-shaped", + raw: "steps:\n - name:\n x: y\n", // steps[0].name should be a scalar + want: []string{"steps[0].name", "a scalar value", "a mapping"}, + }, + { + name: "unknown top-level field (typo)", + raw: "dependancies:\n - bun\n", + want: []string{"unknown field", "dependancies"}, + }, + { + name: "unknown field inside a list element", + raw: "steps:\n - name: x\n cmd: y\n", // it's `command`, not `cmd` + want: []string{"unknown field", "steps[0].cmd"}, + }, + { + // `name` (filename-sourced, yaml:"-") must be tolerated so the real + // typo `registre` on a later line is the thing that surfaces. + name: "tolerated name does not mask a later typo", + raw: "name: mill\nengine: microvm\nregistre:\n - x: y\n", + want: []string{"unknown field", "registre"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := DescribeManifestError(tc.raw, testManifest{}) + if err == nil { + t.Fatalf("expected an error, got nil") + } + for _, w := range tc.want { + if !strings.Contains(err.Error(), w) { + t.Errorf("error %q missing %q", err, w) + } + } + }) + } +} + +func TestDescribeManifestErrorNoFalsePositives(t *testing.T) { + cases := []string{ + // well-formed manifest + "image: nixos\nregistry:\n nixpkgs: github:nixos/nixpkgs\ndependencies:\n - bun\n", + // empty value is harmless, not a mismatch + "image: nixos\nregistry:\n", + // generic workflow keys live in the same doc and aren't engine fields, + // but must not be flagged as unknown at the root + "engine: microvm\nwhen:\n - event: [push]\nclone:\n skip: true\nimage: nixos\n", + // `any` map values accept any shape, including nested lists/maps + "registry:\n k:\n - a\n - b\n", + // well-formed nested map-of-lists + "nested:\n foo:\n - a\n - b\n", + // user-defined map keys are data, never flagged as unknown fields + "nested:\n any-package-name:\n - a\n", + } + for _, raw := range cases { + if err := DescribeManifestError(raw, testManifest{}); err != nil { + t.Errorf("DescribeManifestError(%q) = %v, want nil", raw, err) + } + } +} + +func TestDescribeManifestErrorPointerSchema(t *testing.T) { + // nixery passes a pointer to an anonymous struct + schema := &testManifest{} + err := DescribeManifestError("nested: oops\n", schema) + if err == nil || !strings.Contains(err.Error(), "nested") { + t.Fatalf("expected an error naming `nested`, got %v", err) + } +} diff --git a/spindle/engines/microvm/engine.go b/spindle/engines/microvm/engine.go index 5dfe3ebb..434ccd13 100644 --- a/spindle/engines/microvm/engine.go +++ b/spindle/engines/microvm/engine.go @@ -104,6 +104,9 @@ func (e *Engine) InitWorkflow(twf tangled.Pipeline_Workflow, tpl tangled.Pipelin swf := &models.Workflow{} var dwf manifestWorkflow + if err := engine.DescribeManifestError(twf.Raw, manifestWorkflow{}); err != nil { + return nil, err + } if err := yaml.Unmarshal([]byte(twf.Raw), &dwf); err != nil { return nil, err } diff --git a/spindle/engines/nixery/engine.go b/spindle/engines/nixery/engine.go index 07ca225b..84dd6908 100644 --- a/spindle/engines/nixery/engine.go +++ b/spindle/engines/nixery/engine.go @@ -91,8 +91,10 @@ func (e *Engine) InitWorkflow(twf tangled.Pipeline_Workflow, tpl tangled.Pipelin Dependencies map[string][]string `yaml:"dependencies"` Environment map[string]string `yaml:"environment"` }{} - err := yaml.Unmarshal([]byte(twf.Raw), &dwf) - if err != nil { + if err := engine.DescribeManifestError(twf.Raw, dwf); err != nil { + return nil, err + } + if err := yaml.Unmarshal([]byte(twf.Raw), &dwf); err != nil { return nil, err } diff --git a/spindle/server.go b/spindle/server.go index 82cbdf95..df434074 100644 --- a/spindle/server.go +++ b/spindle/server.go @@ -427,6 +427,8 @@ func (s *Spindle) processPipeline(ctx context.Context, src eventconsumer.Source, for _, w := range tpl.Workflows { if w != nil { if _, ok := s.engs[w.Engine]; !ok { + s.l.Error("workflow failed: unknown engine", + "pipeline", pipelineId, "workflow", w.Name, "engine", w.Engine) err = s.db.StatusFailed(models.WorkflowId{ PipelineId: pipelineId, Name: w.Name, @@ -446,6 +448,8 @@ func (s *Spindle) processPipeline(ctx context.Context, src eventconsumer.Source, ewf, err := s.engs[w.Engine].InitWorkflow(*w, tpl) if err != nil { + s.l.Error("workflow failed: init workflow", + "pipeline", pipelineId, "workflow", w.Name, "engine", w.Engine, "err", err) err = s.db.StatusFailed(models.WorkflowId{ PipelineId: pipelineId, Name: w.Name,