forgejo/act
Archived
24
5
Fork 16
Code Issues Pull requests Releases Activity Actions

feat!: add the validate argument to reading workflows #180

Merged
earl-warren merged 2 commits from earl-warren/act:wip-validation into main 2025-07-16 08:46:36 +00:00
Conversation 11 Commits 2 Files changed 11 +57 -51
earl-warren commented 2025-07-14 21:32:44 +00:00
Contributor
Copy link

This is a followup of #170 so that it is possible to read a workflow without validation. It is not uncommon for Forgejo to read a workflow just to extract a few information from it, knowing it has been validated before. It would be a performance regression if schema validation happened in these cases.

This is a port of https://github.com/nektos/act/pull/2717/files

It is a breaking change in the context of Forgejo and Forgejo runner because it will need to add the new validate argument when reading workflows.

This is a followup of https://code.forgejo.org/forgejo/act/pulls/170 so that it is possible to read a workflow without validation. It is not uncommon for Forgejo to read a workflow just to extract a few information from it, knowing it has been validated before. It would be a performance regression if schema validation happened in these cases. This is a port of https://github.com/nektos/act/pull/2717/files It is a breaking change in the context of Forgejo and Forgejo runner because it will need to add the new `validate` argument when reading workflows.
earl-warren force-pushed wip-validation from cb3ae8e922
Some checks failed
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Failing after 1m17s
Details
checks / integration (pull_request) Has been skipped
Details
to c49a3085d8
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 3m10s
Details
checks / integration (pull_request) Successful in 1m25s
Details
2025-07-14 21:36:08 +00:00 Compare
earl-warren changed title from feat: allow to disable workflow schema validation [skip cascade] to feat: allow to disable workflow schema validation 2025-07-14 21:41:57 +00:00
earl-warren force-pushed wip-validation from c49a3085d8
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 3m10s
Details
checks / integration (pull_request) Successful in 1m25s
Details
to 8fc5ac4f69
Some checks failed
checks / unit (pull_request) Successful in 2m17s
Details
checks / integration (pull_request) Successful in 1m36s
Details
/ cascade (pull_request_target) Failing after 1m20s
Details
2025-07-14 21:42:20 +00:00 Compare
cascading-pr commented 2025-07-14 21:43:40 +00:00
Contributor
Copy link

cascading-pr updated at forgejo/runner#698

cascading-pr updated at https://code.forgejo.org/forgejo/runner/pulls/698
cascading-pr commented 2025-07-14 22:07:29 +00:00
Contributor
Copy link

cascading-pr updated at forgejo/runner#698

cascading-pr updated at https://code.forgejo.org/forgejo/runner/pulls/698
earl-warren changed title from feat: allow to disable workflow schema validation to feat!: allow to disable workflow schema validation 2025-07-14 22:08:48 +00:00
earl-warren changed title from feat!: allow to disable workflow schema validation to feat!: allow to disable workflow schema validation [skip cascade] 2025-07-14 22:09:24 +00:00
earl-warren force-pushed wip-validation from 8fc5ac4f69
Some checks failed
checks / unit (pull_request) Successful in 2m17s
Details
checks / integration (pull_request) Successful in 1m36s
Details
/ cascade (pull_request_target) Failing after 1m20s
Details
to 309bedff0c
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m32s
Details
checks / integration (pull_request) Successful in 1m29s
Details
2025-07-14 22:09:59 +00:00 Compare
viceice reviewed 2025-07-15 04:58:39 +00:00
viceice left a comment
Owner

otherwise LGTM

