Merge pull request 'fix(cmd): actions artifacts cannot be migrated' (#4085) from earl-warren/forgejo:wip-migration-artifacts into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4085
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>

cmd/migrate_storage.go

@@ -5,7 +5,9 @@ package cmd
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"io/fs"
 	"strings"
 
 	actions_model "code.gitea.io/gitea/models/actions"
@@ -162,8 +164,20 @@ func migrateActionsLog(ctx context.Context, dstStorage storage.ObjectStorage) er
 
 func migrateActionsArtifacts(ctx context.Context, dstStorage storage.ObjectStorage) error {
 	return db.Iterate(ctx, nil, func(ctx context.Context, artifact *actions_model.ActionArtifact) error {
-		_, err := storage.Copy(dstStorage, artifact.ArtifactPath, storage.ActionsArtifacts, artifact.ArtifactPath)
+		if artifact.Status == int64(actions_model.ArtifactStatusExpired) {
+			return nil
+		}
+
+		_, err := storage.Copy(dstStorage, artifact.StoragePath, storage.ActionsArtifacts, artifact.StoragePath)
+		if err != nil {
+			if errors.Is(err, fs.ErrNotExist) {
+				log.Warn("ignored: actions artifact %s exists in the database but not in storage", artifact.StoragePath)
+				return nil
+			}
 			return err
+		}
+
+		return nil
 	})
 }
 

cmd/migrate_storage_test.go

@@ -5,10 +5,12 @@ package cmd
 
 import (
 	"context"
+	"io"
 	"os"
 	"strings"
 	"testing"
 
+	"code.gitea.io/gitea/models/actions"
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/packages"
 	"code.gitea.io/gitea/models/unittest"
@@ -16,11 +18,28 @@ import (
 	packages_module "code.gitea.io/gitea/modules/packages"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/storage"
+	"code.gitea.io/gitea/modules/test"
 	packages_service "code.gitea.io/gitea/services/packages"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+func createLocalStorage(t *testing.T) (storage.ObjectStorage, string) {
+	t.Helper()
+
+	p := t.TempDir()
+
+	storage, err := storage.NewLocalStorage(
+		context.Background(),
+		&setting.Storage{
+			Path: p,
+		})
+	require.NoError(t, err)
+
+	return storage, p
+}
+
 func TestMigratePackages(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 
@@ -55,14 +74,7 @@ func TestMigratePackages(t *testing.T) {
 
 	ctx := context.Background()
 
-	p := t.TempDir()
-
-	dstStorage, err := storage.NewLocalStorage(
-		ctx,
-		&setting.Storage{
-			Path: p,
-		})
-	assert.NoError(t, err)
+	dstStorage, p := createLocalStorage(t)
 
 	err = migratePackages(ctx, dstStorage)
 	assert.NoError(t, err)
@@ -73,3 +85,50 @@ func TestMigratePackages(t *testing.T) {
 	assert.EqualValues(t, "01", entries[0].Name())
 	assert.EqualValues(t, "tmp", entries[1].Name())
 }
+
+func TestMigrateActionsArtifacts(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	srcStorage, _ := createLocalStorage(t)
+	defer test.MockVariableValue(&storage.ActionsArtifacts, srcStorage)()
+	id := int64(0)
+
+	addArtifact := func(storagePath string, status actions.ArtifactStatus) {
+		id++
+		artifact := &actions.ActionArtifact{
+			ID:           id,
+			ArtifactName: storagePath,
+			StoragePath:  storagePath,
+			Status:       int64(status),
+		}
+		_, err := db.GetEngine(db.DefaultContext).Insert(artifact)
+		require.NoError(t, err)
+		srcStorage.Save(storagePath, strings.NewReader(storagePath), -1)
+	}
+
+	exists := "/exists"
+	addArtifact(exists, actions.ArtifactStatusUploadConfirmed)
+
+	expired := "/expired"
+	addArtifact(expired, actions.ArtifactStatusExpired)
+
+	notFound := "/notfound"
+	addArtifact(notFound, actions.ArtifactStatusUploadConfirmed)
+	srcStorage.Delete(notFound)
+
+	dstStorage, _ := createLocalStorage(t)
+
+	assert.NoError(t, migrateActionsArtifacts(db.DefaultContext, dstStorage))
+
+	object, err := dstStorage.Open(exists)
+	assert.NoError(t, err)
+	buf, err := io.ReadAll(object)
+	require.NoError(t, err)
+	assert.Equal(t, exists, string(buf))
+
+	_, err = dstStorage.Stat(expired)
+	assert.Error(t, err)
+
+	_, err = dstStorage.Stat(notFound)
+	assert.Error(t, err)
+}

release-notes/8.0.0/fix/4085.md

@@ -0,0 +1 @@
+- `forgejo migrate-storage --type actions-artifacts` always fails because it picks the wrong path