-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Actions concurrency
syntax
#32751
base: main
Are you sure you want to change the base?
Conversation
3551677
to
fcf4517
Compare
52833e7
to
130f2a2
Compare
461c7c1
to
d5168a2
Compare
e038ed2
to
f77f266
Compare
concurrency
for Actionsconcurrency
syntax
ad71599
to
8f5948b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
) Move the main logic of `generateTaskContext` and `findTaskNeeds` to the `services` layer. This is a part of #32751, since we need the git context and `needs` to parse the concurrency expressions. --------- Co-authored-by: Lunny Xiao <[email protected]> Co-authored-by: wxiaoguang <[email protected]>
87a79d9
to
eb43091
Compare
19d3dcb
to
93af0ee
Compare
…gitea#33280) Move the main logic of `generateTaskContext` and `findTaskNeeds` to the `services` layer. This is a part of go-gitea#32751, since we need the git context and `needs` to parse the concurrency expressions. --------- Co-authored-by: Lunny Xiao <[email protected]> Co-authored-by: wxiaoguang <[email protected]> (cherry picked from commit d0962ce) Conflicts: routers/api/actions/runner/main_test.go routers/api/actions/runner/utils.go services/actions/context_test.go services/actions/init_test.go tests/integration/actions_job_test.go simple conflicts related to ref_type": string(refName.RefType()), // string, The type of ref that triggered the workflow run. Valid values are branch or tag. Use env GITEA_RUNNER_REGISTRATION_TOKEN as global runner token (go-gitea#32946)
a1d070c
to
c433843
Compare
concurrency
syntaxconcurrency
syntax
This PR is ready for review. Please also review https://gitea.com/gitea/act/pulls/124 . |
c433843
to
7b27a42
Compare
3263d4e
to
85baf37
Compare
To support `concurrency` syntax for Gitea Actions Gitea PR: go-gitea/gitea#32751 Reviewed-on: https://gitea.com/gitea/act/pulls/124 Reviewed-by: Lunny Xiao <[email protected]> Co-authored-by: Zettat123 <[email protected]> Co-committed-by: Zettat123 <[email protected]>
Please resolve the conflicts and since https://gitea.com/gitea/act/pulls/124 merged, this PR can be updated. |
85baf37
to
9e21174
Compare
The conflicts were caused by model migrations instead of gitea/act. Now I've updated this PR and conflicts are resolved. |
return ShouldBlockRunByConcurrency(ctx, job.Run) | ||
} | ||
|
||
func CancelPreviousJobsByConcurrency(ctx context.Context, job *ActionRunJob) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I became today aware of a bug in the CancelPreviousJobs, jobs cancelled by this method don't get it's commit status updated from pending to cancelled this PR rewrites it.
Furthermore the actions_model cannot send webhooks / commit status, my conclusion is we have to change the return type to be like
([]*ActionRunJob, error) and always return the list for all jobs successfully cancelled. Then the actions_service can expose the function to its callers while handling commit status (and in future webhook) by traversing the ActionRunJob array
This bug did hit me in my workflow_job
event hook code aka #33694 (comment) and this function reproduces it
The reason I got this bug was that I looked where commitstatus is maintained and this one never triggered a webhook, but it seems to skip commit status as well.
I will write an bug report soon and a proposed fix, but since this PR also changes affected code I want to sync here first.
My idea how to fix this method is shown in #33764 and should be integrated here or fixed differently.
if err := CancelPreviousJobsByConcurrency(ctx, job); err != nil { | ||
return nil, false, err | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...per previous comment, this need to forward cancelled jobs as list to actions_service for both error and non error return value (we need to maintain the successfully cancelled status checks even if some failed)
actually this PR simplifies the bug surface a lot, only pickTask in actions services is a caller.
The old auto conconcurrency had a much larger defect area
r := newJobStatusResolver(tt.jobs) | ||
assert.Equal(t, tt.want, r.Resolve()) | ||
r := newJobStatusResolver(tt.jobs, nil) | ||
assert.Equal(t, tt.want, r.Resolve(context.Background())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.Equal(t, tt.want, r.Resolve(context.Background())) | |
assert.Equal(t, tt.want, r.Resolve(t.Context())) |
} | ||
|
||
gitCtx := GenerateGiteaContext(run, nil) | ||
jobResults := map[string]*jobparser.JobResult{"": {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the empty element is necessary?
@@ -99,6 +100,9 @@ func (opts FindRunOptions) ToConds() builder.Cond { | |||
if opts.TriggerEvent != "" { | |||
cond = cond.And(builder.Eq{"trigger_event": opts.TriggerEvent}) | |||
} | |||
if len(opts.ConcurrencyGroup) > 0 { | |||
cond = cond.And(builder.Eq{"concurrency_group": opts.ConcurrencyGroup}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is concurrency_group
an index column?
Fix #24769
Fix #32662
Fix #33260
Depends on https://gitea.com/gitea/act/pulls/124
This PR removes the auto-cancellation feature added by #25716. Users need to manually add
concurrency
to workflows to control concurrent workflows or jobs.