feat: improve readability of error messages from ParseRawOn #1063

mfenniak · 2025-10-06T01:20:10Z

mfenniak commented

2025-10-06 01:20:10 +00:00

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: feat: improve readability of error messages from ParseRawOn

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

mfenniak added 1 commit

2025-10-06 01:20:10 +00:00

feat: improve readability of error messages from ParseRawOn

checks / validate pre-commit-hooks file (pull_request) Successful in 32s

Details

checks / validate mocks (pull_request) Successful in 39s

Details

checks / build and test (pull_request) Successful in 49s

Details

checks / runner exec tests (pull_request) Successful in 24s

Details

checks / runner integration tests (pull_request) Successful in 4m10s

Details

checks / integration tests (pull_request) Successful in 10m5s

Details

issue-labels / release-notes (pull_request_target) Successful in 7s

Details

cascade / debug (pull_request_target) Has been skipped

Details

cascade / end-to-end (pull_request_target) Successful in 5s

Details

cascade / forgejo (pull_request_target) Successful in 1m25s

Details

cf47e20832

mfenniak reviewed

2025-10-06 01:30:16 +00:00

act/jobparser/model.go

					
				@ -272,4 +267,0 @@

								res = append(res, &Event{

									Name: k,

									acts: map[string][]string{},

								})

These switch cases aren't covered by any tests, don't match any documented supported on block, and don't actually use the value t.

The first case string would represent if someone had this in their workflow...

on:
   push: abc

But it's going to act identically to:

on: push

The second case []string would represent...

on:
    push: [string1, string2]

But it's going to act identically to:

on: push

This change validates the expected options a little more firmly which may make it a breaking change, but it would highlight any misconfiguration... if someone believed push: abc was filtering to the branch abc they would have unexpected behaviour currently.

I'm 50/50 on removing them? Maybe it's just not in the scope of this change? I just couldn't test them which is why it felt like they needed to be removed.

These `switch` cases aren't covered by any tests, don't match any documented supported `on` block, and don't actually *use* the value `t`. The first case `string` would represent if someone had this in their workflow... ``` on: push: abc ``` But it's going to act identically to: ``` on: push ``` The second case `[]string` would represent... ``` on: push: [string1, string2] ``` But it's going to act identically to: ``` on: push ``` This change validates the expected options a little more firmly which may make it a breaking change, but it would highlight any misconfiguration... if someone believed `push: abc` was filtering to the branch abc they would have unexpected behaviour currently. I'm 50/50 on removing them? Maybe it's just not in the scope of this change? I just couldn't test them which is why it felt like they needed to be removed.

I am OK to remove them.

I also have in the back of my mind a "weird behavior" red flag because I ran into unexplained behavior in the past. Until I keep using the exact same pattern for all on: to avoid discovering bugs.

I suspect a lot of people used the same "stick to the happy path" method.

The schema validation could catch push: abc as being incorrect, but I'm unclear if that fits. The many variations of on: structure makes it not the most amicable specification for static checking.

I am OK to remove them. I also have in the back of my mind a "weird behavior" red flag because I ran into unexplained behavior in the past. Until I keep using the exact same pattern for all `on:` to avoid discovering bugs. I suspect a lot of people used the same "stick to the happy path" method. The schema validation could catch `push: abc` as being incorrect, but I'm unclear if that fits. The many variations of `on:` structure makes it not the most amicable specification for static checking.