Merge pull request 'Artifact deletion: port of gitea#27172 and gitea#29241' (#2431) from algernon/forgejo:gitea-port/artifact-deletion into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2431
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

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
+}

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
 

routers/web/repo/actions/view.go

@@ -98,15 +98,16 @@ type ViewRequest struct {
 type ViewResponse struct {
 	State struct {
 		Run struct {
-			Link       string     `json:"link"`
+			Link              string     `json:"link"`
-			Title      string     `json:"title"`
+			Title             string     `json:"title"`
-			Status     string     `json:"status"`
+			Status            string     `json:"status"`
-			CanCancel  bool       `json:"canCancel"`
+			CanCancel         bool       `json:"canCancel"`
-			CanApprove bool       `json:"canApprove"` // the run needs an approval and the doer has permission to approve
+			CanApprove        bool       `json:"canApprove"` // the run needs an approval and the doer has permission to approve
-			CanRerun   bool       `json:"canRerun"`
+			CanRerun          bool       `json:"canRerun"`
-			Done       bool       `json:"done"`
+			CanDeleteArtifact bool       `json:"canDeleteArtifact"`
-			Jobs       []*ViewJob `json:"jobs"`
+			Done              bool       `json:"done"`
-			Commit     ViewCommit `json:"commit"`
+			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)

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)
 				})
 			})

services/actions/cleanup.go

@@ -20,23 +20,59 @@ 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
 	}
 	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
 }
+
+// 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 := 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 {
+			log.Debug("No more artifacts pending deletion")
+			break
+		}
+	}
+	return nil
+}

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"}}"

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)
+	})
+}

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() {
               <a class="job-artifacts-link" target="_blank" :href="run.link+'/artifacts/'+artifact.name">
                 <SvgIcon name="octicon-file" class="ui text black job-artifacts-icon"/>{{ artifact.name }}
               </a>
+              <a v-if="run.canDeleteArtifact" @click="deleteArtifact(artifact.name)" class="job-artifacts-delete">
+                <SvgIcon name="octicon-trash" class="ui text black job-artifacts-icon"/>
+              </a>
             </li>
           </ul>
         </div>
@@ -528,6 +539,8 @@ export function initRepositoryActionView() {
 .job-artifacts-item {
   margin: 5px 0;
   padding: 6px;
+  display: flex;
+  justify-content: space-between;
 }
 
 .job-artifacts-list {

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,