Skip to content

Cleanup ActionRun creation #35624

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Cleanup ActionRun creation #35624

wants to merge 14 commits into from
+132 −236

Conversation

ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Oct 10, 2025 •
edited
Loading

  • share more logic

Closes #35622

@ChristopherHX
Cleanup ActionRun creation
678a970 
* share more logic
@ChristopherHX ChristopherHX added type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality. topic/gitea-actions related to the actions of Gitea labels Oct 10, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 10, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Oct 10, 2025
@ChristopherHX ChristopherHX marked this pull request as ready for review October 11, 2025 10:51
@ChristopherHX
Copy link
Contributor Author

This shows how, worse the duplication is at the moment. Preparation for further refactoring.

Maybe

  • merge the new PrepareRun with InsertRun
  • change the name of PrepareRun

At least the surface of doing code path specific inconsistent changes decreases and more tests cover the shared code than the specific code ever did.

ChristopherHX
ChristopherHX commented Oct 11, 2025
View reviewed changes
services/actions/run.go Outdated
}

// FIXME PERF skip this for schedule, dispatch etc.
CreateCommitStatus(ctx, allJobs...)
Copy link
Contributor Author

@ChristopherHX ChristopherHX Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old code conditionally didn't call this, nested function does the check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fix this FIXME in this PR? Or leave it to the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this now, by extracting the event check of createCommitStatus into a reusable function that takes the run without traversing over all jobs.

delvh
delvh approved these changes Oct 11, 2025
View reviewed changes
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 11, 2025
Zettat123
Zettat123 reviewed Oct 11, 2025
View reviewed changes
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 11, 2025
wxiaoguang
wxiaoguang reviewed Oct 12, 2025
View reviewed changes
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Oct 12, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 12, 2025 •
edited
Loading

Made some new changes, now we require callers to pass "run" when calling CreateCommitStatusForRunJobs, then it is CreateCommitStatusForRunJobs's responsibility to decide whether to create the status (for example: no status for cron/scheduled jobs). And we only need to retrieve event name and commit id once for a run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh approved these changes

+1 more reviewer

@Zettat123 Zettat123 Zettat123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code topic/gitea-actions related to the actions of Gitea type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent context availability in run-name

5 participants

@ChristopherHX @wxiaoguang @Zettat123 @delvh @GiteaBot