fix: runs-on is also mandatory for reusable workflows and may be needing container #194

earl-warren · 2025-07-25T09:43:50Z

earl-warren commented

2025-07-25 09:43:50 +00:00

If not the schema validation will fail because it will be try to validate as if not calling a reusable workflow.

=== RUN   TestWorkflowCallRunsOn
    schema_test.go:119:
        	Error Trace:	/home/earl-warren/software/act/pkg/schema/schema_test.go:119
        	Error:      	Received unexpected error:
        	            	Line: 10 Column 5: Failed to match job-factory: Line: 12 Column 5: Unknown Property uses
        	            	Line: 13 Column 5: Unknown Property with
        	            	Line: 15 Column 5: Unknown Property secrets
        	            	Line: 10 Column 5: Failed to match workflow-job: Line: 11 Column 5: Unknown Property runs-on
        	Test:       	TestWorkflowCallRunsOn

If not the schema validation will fail because it will be try to validate as if not calling a reusable workflow. ``` === RUN TestWorkflowCallRunsOn schema_test.go:119: Error Trace: /home/earl-warren/software/act/pkg/schema/schema_test.go:119 Error: Received unexpected error: Line: 10 Column 5: Failed to match job-factory: Line: 12 Column 5: Unknown Property uses Line: 13 Column 5: Unknown Property with Line: 15 Column 5: Unknown Property secrets Line: 10 Column 5: Failed to match workflow-job: Line: 11 Column 5: Unknown Property runs-on Test: TestWorkflowCallRunsOn ```

earl-warren added the

Kind/Bug

label

2025-07-25 09:43:50 +00:00

earl-warren self-assigned this

2025-07-25 09:43:53 +00:00

earl-warren changed title from ~~fix: runs-on is also mandatory for reusable workflows [skip cascade]~~ to fix: runs-on is also mandatory for reusable workflows

2025-07-25 09:48:09 +00:00

earl-warren force-pushed wip-workflow-call from 364e2167ce

/ cascade (pull_request_target) Has been skipped

Details

checks / unit (pull_request) Successful in 2m30s

Details

checks / integration (pull_request) Successful in 1m47s

Details

to 50c850d757

/ cascade (pull_request_target) Successful in 1m31s

Details

checks / unit (pull_request) Successful in 1m32s

Details

checks / integration (pull_request) Successful in 2m53s

Details

2025-07-25 09:49:59 +00:00

Compare

cascading-pr referenced this pull request from forgejo/runner

2025-07-25 09:50:53 +00:00

cascading-pr from https://code.forgejo.org/forgejo/act refs/pull/194/head to forgejo/act-194 #732

cascading-pr commented

2025-07-25 09:50:54 +00:00

cascading-pr updated at forgejo/runner#732

cascading-pr updated at https://code.forgejo.org/forgejo/runner/pulls/732

cascading-pr commented

2025-07-25 10:20:21 +00:00

cascading-pr updated at forgejo/runner#732

cascading-pr updated at https://code.forgejo.org/forgejo/runner/pulls/732

earl-warren requested reviews from viceice, Mai-Lapyst

2025-07-25 11:29:35 +00:00

viceice reviewed

2025-07-25 12:37:05 +00:00

pkg/schema/schema_test.go Outdated

					
				@ -93,0 +104,4 @@

				jobs:

				  build_frontend_dev:

				    name: Build Silo Frontend DEV

				    runs-on: ubuntu-latest

I'm confused. Why do we need a runs-on here? 🤔(I know we don't support reusable workflows yet)

I'm confused. Why do we need a `runs-on` here? 🤔(I know we don't support reusable workflows yet)

I think you're right. It is redundant with the runs-on that is mandatory in the workflow being reused. My bad.

earl-warren marked this conversation as resolved

earl-warren commented

2025-07-26 10:29:02 +00:00

Actually... this is necessary because a workflow that uses a workflow call will need a runs-on to be picked up by the runner. Otherwise it will not. The workflow that is re-used also needs a runs-on although it may not be necessary in some cases:

Not necessary if it is called from another workflow with the workflow_call event
Necessary if it is called because it is triggered by a push event

The two cases cannot be distinguished syntactically and have to both be valid.

Actually... this is necessary because a workflow that uses a workflow call will need a runs-on to be picked up by the runner. Otherwise it will not. The workflow that is re-used also needs a runs-on although it may not be necessary in some cases: - Not necessary if it is called from another workflow with the `workflow_call` event - Necessary if it is called because it is triggered by a push event The two cases cannot be distinguished syntactically and have to both be valid.

earl-warren changed title from ~~fix: runs-on is also mandatory for reusable workflows~~ to fix: runs-on is also mandatory for reusable workflows and may be needing container

