From c551d3f3ab13379b0740fc45bc4dfc8f2fb84e16 Mon Sep 17 00:00:00 2001 From: FuXiaoHei Date: Sun, 18 Feb 2024 18:33:50 +0800 Subject: [PATCH 1/3] Artifact deletion in actions ui (#27172) Add deletion link in runs view page. Fix #26315 ![image](https://github.com/go-gitea/gitea/assets/2142787/aa65a4ab-f434-4deb-b953-21e63c212033) When click deletion button. It marks this artifact `need-delete`. This artifact would be deleted when actions cleanup cron task. --- models/actions/artifact.go | 22 ++++++++++ options/locale/locale_en-US.ini | 1 + routers/web/repo/actions/view.go | 51 +++++++++++++++++++----- routers/web/web.go | 1 + services/actions/cleanup.go | 38 +++++++++++++++++- templates/repo/actions/view.tmpl | 1 + web_src/js/components/RepoActionView.vue | 15 ++++++- web_src/js/svg.js | 2 + 8 files changed, 120 insertions(+), 11 deletions(-) diff --git a/models/actions/artifact.go b/models/actions/artifact.go index 5390f6288f..3d0a288e62 100644 --- a/models/actions/artifact.go +++ b/models/actions/artifact.go @@ -26,6 +26,8 @@ const ( ArtifactStatusUploadConfirmed // 2， ArtifactStatusUploadConfirmed is the status of an artifact upload that is confirmed ArtifactStatusUploadError // 3， ArtifactStatusUploadError is the status of an artifact upload that is errored ArtifactStatusExpired // 4, ArtifactStatusExpired is the status of an artifact that is expired + ArtifactStatusPendingDeletion // 5, ArtifactStatusPendingDeletion is the status of an artifact that is pending deletion + ArtifactStatusDeleted // 6, ArtifactStatusDeleted is the status of an artifact that is deleted ) func init() { @@ -147,8 +149,28 @@ func ListNeedExpiredArtifacts(ctx context.Context) ([]*ActionArtifact, error) { Where("expired_unix < ? AND status = ?", timeutil.TimeStamp(time.Now().Unix()), ArtifactStatusUploadConfirmed).Find(&arts) } +// ListPendingDeleteArtifacts returns all artifacts in pending-delete status. +// limit is the max number of artifacts to return. +func ListPendingDeleteArtifacts(ctx context.Context, limit int) ([]*ActionArtifact, error) { + arts := make([]*ActionArtifact, 0, limit) + return arts, db.GetEngine(ctx). + Where("status = ?", ArtifactStatusPendingDeletion).Limit(limit).Find(&arts) +} + // SetArtifactExpired sets an artifact to expired func SetArtifactExpired(ctx context.Context, artifactID int64) error { _, err := db.GetEngine(ctx).Where("id=? AND status = ?", artifactID, ArtifactStatusUploadConfirmed).Cols("status").Update(&ActionArtifact{Status: int64(ArtifactStatusExpired)}) return err } + +// SetArtifactNeedDelete sets an artifact to need-delete, cron job will delete it +func SetArtifactNeedDelete(ctx context.Context, runID int64, name string) error { + _, err := db.GetEngine(ctx).Where("run_id=? AND artifact_name=? AND status = ?", runID, name, ArtifactStatusUploadConfirmed).Cols("status").Update(&ActionArtifact{Status: int64(ArtifactStatusPendingDeletion)}) + return err +} + +// SetArtifactDeleted sets an artifact to deleted +func SetArtifactDeleted(ctx context.Context, artifactID int64) error { + _, err := db.GetEngine(ctx).ID(artifactID).Cols("status").Update(&ActionArtifact{Status: int64(ArtifactStatusDeleted)}) + return err +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index bdae9a29ac..83061bcd9d 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -124,6 +124,7 @@ pin = Pin unpin = Unpin artifacts = Artifacts +confirm_delete_artifact = Are you sure you want to delete the artifact '%s' ? archived = Archived diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index ba2e63c3cc..903ff2632f 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -98,15 +98,16 @@ type ViewRequest struct { type ViewResponse struct { State struct { Run struct { - Link string `json:"link"` - Title string `json:"title"` - Status string `json:"status"` - CanCancel bool `json:"canCancel"` - CanApprove bool `json:"canApprove"` // the run needs an approval and the doer has permission to approve - CanRerun bool `json:"canRerun"` - Done bool `json:"done"` - Jobs []*ViewJob `json:"jobs"` - Commit ViewCommit `json:"commit"` + Link string `json:"link"` + Title string `json:"title"` + Status string `json:"status"` + CanCancel bool `json:"canCancel"` + CanApprove bool `json:"canApprove"` // the run needs an approval and the doer has permission to approve + CanRerun bool `json:"canRerun"` + CanDeleteArtifact bool `json:"canDeleteArtifact"` + Done bool `json:"done"` + Jobs []*ViewJob `json:"jobs"` + Commit ViewCommit `json:"commit"` } `json:"run"` CurrentJob struct { Title string `json:"title"` @@ -187,6 +188,7 @@ func ViewPost(ctx *context_module.Context) { resp.State.Run.CanCancel = !run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions) resp.State.Run.CanApprove = run.NeedApproval && ctx.Repo.CanWrite(unit.TypeActions) resp.State.Run.CanRerun = run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions) + resp.State.Run.CanDeleteArtifact = run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions) resp.State.Run.Done = run.Status.IsDone() resp.State.Run.Jobs = make([]*ViewJob, 0, len(jobs)) // marshal to '[]' instead fo 'null' in json resp.State.Run.Status = run.Status.String() @@ -576,6 +578,29 @@ func ArtifactsView(ctx *context_module.Context) { ctx.JSON(http.StatusOK, artifactsResponse) } +func ArtifactsDeleteView(ctx *context_module.Context) { + if !ctx.Repo.CanWrite(unit.TypeActions) { + ctx.Error(http.StatusForbidden, "no permission") + return + } + + runIndex := ctx.ParamsInt64("run") + artifactName := ctx.Params("artifact_name") + + run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex) + if err != nil { + ctx.NotFoundOrServerError("GetRunByIndex", func(err error) bool { + return errors.Is(err, util.ErrNotExist) + }, err) + return + } + if err = actions_model.SetArtifactNeedDelete(ctx, run.ID, artifactName); err != nil { + ctx.Error(http.StatusInternalServerError, err.Error()) + return + } + ctx.JSON(http.StatusOK, struct{}{}) +} + func ArtifactsDownloadView(ctx *context_module.Context) { runIndex := ctx.ParamsInt64("run") artifactName := ctx.Params("artifact_name") @@ -603,6 +628,14 @@ func ArtifactsDownloadView(ctx *context_module.Context) { return } + // if artifacts status is not uploaded-confirmed, treat it as not found + for _, art := range artifacts { + if art.Status != int64(actions_model.ArtifactStatusUploadConfirmed) { + ctx.Error(http.StatusNotFound, "artifact not found") + return + } + } + ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%s.zip; filename*=UTF-8''%s.zip", url.PathEscape(artifactName), artifactName)) writer := zip.NewWriter(ctx.Resp) diff --git a/routers/web/web.go b/routers/web/web.go index 400bed9288..0684b2ac82 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1401,6 +1401,7 @@ func registerRoutes(m *web.Route) { m.Post("/approve", reqRepoActionsWriter, actions.Approve) m.Post("/artifacts", actions.ArtifactsView) m.Get("/artifacts/{artifact_name}", actions.ArtifactsDownloadView) + m.Delete("/artifacts/{artifact_name}", actions.ArtifactsDeleteView) m.Post("/rerun", reqRepoActionsWriter, actions.Rerun) }) }) diff --git a/services/actions/cleanup.go b/services/actions/cleanup.go index 785eeb5838..59e2cc85de 100644 --- a/services/actions/cleanup.go +++ b/services/actions/cleanup.go @@ -20,8 +20,15 @@ func Cleanup(taskCtx context.Context, olderThan time.Duration) error { return CleanupArtifacts(taskCtx) } -// CleanupArtifacts removes expired artifacts and set records expired status +// CleanupArtifacts removes expired add need-deleted artifacts and set records expired status func CleanupArtifacts(taskCtx context.Context) error { + if err := cleanExpiredArtifacts(taskCtx); err != nil { + return err + } + return cleanNeedDeleteArtifacts(taskCtx) +} + +func cleanExpiredArtifacts(taskCtx context.Context) error { artifacts, err := actions.ListNeedExpiredArtifacts(taskCtx) if err != nil { return err @@ -40,3 +47,32 @@ func CleanupArtifacts(taskCtx context.Context) error { } return nil } + +// deleteArtifactBatchSize is the batch size of deleting artifacts +const deleteArtifactBatchSize = 100 + +func cleanNeedDeleteArtifacts(taskCtx context.Context) error { + for { + artifacts, err := actions.ListPendingDeleteArtifacts(taskCtx, deleteArtifactBatchSize) + if err != nil { + return err + } + log.Info("Found %d artifacts pending deletion", len(artifacts)) + for _, artifact := range artifacts { + if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil { + log.Error("Cannot delete artifact %d: %v", artifact.ID, err) + continue + } + if err := actions.SetArtifactDeleted(taskCtx, artifact.ID); err != nil { + log.Error("Cannot set artifact %d deleted: %v", artifact.ID, err) + continue + } + log.Info("Artifact %d set deleted", artifact.ID) + } + if len(artifacts) < deleteArtifactBatchSize { + log.Debug("No more artifacts pending deletion") + break + } + } + return nil +} diff --git a/templates/repo/actions/view.tmpl b/templates/repo/actions/view.tmpl index 6b07e7000a..f8b106147b 100644 --- a/templates/repo/actions/view.tmpl +++ b/templates/repo/actions/view.tmpl @@ -19,6 +19,7 @@ data-locale-status-skipped="{{ctx.Locale.Tr "actions.status.skipped"}}" data-locale-status-blocked="{{ctx.Locale.Tr "actions.status.blocked"}}" data-locale-artifacts-title="{{ctx.Locale.Tr "artifacts"}}" + data-locale-confirm-delete-artifact="{{ctx.Locale.Tr "confirm_delete_artifact"}}" data-locale-show-timestamps="{{ctx.Locale.Tr "show_timestamps"}}" data-locale-show-log-seconds="{{ctx.Locale.Tr "show_log_seconds"}}" data-locale-show-full-screen="{{ctx.Locale.Tr "show_full_screen"}}" diff --git a/web_src/js/components/RepoActionView.vue b/web_src/js/components/RepoActionView.vue index 797869b78c..c4a7389bc5 100644 --- a/web_src/js/components/RepoActionView.vue +++ b/web_src/js/components/RepoActionView.vue @@ -5,7 +5,7 @@ import {createApp} from 'vue'; import {toggleElem} from '../utils/dom.js'; import {getCurrentLocale} from '../utils.js'; import {renderAnsi} from '../render/ansi.js'; -import {POST} from '../modules/fetch.js'; +import {POST, DELETE} from '../modules/fetch.js'; const sfc = { name: 'RepoActionView', @@ -200,6 +200,12 @@ const sfc = { return await resp.json(); }, + async deleteArtifact(name) { + if (!window.confirm(this.locale.confirmDeleteArtifact.replace('%s', name))) return; + await DELETE(`${this.run.link}/artifacts/${name}`); + await this.loadJob(); + }, + async fetchJob() { const logCursors = this.currentJobStepsStates.map((it, idx) => { // cursor is used to indicate the last position of the logs @@ -329,6 +335,8 @@ export function initRepositoryActionView() { cancel: el.getAttribute('data-locale-cancel'), rerun: el.getAttribute('data-locale-rerun'), artifactsTitle: el.getAttribute('data-locale-artifacts-title'), + areYouSure: el.getAttribute('data-locale-are-you-sure'), + confirmDeleteArtifact: el.getAttribute('data-locale-confirm-delete-artifact'), rerun_all: el.getAttribute('data-locale-rerun-all'), showTimeStamps: el.getAttribute('data-locale-show-timestamps'), showLogSeconds: el.getAttribute('data-locale-show-log-seconds'), @@ -404,6 +412,9 @@ export function initRepositoryActionView() { {{ artifact.name }} + + + @@ -528,6 +539,8 @@ export function initRepositoryActionView() { .job-artifacts-item { margin: 5px 0; padding: 6px; + display: flex; + justify-content: space-between; } .job-artifacts-list { diff --git a/web_src/js/svg.js b/web_src/js/svg.js index 084256587c..471b5136bd 100644 --- a/web_src/js/svg.js +++ b/web_src/js/svg.js @@ -67,6 +67,7 @@ import octiconStrikethrough from '../../public/assets/img/svg/octicon-strikethro import octiconSync from '../../public/assets/img/svg/octicon-sync.svg'; import octiconTable from '../../public/assets/img/svg/octicon-table.svg'; import octiconTag from '../../public/assets/img/svg/octicon-tag.svg'; +import octiconTrash from '../../public/assets/img/svg/octicon-trash.svg'; import octiconTriangleDown from '../../public/assets/img/svg/octicon-triangle-down.svg'; import octiconX from '../../public/assets/img/svg/octicon-x.svg'; import octiconXCircleFill from '../../public/assets/img/svg/octicon-x-circle-fill.svg'; @@ -139,6 +140,7 @@ const svgs = { 'octicon-sync': octiconSync, 'octicon-table': octiconTable, 'octicon-tag': octiconTag, + 'octicon-trash': octiconTrash, 'octicon-triangle-down': octiconTriangleDown, 'octicon-x': octiconX, 'octicon-x-circle-fill': octiconXCircleFill, From 7f64e4d2a3f20b7d7de6542de5e0856c643e821f Mon Sep 17 00:00:00 2001 From: FuXiaoHei Date: Sun, 18 Feb 2024 22:25:14 +0800 Subject: [PATCH 2/3] Expire artifacts before deleting them physically (#29241) https://github.com/go-gitea/gitea/pull/27172#discussion_r1493735466 When cleanup artifacts, it removes storage first. If storage is not exist (maybe delete manually), it gets error and continue loop. It makes a dead loop if there are a lot pending but non-existing artifacts. Now it updates db record at first to avoid keep a lot of pending status artifacts. --- services/actions/cleanup.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/services/actions/cleanup.go b/services/actions/cleanup.go index 59e2cc85de..5376c2624c 100644 --- a/services/actions/cleanup.go +++ b/services/actions/cleanup.go @@ -35,14 +35,14 @@ func cleanExpiredArtifacts(taskCtx context.Context) error { } log.Info("Found %d expired artifacts", len(artifacts)) for _, artifact := range artifacts { - if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil { - log.Error("Cannot delete artifact %d: %v", artifact.ID, err) - continue - } if err := actions.SetArtifactExpired(taskCtx, artifact.ID); err != nil { log.Error("Cannot set artifact %d expired: %v", artifact.ID, err) continue } + if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil { + log.Error("Cannot delete artifact %d: %v", artifact.ID, err) + continue + } log.Info("Artifact %d set expired", artifact.ID) } return nil @@ -59,14 +59,14 @@ func cleanNeedDeleteArtifacts(taskCtx context.Context) error { } log.Info("Found %d artifacts pending deletion", len(artifacts)) for _, artifact := range artifacts { - if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil { - log.Error("Cannot delete artifact %d: %v", artifact.ID, err) - continue - } if err := actions.SetArtifactDeleted(taskCtx, artifact.ID); err != nil { log.Error("Cannot set artifact %d deleted: %v", artifact.ID, err) continue } + if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil { + log.Error("Cannot delete artifact %d: %v", artifact.ID, err) + continue + } log.Info("Artifact %d set deleted", artifact.ID) } if len(artifacts) < deleteArtifactBatchSize { From 98943fdb43fd28fcc8a3fd5185bfca36de63aa0a Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Fri, 23 Feb 2024 09:24:43 +0100 Subject: [PATCH 3/3] tests: Add a basic test for artifact deletion Adds a very bare-bones test for artifact deletion. It does not exercise the functionality itself, just the presence of the functionality. Signed-off-by: Gergely Nagy --- tests/integration/actions_route_test.go | 35 +++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/integration/actions_route_test.go b/tests/integration/actions_route_test.go index c941fca2e5..9a4a5801d2 100644 --- a/tests/integration/actions_route_test.go +++ b/tests/integration/actions_route_test.go @@ -125,3 +125,38 @@ func TestActionsWebRouteLatestRun(t *testing.T) { assert.Equal(t, workflow.HTMLURL(), resp.Header().Get("Location")) }) } + +func TestActionsArtifactDeletion(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + // create the repo + repo, _, f := CreateDeclarativeRepo(t, user2, "", + []unit_model.Type{unit_model.TypeActions}, nil, + []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: ".gitea/workflows/pr.yml", + ContentReader: strings.NewReader("name: test\non:\n push:\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n - run: echo helloworld\n"), + }, + }, + ) + defer f() + + // a run has been created + assert.Equal(t, 1, unittest.GetCount(t, &actions_model.ActionRun{RepoID: repo.ID})) + + // Load the run we just created + run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: repo.ID}) + err := run.LoadAttributes(context.Background()) + assert.NoError(t, err) + + // Visit it's web view + req := NewRequest(t, "GET", run.HTMLURL()) + resp := MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + // Assert that the artifact deletion markup exists + htmlDoc.AssertElement(t, "[data-locale-confirm-delete-artifact]", true) + }) +}