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.

<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- features
  - [PR](https://code.forgejo.org/forgejo/runner/pulls/1063): <!--number 1063 --><!--line 0 --><!--description ZmVhdDogaW1wcm92ZSByZWFkYWJpbGl0eSBvZiBlcnJvciBtZXNzYWdlcyBmcm9tIFBhcnNlUmF3T24=-->feat: improve readability of error messages from ParseRawOn<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/1063
Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

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) {
+						if err := isInvalidOnType(k, act); err != nil {
-							return nil, fmt.Errorf("unknown on type: %#v", v)
+							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 {
+						if schedules[i][kk], ok = vvv.(string); !ok {
-							return nil, fmt.Errorf("unknown on type: %#v", v)
+							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 {
+func isInvalidOnType(onType, subKey string) error {
-	if onType == "workflow_dispatch" && subKey == "inputs" {
+	if onType == "workflow_dispatch" {
-		return false
+		if subKey == "inputs" {
+			return nil
 		}
-	if onType == "workflow_call" && (subKey == "inputs" || subKey == "outputs") {
+		return fmt.Errorf("workflow_dispatch only supports key \"inputs\", but key %q was found", subKey)
-		return false
 	}
-	return true
+	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 fmt.Errorf("unexpected key %q.%q", onType, subKey)
 }
 
 // parseMappingNode parse a mapping node and preserve order.

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)
+			if kase.err != "" {
+				assert.ErrorContains(t, err, kase.err)
+			} else {
 				assert.NoError(t, err)
 				assert.EqualValues(t, kase.result, events, fmt.Sprintf("%#v", events))
+			}
 		})
 	}
 }