2025-07-26 10:29:42 +00:00

earl-warren referenced this pull request from forgejo/end-to-end

2025-07-26 10:44:46 +00:00

actions: add test for reusable workflows #839

earl-warren commented

2025-07-26 10:46:02 +00:00

forgejo/end-to-end#839 was manually run ahead of time to verify it passes.

https://code.forgejo.org/forgejo/end-to-end/pulls/839 was manually run ahead of time to verify it passes. ![image](/attachments/d49f87b5-af96-42f2-beea-6f7a0e394a04)

image.png

125 KiB

earl-warren changed title from ~~fix: runs-on is also mandatory for reusable workflows and may be needing container~~ to fix: runs-on is also mandatory for reusable workflows and may be needing container [skip cascade]

2025-07-26 10:46:51 +00:00

earl-warren force-pushed wip-workflow-call from 50c850d757

/ cascade (pull_request_target) Successful in 1m31s

Details

checks / unit (pull_request) Successful in 1m32s

Details

checks / integration (pull_request) Successful in 2m53s

Details

to 736fbff925

/ cascade (pull_request_target) Has been skipped

Details

checks / unit (pull_request) Successful in 1m46s

Details

checks / integration (pull_request) Successful in 1m53s

Details

2025-07-26 10:46:56 +00:00

Compare

earl-warren commented

2025-07-26 10:47:43 +00:00

Waiting on forgejo/end-to-end#839 to be merged to confirm the end-to-end test passes.

Waiting on https://code.forgejo.org/forgejo/end-to-end/pulls/839 to be merged to confirm the end-to-end test passes.

earl-warren referenced this pull request from a commit

2025-07-26 11:23:40 +00:00

actions: add test for reusable workflows (#839)

earl-warren force-pushed wip-workflow-call from 736fbff925

/ cascade (pull_request_target) Has been skipped

Details

checks / unit (pull_request) Successful in 1m46s

Details

checks / integration (pull_request) Successful in 1m53s

Details

to 08f21a5bf3

/ cascade (pull_request_target) Has been skipped

Details

checks / unit (pull_request) Successful in 2m6s

Details

checks / integration (pull_request) Successful in 2m16s

Details

2025-07-26 11:27:25 +00:00

Compare

earl-warren changed title from ~~fix: runs-on is also mandatory for reusable workflows and may be needing container [skip cascade]~~ to fix: runs-on is also mandatory for reusable workflows and may be needing container

2025-07-26 11:27:54 +00:00

earl-warren force-pushed wip-workflow-call from 08f21a5bf3

/ cascade (pull_request_target) Has been skipped

Details

checks / unit (pull_request) Successful in 2m6s

Details

checks / integration (pull_request) Successful in 2m16s

Details

to 54ca9dbb47

checks / unit (pull_request) Successful in 2m7s

Details

checks / integration (pull_request) Successful in 2m20s

Details

/ cascade (pull_request_target) Failing after 30s

Details

2025-07-26 11:28:00 +00:00

Compare

viceice approved these changes

2025-07-26 11:28:17 +00:00

pkg/schema/workflow_schema.json

					
				@ -1474,1 +1474,4 @@

				          },

				          "runs-on": {

				            "type": "runs-on",

				            "required": true

wouldn't this break existing workflows if it's required? 🤔

I still don't understand why it's needed, but will approve. can be fixed later

wouldn't this break existing workflows if it's required? 🤔 I still don't understand why it's needed, but will approve. can be fixed later

It is required as of a few runner versions ago. The default value when it is not specified is undocumented and lead to various breakage. It was decided to not support a default value (based on what?). This is therefore not a breaking change but rather a clearer error message than the often difficult/obscure consequences of what happens when a runs-on is not specified.

In GitHub there is a limited number of runs-on and their semantic is tightly controlled by the server. Forgejo Actions grants a lot more flexibility and the interpretation is left to the runner ultimately. In that context figuring out a sane semantic for when runs-on is not specified would require thinking. I'm not saying it is not possible, but it definitely is not trivial.

Does that make sense?

It is required as of a few runner versions ago. The default value when it is not specified is undocumented and lead to various breakage. It was decided to not support a default value (based on what?). This is therefore not a breaking change but rather a clearer error message than the often difficult/obscure consequences of what happens when a `runs-on` is not specified. In GitHub there is a limited number of runs-on and their semantic is tightly controlled by the server. Forgejo Actions grants a lot more flexibility and the interpretation is left to the runner ultimately. In that context figuring out a sane semantic for when `runs-on` is not specified would require thinking. I'm not saying it is not possible, but it definitely is not trivial. Does that make sense?