Merge branch 'sh-cleanup-lfs-preparer' into 'master'

Reduce code duplication in LFS upload preparer

See merge request gitlab-org/gitlab-workhorse!560

internal/lfs/lfs.go

@@ -32,23 +32,16 @@ func (l *object) Verify(fh *filestore.FileHandler) error {
 }
 
 type uploadPreparer struct {
-	credentials config.ObjectStorageCredentials
+	objectPreparer upload.Preparer
 }
 
-func NewLfsUploadPreparer(c config.Config) upload.Preparer {
-	creds := c.ObjectStorageCredentials
-
-	if creds == nil {
-		creds = &config.ObjectStorageCredentials{}
-	}
-
-	return &uploadPreparer{credentials: *creds}
+func NewLfsUploadPreparer(c config.Config, objectPreparer upload.Preparer) upload.Preparer {
+	return &uploadPreparer{objectPreparer: objectPreparer}
 }
 
 func (l *uploadPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, upload.Verifier, error) {
-	opts := filestore.GetOpts(a)
+	opts, _, _ := l.objectPreparer.Prepare(a)
 	opts.TempFilePrefix = a.LfsOid
-	opts.ObjectStorageConfig.S3Credentials = l.credentials.S3Credentials
 
 	return opts, &object{oid: a.LfsOid, size: a.LfsSize}, nil
 }

internal/lfs/lfs_test.go

+package lfs_test
+
+import (
+	"testing"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/lfs"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upload"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestLfsUploadPreparerWithConfig(t *testing.T) {
+	lfsOid := "abcd1234"
+	creds := config.S3Credentials{
+		AwsAccessKeyID:     "test-key",
+		AwsSecretAccessKey: "test-secret",
+	}
+
+	c := config.Config{
+		ObjectStorageCredentials: &config.ObjectStorageCredentials{
+			Provider:      "AWS",
+			S3Credentials: creds,
+		},
+	}
+
+	r := &api.Response{
+		LfsOid: lfsOid,
+		RemoteObject: api.RemoteObject{
+			UseWorkhorseClient: true,
+			ObjectStorage: &api.ObjectStorageParams{
+				Provider: "AWS",
+			},
+		},
+	}
+
+	uploadPreparer := upload.NewObjectStoragePreparer(c)
+	lfsPreparer := lfs.NewLfsUploadPreparer(c, uploadPreparer)
+	opts, verifier, err := lfsPreparer.Prepare(r)
+
+	require.NoError(t, err)
+	require.Equal(t, lfsOid, opts.TempFilePrefix)
+	require.True(t, opts.ObjectStorageConfig.IsAWS())
+	require.True(t, opts.UseWorkhorseClient)
+	require.Equal(t, creds, opts.ObjectStorageConfig.S3Credentials)
+	require.NotNil(t, verifier)
+}
+
+func TestLfsUploadPreparerWithNoConfig(t *testing.T) {
+	c := config.Config{}
+	r := &api.Response{}
+	uploadPreparer := upload.NewObjectStoragePreparer(c)
+	lfsPreparer := lfs.NewLfsUploadPreparer(c, uploadPreparer)
+	opts, verifier, err := lfsPreparer.Prepare(r)
+
+	require.NoError(t, err)
+	require.False(t, opts.UseWorkhorseClient)
+	require.NotNil(t, verifier)
+}

internal/upstream/routes.go

@@ -283,7 +283,7 @@ func createUploadPreparers(cfg config.Config) uploadPreparers {
 
 	return uploadPreparers{
 		artifacts: defaultPreparer,
-		lfs:       lfs.NewLfsUploadPreparer(cfg),
+		lfs:       lfs.NewLfsUploadPreparer(cfg, defaultPreparer),
 		packages:  defaultPreparer,
 		uploads:   defaultPreparer,
 	}