Skip to content

Add endpoint deleting workflow run #34337

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 48 commits into
base: main
Choose a base branch
from
Open

Add endpoint deleting workflow run #34337

wants to merge 48 commits into from
+647 −9

Conversation

NorthRealm
Copy link
Contributor

@NorthRealm NorthRealm commented May 1, 2025 •
edited
Loading

Add endpoint deleting workflow run
Resolves #26219

/claim #26219

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 1, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels May 1, 2025
@NorthRealm NorthRealm marked this pull request as ready for review May 2, 2025 10:22
@github-actions github-actions bot added modifies/translation modifies/templates This PR modifies the template files labels May 3, 2025
@NorthRealm
Copy link
Contributor Author

@ChristopherHX I just tried adding CleanupEphemeralRunners to modules/actions/run.go and cannot compile. Import cycle error.

Just a random idea: Move your code from actions modules to actions services? so both are in the same module.

AFAIK Web and rest api can reference service, but I didn't think about where your deletion code is, relative to my cleanup function.

Anything else about ephemeral runners might be better discussed in a new issue / proposal. The old code works, because tasks that are finished within 24h are never deleted yet.

Just pushed new commits. Please check.

@ChristopherHX
Copy link
Contributor

ChristopherHX commented May 8, 2025 •
edited
Loading

With deletion this is a follow up problem, this page looks very bad. No navigation bar, no styles.

image

Create / search for issues about this finding soon.

It so much easier to get this broken page with deletion, since the statuses point to that URL.

Out of scope for this PR

ChristopherHX
ChristopherHX reviewed May 8, 2025
View reviewed changes
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

Overall looks good, but I defer my approval after more testing from my side.

@lunny lunny mentioned this pull request May 11, 2025
Open
@NorthRealm @lunny
Update template
fbd1a0e 
Co-authored-by: Lunny Xiao <[email protected]>
@NorthRealm NorthRealm requested a review from lunny May 12, 2025 07:39
@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 May 12, 2025
wxiaoguang
wxiaoguang reviewed May 13, 2025
View reviewed changes
wxiaoguang
wxiaoguang reviewed May 13, 2025
View reviewed changes
routers/api/v1/repo/action.go

runID := ctx.PathParamInt64("run")

run, err := actions_model.GetRunByID(ctx, runID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Permission check bypass? A doer could delete repoB's action runs by accessing repoA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Permission check bypass? A doer could delete repoB's action runs by accessing repoA?

I believe the middlewares before repo.DeleteActionRun already do permission check.

Copy link
Contributor

Choose a reason for hiding this comment

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

How? The middleware allows to access repoA, but what if your "runID" is for repoB?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with wxiaoguang, the middleware does not check the path parameter "run" to belong to the checked repository.

Please check if the RepoID of the ActionRun with runid matches the ctx.Repo.Repository.ID

Feel free to look at the artifacts rest api Get and Delete by id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChristopherHX In that case I think your artifact implementation has the same defection too

Copy link
Contributor

Choose a reason for hiding this comment

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

My GetArtifactsOfRun has a repoid filter => database checks it

My getArtifactsByPathParam has an explicit repo_id check after reading it by id from database

If you found something, please point me to it. I would be happy to fix it.

wxiaoguang
wxiaoguang reviewed May 13, 2025
View reviewed changes
templates/repo/actions/runs_list.tmpl
@@ -36,6 +36,15 @@
<div class="run-list-meta">{{svg "octicon-calendar" 16}}{{DateUtils.TimeSince .Updated}}</div>
<div class="run-list-meta">{{svg "octicon-stopwatch" 16}}{{.Duration}}</div>
</div>
{{if and ($.AllowDeleteWorkflowRuns) (.Link) (.Status.IsDone)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why check (.Link)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At line 14 {{if .Link}}{{.Link}}{{else}}{{$.Link}}/{{.Index}}{{end}}
I don't quite understand this but I found .Link is the one wanted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I do not think the old code is right (since Implement actions (#21937))

the run.Link should be used directly without check.

Copy link
Contributor Author

@NorthRealm NorthRealm May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

iirc what I found is, take gitea as example, {{$.Link}} would be https://gitea.com/gitea/act/actions that misses out /runs.
So At line 14 can just remove the if check and at line 39 can remove the check?
In current Gitea probably {{$.Link}}/{{.Index}} would never be reached.
Anyone else?

Copy link
Contributor

@wxiaoguang wxiaoguang May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMO the old $.Link is an abuse (over-defensive programming)
.... I think we should always use run.Link in this case (just forget the $.Link and removing the if check should be good enough).

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

@ChristopherHX ChristopherHX ChristopherHX left review comments

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
🙋 Bounty claim lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.

Delete workflow runs
6 participants
@NorthRealm @inxcts @wxiaoguang @ChristopherHX @lunny @GiteaBot