otherwise LGTM
pkg/model/workflow.go Outdated
@ -109,0 +114,4 @@
Definition: "workflow-root-strict",
Schema: schema.GetWorkflowSchema(),
}).UnmarshalYAML(node); err != nil {
return errors.Join(err, fmt.Errorf("Actions YAML Strict Schema Validation Error detected:\nFor more information, see: https://nektosact.com/usage/schema.html"))
viceice commented 2025-07-15 04:57:20 +00:00
Owner

we should point to a different url

we should point to a different url
👍 1
earl-warren commented 2025-07-15 09:46:54 +00:00
Author
Contributor

You made me realize that I completely misunderstood the purpose of the ACT PR I cherry picked. It always validates. Only with different methods. I'll change this PR to do what it is meant to do instead of what it currently does. 😊

You made me realize that I completely misunderstood the purpose of the ACT PR I cherry picked. It always validates. Only with different methods. I'll change this PR to do what it is meant to do instead of what it currently does. 😊
earl-warren commented 2025-07-15 09:59:21 +00:00
Author
Contributor

#180/files

This is the change that matters. Do not add a strict mode. Remove validation from the existing struct (Workflow) and add it to a new struct (WorkflowValidate) and only use it if validate is true.

https://code.forgejo.org/forgejo/act/pulls/180/files#diff-3ab725671ce0046ec2c8cd637a503a74a84294a1 This is the change that matters. Do not add a strict mode. Remove validation from the existing struct (Workflow) and add it to a new struct (WorkflowValidate) and only use it if validate is true.
earl-warren changed title from feat!: allow to disable workflow schema validation [skip cascade] to WIP: feat!: allow to disable workflow schema validation [skip cascade] 2025-07-15 09:45:17 +00:00
earl-warren force-pushed wip-validation from 309bedff0c
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m32s
Details
checks / integration (pull_request) Successful in 1m29s
Details
to 4db3721331
Some checks failed
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Failing after 39s
Details
checks / integration (pull_request) Has been skipped
Details
2025-07-15 09:55:35 +00:00 Compare
earl-warren changed title from WIP: feat!: allow to disable workflow schema validation [skip cascade] to WIP: feat!: add the validate argument to reading workflows [skip cascade] 2025-07-15 09:56:03 +00:00
earl-warren force-pushed wip-validation from 4db3721331
Some checks failed
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Failing after 39s
Details
checks / integration (pull_request) Has been skipped
Details
to c7b3d225d3
Some checks failed
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Failing after 33s
Details
checks / integration (pull_request) Has been skipped
Details
2025-07-15 09:57:52 +00:00 Compare
viceice requested changes 2025-07-15 10:07:37 +00:00
Dismissed
viceice left a comment
Owner

lgtm, but multiple test failures

eg:

=== RUN   TestReadWorkflow_StepsTypes
    workflow_test.go:436: 
        	Error Trace:	/workspace/forgejo/act/pkg/model/workflow_test.go:436
        	Error:      	An error is expected but got nil.
        	Test:       	TestReadWorkflow_StepsTypes
        	Messages:   	read workflow should fail
--- FAIL: TestReadWorkflow_StepsTypes (0.00s)
lgtm, but multiple test failures eg: ``` === RUN TestReadWorkflow_StepsTypes workflow_test.go:436: Error Trace: /workspace/forgejo/act/pkg/model/workflow_test.go:436 Error: An error is expected but got nil. Test: TestReadWorkflow_StepsTypes Messages: read workflow should fail --- FAIL: TestReadWorkflow_StepsTypes (0.00s) ```
earl-warren force-pushed wip-validation from c7b3d225d3
Some checks failed
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Failing after 33s
Details
checks / integration (pull_request) Has been skipped
Details
to ca963646d4
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m16s
Details
checks / integration (pull_request) Successful in 2m28s
Details
2025-07-15 10:51:20 +00:00 Compare
earl-warren commented 2025-07-15 10:53:06 +00:00
Author
Contributor
Copy link

Those tests are the one asserting schema validation detect errors. They now need to explicitly require validation.

Those tests are the one asserting schema validation detect errors. They now need to explicitly require validation.
earl-warren commented 2025-07-15 10:56:30 +00:00
Author
Contributor
Copy link

c7b3d225d3..ca963646d4 makes it green

https://code.forgejo.org/forgejo/act/compare/c7b3d225d36d669e90c8afbb09b8540522b547c1..ca963646d4eaf4f126cc3f22e7bc6adfa19e2e30 makes it green
earl-warren changed title from WIP: feat!: add the validate argument to reading workflows [skip cascade] to feat!: add the validate argument to reading workflows [skip cascade] 2025-07-15 10:56:33 +00:00
viceice commented 2025-07-15 14:10:59 +00:00
Owner
Copy link

still failing ci checks

still failing ci checks
earl-warren force-pushed wip-validation from 732db6753e
Some checks failed
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m17s
Details
checks / integration (pull_request) Failing after 1m11s
Details
to 34dfb407af
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m17s
Details
checks / integration (pull_request) Successful in 13m25s
Details
2025-07-15 21:03:13 +00:00 Compare
earl-warren changed title from feat!: add the validate argument to reading workflows [skip cascade] to feat!: add the validate argument to reading workflows 2025-07-15 21:20:44 +00:00
earl-warren commented 2025-07-15 21:20:59 +00:00
Author
Contributor
Copy link

Rebased and green for real now 😁

Rebased and green for real now 😁
earl-warren force-pushed wip-validation from 34dfb407af
All checks were successful
/ cascade (pull_request_target) Has been skipped
Details
checks / unit (pull_request) Successful in 1m17s
Details
checks / integration (pull_request) Successful in 13m25s
Details
to e278c02468
Some checks failed
checks / unit (pull_request) Successful in 3m30s
Details
/ cascade (pull_request_target) Failing after 4m27s
Details
checks / integration (pull_request) Successful in 2m14s
Details
2025-07-16 06:55:15 +00:00 Compare
earl-warren commented 2025-07-16 06:55:33 +00:00
Author
Contributor
Copy link

Rebased because the CLI is gone now

Rebased because the CLI is gone now
cascading-pr commented 2025-07-16 06:57:07 +00:00
Contributor
Copy link

cascading-pr updated at forgejo/runner#711

cascading-pr updated at https://code.forgejo.org/forgejo/runner/pulls/711
earl-warren changed title from feat!: add the validate argument to reading workflows to feat!: add the validate argument to reading workflows [skip cascade] 2025-07-16 07:02:15 +00:00
earl-warren force-pushed wip-validation from e278c02468
Some checks failed
checks / unit (pull_request) Successful in 3m30s
Details
/ cascade (pull_request_target) Failing after 4m27s
Details
checks / integration (pull_request) Successful in 2m14s
Details
to d4417914bc
Some checks failed
checks / unit (pull_request) Successful in 2m36s
Details
checks / integration (pull_request) Successful in 2m9s
Details
/ cascade (pull_request_target) Failing after 17s
Details
2025-07-16 07:02:23 +00:00 Compare
earl-warren changed title from feat!: add the validate argument to reading workflows [skip cascade] to feat!: add the validate argument to reading workflows 2025-07-16 07:02:54 +00:00
viceice approved these changes 2025-07-16 08:24:56 +00:00
earl-warren deleted branch wip-validation 2025-07-16 08:46:36 +00:00
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Kwonunn
viceice
Labels
Clear labels   
Compat/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind
Chore
Housekeeping, cleanup and chores

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No labels
Compat/Breaking
Kind/Bug
Kind
Chore
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
earl-warren
3 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo/act!180
No description provided.