From 3f52c56d1e24c31297290eacfd035fb703e6b810 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Tue, 7 Oct 2025 14:41:55 +0000 Subject: [PATCH] feat: improve readability of error messages from ParseRawOn (#1063) With https://codeberg.org/forgejo/forgejo/pulls/9530, the error messages from `ParseRawOn` are user-facing and need a pass to improve their meaning. - features - [PR](https://code.forgejo.org/forgejo/runner/pulls/1063): feat: improve readability of error messages from ParseRawOn Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/1063 Reviewed-by: earl-warren Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- act/jobparser/model.go | 60 ++++++++++++++++----------------- act/jobparser/model_test.go | 67 ++++++++++++++++++++++++++++++++++--- 2 files changed, 93 insertions(+), 34 deletions(-) diff --git a/act/jobparser/model.go b/act/jobparser/model.go index e94123e3..74de0450 100644 --- a/act/jobparser/model.go +++ b/act/jobparser/model.go @@ -2,6 +2,7 @@ package jobparser import ( "fmt" + "strings" "code.forgejo.org/forgejo/runner/v11/act/model" "go.yaml.in/yaml/v3" @@ -226,7 +227,7 @@ func ParseRawOn(rawOn *yaml.Node) ([]*Event, error) { var val string err := rawOn.Decode(&val) if err != nil { - return nil, err + return nil, fmt.Errorf("unable to interpret scalar value into a string: %w", err) } return []*Event{ {Name: val}, @@ -238,12 +239,12 @@ func ParseRawOn(rawOn *yaml.Node) ([]*Event, error) { return nil, err } res := make([]*Event, 0, len(val)) - for _, v := range val { + for i, v := range val { switch t := v.(type) { case string: res = append(res, &Event{Name: t}) default: - return nil, fmt.Errorf("invalid type %T", t) + return nil, fmt.Errorf("value at index %d was unexpected type %[2]T; must be a string but was %#[2]v", i, v) } } return res, nil @@ -263,16 +264,6 @@ func ParseRawOn(rawOn *yaml.Node) ([]*Event, error) { continue } switch t := v.(type) { - case string: - res = append(res, &Event{ - Name: k, - acts: map[string][]string{}, - }) - case []string: - res = append(res, &Event{ - Name: k, - acts: map[string][]string{}, - }) case map[string]any: acts := make(map[string][]string, len(t)) for act, branches := range t { @@ -286,15 +277,15 @@ func ParseRawOn(rawOn *yaml.Node) ([]*Event, error) { for i, v := range b { var ok bool if acts[act][i], ok = v.(string); !ok { - return nil, fmt.Errorf("unknown on type: %#v", branches) + return nil, fmt.Errorf("key %q.%q index %d had unexpected type %[4]T; a string was expected but was %#[4]v", k, act, i, v) } } case map[string]any: - if isInvalidOnType(k, act) { - return nil, fmt.Errorf("unknown on type: %#v", v) + if err := isInvalidOnType(k, act); err != nil { + return nil, fmt.Errorf("invalid value on key %q: %w", k, err) } default: - return nil, fmt.Errorf("unknown on type: %#v", branches) + return nil, fmt.Errorf("key %q.%q had unexpected type %T; was %#v", k, act, branches, branches) } } if k == "workflow_dispatch" || k == "workflow_call" { @@ -306,19 +297,22 @@ func ParseRawOn(rawOn *yaml.Node) ([]*Event, error) { }) case []any: if k != "schedule" { - return nil, fmt.Errorf("unknown on type: %#v", v) + return nil, fmt.Errorf("key %q had an type %T; only the 'schedule' key is expected with this type", k, v) } schedules := make([]map[string]string, len(t)) for i, tt := range t { vv, ok := tt.(map[string]any) if !ok { - return nil, fmt.Errorf("unknown on type: %#v", v) + return nil, fmt.Errorf("key %q[%d] had unexpected type %[3]T; a map with a key \"cron\" was expected, but value was %#[3]v", k, i, tt) } schedules[i] = make(map[string]string, len(vv)) - for k, vvv := range vv { + for kk, vvv := range vv { + if strings.ToLower(kk) != "cron" { + return nil, fmt.Errorf("key %q[%d] had unexpected key %q; \"cron\" was expected", k, i, kk) + } var ok bool - if schedules[i][k], ok = vvv.(string); !ok { - return nil, fmt.Errorf("unknown on type: %#v", v) + if schedules[i][kk], ok = vvv.(string); !ok { + return nil, fmt.Errorf("key %q[%d].%q had unexpected type %[4]T; a string was expected by was %#[4]v", k, i, kk, vvv) } } } @@ -327,23 +321,29 @@ func ParseRawOn(rawOn *yaml.Node) ([]*Event, error) { schedules: schedules, }) default: - return nil, fmt.Errorf("unknown on type: %#v", v) + return nil, fmt.Errorf("key %q had unexpected type %[2]T; expected a map or array but was %#[2]v", k, v) } } return res, nil default: - return nil, fmt.Errorf("unknown on type: %v", rawOn.Kind) + return nil, fmt.Errorf("unexpected yaml node in `on`: %v", rawOn.Kind) } } -func isInvalidOnType(onType, subKey string) bool { - if onType == "workflow_dispatch" && subKey == "inputs" { - return false +func isInvalidOnType(onType, subKey string) error { + if onType == "workflow_dispatch" { + if subKey == "inputs" { + return nil + } + return fmt.Errorf("workflow_dispatch only supports key \"inputs\", but key %q was found", subKey) } - if onType == "workflow_call" && (subKey == "inputs" || subKey == "outputs") { - return false + if onType == "workflow_call" { + if subKey == "inputs" || subKey == "outputs" { + return nil + } + return fmt.Errorf("workflow_call only supports keys \"inputs\" and \"outputs\", but key %q was found", subKey) } - return true + return fmt.Errorf("unexpected key %q.%q", onType, subKey) } // parseMappingNode parse a mapping node and preserve order. diff --git a/act/jobparser/model_test.go b/act/jobparser/model_test.go index 45d77428..454de71c 100644 --- a/act/jobparser/model_test.go +++ b/act/jobparser/model_test.go @@ -16,6 +16,7 @@ func TestParseRawOn(t *testing.T) { kases := []struct { input string result []*Event + err string }{ { input: "on: issue_comment", @@ -33,7 +34,10 @@ func TestParseRawOn(t *testing.T) { }, }, }, - + { + input: "on: [123]", + err: "value at index 0 was unexpected type int; must be a string but was 123", + }, { input: "on:\n - push\n - pull_request", result: []*Event{ @@ -45,6 +49,19 @@ func TestParseRawOn(t *testing.T) { }, }, }, + { + input: "on: { push: null }", + result: []*Event{ + { + Name: "push", + acts: map[string][]string{}, + }, + }, + }, + { + input: "on: { push: 'abc' }", + err: "key \"push\" had unexpected type string; expected a map or array but was \"abc\"", + }, { input: "on:\n push:\n branches:\n - master", result: []*Event{ @@ -72,6 +89,10 @@ func TestParseRawOn(t *testing.T) { }, }, }, + { + input: "on:\n branch_protection_rule:\n types: [123, deleted]", + err: "key \"branch_protection_rule\".\"types\" index 0 had unexpected type int; a string was expected but was 123", + }, { input: "on:\n project:\n types: [created, deleted]\n milestone:\n types: [opened, deleted]", result: []*Event{ @@ -189,6 +210,22 @@ func TestParseRawOn(t *testing.T) { }, }, }, + { + input: "on:\n schedule2:\n - cron: '20 6 * * *'", + err: "key \"schedule2\" had an type []interface {}; only the 'schedule' key is expected with this type", + }, + { + input: "on:\n schedule:\n - 123", + err: "key \"schedule\"[0] had unexpected type int; a map with a key \"cron\" was expected, but value was 123", + }, + { + input: "on:\n schedule:\n - corn: '20 6 * * *'", + err: "key \"schedule\"[0] had unexpected key \"corn\"; \"cron\" was expected", + }, + { + input: "on:\n schedule:\n - cron: 123", + err: "key \"schedule\"[0].\"cron\" had unexpected type int; a string was expected by was 123", + }, { input: ` on: @@ -222,15 +259,37 @@ on: }, }, }, + { + input: ` +on: + workflow_call: + mistake: + access-token: + description: 'A token passed from the caller workflow' + required: false +`, + err: "invalid value on key \"workflow_call\": workflow_call only supports keys \"inputs\" and \"outputs\", but key \"mistake\" was found", + }, + { + input: ` +on: + workflow_call: { map: 123 } +`, + err: "key \"workflow_call\".\"map\" had unexpected type int; was 123", + }, } for _, kase := range kases { t.Run(kase.input, func(t *testing.T) { origin, err := model.ReadWorkflow(strings.NewReader(kase.input), false) - assert.NoError(t, err) + require.NoError(t, err) events, err := ParseRawOn(&origin.RawOn) - assert.NoError(t, err) - assert.EqualValues(t, kase.result, events, fmt.Sprintf("%#v", events)) + if kase.err != "" { + assert.ErrorContains(t, err, kase.err) + } else { + assert.NoError(t, err) + assert.EqualValues(t, kase.result, events, fmt.Sprintf("%#v", events)) + } }) } }