command/{app, cp, options}, storage/: updates based on pr review

peak · igungor · Oct 22, 2020 · Jul 16, 2020 · Jul 17, 2020 · Jul 17, 2020
commit 377ed32bcb1bb4f9dd1e9a71e74e09bb82b349fc
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/peak/s5cmd/storage"
 	cmpinstall "github.com/posener/complete/cmd/install"
 	"github.com/urfave/cli/v2"
 
@@ -114,35 +113,3 @@ func Main(ctx context.Context, args []string) error {
 
 	return app.RunContext(ctx, args)
 }
-
-// s3opts returns new S3Options object by extracting
-// its fields from the provided context. Region is
-// taken as (default) source-region.
-func s3opts(c *cli.Context) storage.S3Options {
-	region := c.String("source-region")
-	if region == "" {
-		region = c.String("default-source-region")
-	}
-	return storage.S3Options{
-		MaxRetries:  c.Int("retry-count"),
-		Endpoint:    c.String("endpoint-url"),
-		NoVerifySSL: c.Bool("no-verify-ssl"),
-		Region:      region,
-	}
-}
-
-// dstS3opts returns new S3Options object by extracting
-// its fields from the provided context. Region is
-// taken as (default) region, i.e., destination region.
-func dstS3opts(c *cli.Context) storage.S3Options {
-	dstRegion := c.String("region")
-	if dstRegion == "" {
-		dstRegion = c.String("default-region")
-	}
-	return storage.S3Options{
-		MaxRetries:  c.Int("retry-count"),
-		Endpoint:    c.String("endpoint-url"),
-		NoVerifySSL: c.Bool("no-verify-ssl"),
-		Region:      dstRegion,
-	}
-}
@@ -477,11 +477,6 @@ func (c Copy) doUpload(ctx context.Context, srcurl *url.URL, dsturl *url.URL) er
 }
 
 func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) error {
-	srcClient, err := storage.NewClient(srcurl, c.srcS3opts)
-	if err != nil {
-		return err
-	}
-
 	dstClient, err := storage.NewClient(dsturl, c.dstS3opts)
 	if err != nil {
 		return err
@@ -508,6 +503,10 @@ func (c Copy) doCopy(ctx context.Context, srcurl *url.URL, dsturl *url.URL) erro
 	}
 
 	if c.deleteSource {
+		srcClient, err := storage.NewClient(srcurl, c.srcS3opts)
+		if err != nil {
+			return err
+		}
 		if err := srcClient.Delete(ctx, srcurl); err != nil {
 			return err
 		}

diff --git a/command/options.go b/command/options.go
@@ -0,0 +1,38 @@
+package command
+
+import (
+	"github.com/peak/s5cmd/storage"
+	"github.com/urfave/cli/v2"
+)
+
+// s3opts returns new S3Options object by extracting
+// its fields from the provided context. Region is
+// taken as (default) source-region.
+func s3opts(c *cli.Context) storage.S3Options {
+	region := c.String("source-region")
+	if region == "" {
+		region = c.String("default-source-region")
+	}
+	return storage.S3Options{
+		MaxRetries:  c.Int("retry-count"),
+		Endpoint:    c.String("endpoint-url"),
+		NoVerifySSL: c.Bool("no-verify-ssl"),
+		Region:      region,
+	}
+}
+
+// dstS3opts returns new S3Options object by extracting
+// its fields from the provided context. Region is
+// taken as (default) region, i.e., destination region.
+func dstS3opts(c *cli.Context) storage.S3Options {
+	dstRegion := c.String("region")
+	if dstRegion == "" {
+		dstRegion = c.String("default-region")
+	}
+	return storage.S3Options{
+		MaxRetries:  c.Int("retry-count"),
+		Endpoint:    c.String("endpoint-url"),
+		NoVerifySSL: c.Bool("no-verify-ssl"),
+		Region:      dstRegion,
+	}
+}
@@ -668,22 +668,6 @@ func (s *s3Session) newSession(opts S3Options) (*session.Session, error) {
 	return sess, nil
 }
 
-// NumOfSessions returns number of sessions currently active.
-func NumOfSessions() int {
-	s := sessions
-	s.Lock()
-	defer s.Unlock()
-	return len(s.sessions)
-}
-
-// ClearSessions clears all existing sessions.
-func ClearSessions() {
-	s := sessions
-	s.Lock()
-	defer s.Unlock()
-	s.sessions = map[S3Options]*session.Session{}
-}
-
 // customRetryer wraps the SDK's built in DefaultRetryer adding additional
 // error codes. Such as, retry for S3 InternalError code.
 type customRetryer struct {

@@ -9,7 +9,6 @@ import (
 	"math/rand"
 	"net/http"
 	urlpkg "net/url"
-	"os"
 	"reflect"
 	"strings"
 	"testing"
@@ -79,31 +78,6 @@ func TestNewSessionPathStyle(t *testing.T) {
 	}
 }
 
-func TestNewSessionWithRegionSetViaEnv(t *testing.T) {
-	opts := S3Options{
-		Region: "",
-	}
-
-	const expectedRegion = "us-west-2"
-
-	if NumOfSessions() > 0 {
-		ClearSessions()
-	}
-
-	os.Setenv("AWS_REGION", expectedRegion)
-	defer os.Unsetenv("AWS_REGION")
-
-	sess, err := sessions.newSession(opts)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	got := aws.StringValue(sess.Config.Region)
-	if got != expectedRegion {
-		t.Fatalf("expected %v, got %v", expectedRegion, got)
-	}
-}
-
 func TestS3ListSuccess(t *testing.T) {
 	url, err := url.New("s3://bucket/key")
 	if err != nil {
@@ -417,67 +391,6 @@ func TestS3Retry(t *testing.T) {
 	}
 }
 
-func TestNumOfSessions(t *testing.T) {
-	ClearSessions()
-
-	testcases := []struct {
-		name              string
-		sourceRegion      string
-		destinationRegion string
-
-		expectedSessions int
-	}{
-		{
-			name:              "different source-region and region",
-			sourceRegion:      "cn-north-1",
-			destinationRegion: "eu-central-1",
-
-			expectedSessions: 2,
-		},
-		{
-			name:              "same source-region and region",
-			sourceRegion:      "eu-west-1",
-			destinationRegion: "eu-west-1",
-
-			expectedSessions: 1,
-		},
-		{
-			name:             "source-region and region not set",
-			expectedSessions: 1,
-		},
-	}
-	const expectedTotalNumOfSessions = 4
-	for _, tc := range testcases {
-		tc := tc
-
-		t.Run(tc.name, func(t *testing.T) {
-			defer ClearSessions()
-
-			for _, r := range []string{tc.sourceRegion, tc.destinationRegion} {
-				_, err := NewS3Storage(S3Options{
-					Region: r,
-				})
-				if err != nil {
-					t.Error(err)
-				}
-			}
-
-			assert.Equal(t, NumOfSessions(), tc.expectedSessions)
-		})
-	}
-	for _, tc := range testcases {
-		for _, r := range []string{tc.sourceRegion, tc.destinationRegion} {
-			_, err := NewS3Storage(S3Options{
-				Region: r,
-			})
-			if err != nil {
-				t.Error(err)
-			}
-		}
-	}
-	assert.Equal(t, NumOfSessions(), expectedTotalNumOfSessions)
-}
-
 // Credit to aws-sdk-go
 func val(i interface{}, s string) interface{} {
 	v, err := awsutil.ValuesAtPath(i, s)