feat(runs_list): add button delete workflow run #33138
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/S
github-actions
bot
added
the
modifies/templates
pull-request-size
bot
added
size/M
github-actions
bot
added
modifies/go
zsbahtiar
changed the title
|
/claim #26219 |
|
It needs to be "almost" complete for #24256 & #26275 (comment) |
|
UI can be something like this: |
|
hi all thanks for feedback, i willl check after work |
make sure, we can select the workflow run except running and waiting workflow status? @yp05327 @wxiaoguang @Frankkkkk |
Ideally, yes But I won't say it is a must in this PR, we should make the basic logic right before the UI work (the UI work needs more frontend skills and time to make it right) |
pull-request-size
bot
added
the
size/L
|
hi im updated the code, please check and comment if have feedback thanks! @yp05327 @wxiaoguang @Frankkkkk Screen.Recording.2025-01-08.at.3.22.06.PM.mov |
There was a problem hiding this comment.
we should make the basic logic right #33138 (comment)
But I do not think the logic is complete.
It needs to be "almost" complete for #24256 & #26275 (comment)
Are all of them implemented? At least it needs some tests, otherwise the code will break in the future.
I think it's not right, it makes A user could delete B user's action.
#33138 (comment)
It seems that A user could still delete B's actions, the code lacks permission check. Correct me if I was wrong.
yah the first im skip about the restriction, but try to add in this commit: 05af60b i will try for create the test, i hope can create the test |
|
hi all im already done add the test 8c742ab can you check? thanks |
pull-request-size
bot
added
size/XL
| jobIDs, taskIDs []int64 | ||
| taskLogFileNames []string | ||
| ) | ||
| eg.Go(func() error { |
There was a problem hiding this comment.
The database operations should be done in one transaction, but not in "errgroup goroutine".
| ) | ||
| eg.Go(func() error { | ||
| var err error | ||
| actionRuns, err = actions_model.GetRunsByIDsAndTriggerUserID(ctx, req.ActionIDs, ctx.Doer.ID) |
There was a problem hiding this comment.
Why use Doer.ID here? For example: user A is admin, an ActionRun is not created by them, then user A can't delete that ActionRun?
| fileName = filepath.Join(setting.AppWorkPath, setting.AppDataPath, dirNameActionLog, taskLogFileName) | ||
| } | ||
|
|
||
| if err := os.Remove(fileName); err != nil { |
There was a problem hiding this comment.
IIRC there is a "storage" for Action logs, they could be in S3 (ObjectStorage) but not on filesystem
|
I really like the progress on this pull request and I think this will be a very useful addition. If I could make one suggestion, it would be to make the Delete Workflow button less prominent. I would either use an icon or go the GitHub route with the ellipsis button. I made a quick mockup (the ellipsis button would then open a small context menu with the Delete Workflow button as text).
GitHub Example (preferred): |
Submit for issue #26219
/claim #26219
Screen.Recording.2025-01-08.at.1.17.14.AM.mov