Repository: peak/s5cmd
Pull request #424 (2022-04-06 09:04:20)

app: rename `use-v1-api` to `use-list-objects-v1`
Let's be more specific about which V1 of API to use.



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6f180dc..abe28c6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,11 +2,14 @@
 
 ## not released yet
 
+#### Breaking changes
+- Dropped inline comment feature for `run` command. Previously s5cmd supported a command with an inline comment like `ls s3://bucket/object.gz  # inline comment`. ([#309](https://github.com/peak/s5cmd/issues/309))
+
 #### Features
 - Added `--ignore-glacier-warnings` flag to `cp`, `mv` and `select` commands. ([#346](https://github.com/peak/s5cmd/issues/346))
-- Added `--force-glacier-transfer` flag to `select` command. ([#404](https://github.com/peak/s5cmd/pull/404))
+- Added `--force-glacier-transfer` flag to `select` command. ([#346](https://github.com/peak/s5cmd/issues/346))
 - Added AWS Single Sign-On (SSO) profiles support ([#385](https://github.com/peak/s5cmd/issues/385))
-- Added `--use-v1-api` flag to force using S3 ListObjects API instead of ListObjectsV2 API. ([#405](https://github.com/peak/s5cmd/issues/405)
+- Added `--use-list-objects-v1` flag to force using S3 ListObjects API instead of ListObjectsV2 API. ([#405](https://github.com/peak/s5cmd/issues/405)
 
 #### Improvements
 
diff --git a/README.md b/README.md
index 423c13b..a6af2da 100644
--- a/README.md
+++ b/README.md
@@ -378,10 +378,11 @@ cp, mv, rm, mb ...
 
 ### S3 ListObjects API Backward Compatibility
 
-The `--use-v1-api` flag will force using S3 ListObjects API instead of ListObjectsV2 API.
+The `--use-list-objects-v1` flag will force using S3 ListObjectsV1 API. This
+flag is useful for services that do not support ListObjectsV2 API.
 
 ```
-s5cmd --use-v1-api ls
+s5cmd --use-list-objects-v1 ls s3://bucket/
 ```
 
 ### Specifying credentials
diff --git a/command/app.go b/command/app.go
index bb815e8..e03c02f 100644
--- a/command/app.go
+++ b/command/app.go
@@ -70,8 +70,8 @@ var app = &cli.App{
 			Usage: "do not sign requests: credentials will not be loaded if --no-sign-request is provided",
 		},
 		&cli.BoolFlag{
-			Name:  "use-v1-api",
-			Usage: "enable backward compatibility with ListObjects API and disable ListObjectsV2 API",
+			Name:  "use-list-objects-v1",
+			Usage: "use ListObjectsV1 API for services that don't support ListObjectsV2",
 		},
 	},
 	Before: func(c *cli.Context) error {
@@ -138,12 +138,12 @@ var app = &cli.App{
 // NewStorageOpts creates storage.Options object from the given context.
 func NewStorageOpts(c *cli.Context) storage.Options {
 	return storage.Options{
-		MaxRetries:    c.Int("retry-count"),
-		Endpoint:      c.String("endpoint-url"),
-		NoVerifySSL:   c.Bool("no-verify-ssl"),
-		DryRun:        c.Bool("dry-run"),
-		NoSignRequest: c.Bool("no-sign-request"),
-		APIv1Enabled:  c.Bool("use-v1-api"),
+		MaxRetries:       c.Int("retry-count"),
+		Endpoint:         c.String("endpoint-url"),
+		NoVerifySSL:      c.Bool("no-verify-ssl"),
+		DryRun:           c.Bool("dry-run"),
+		NoSignRequest:    c.Bool("no-sign-request"),
+		UseListObjectsV1: c.Bool("use-list-objects-v1"),
 	}
 }
 
diff --git a/command/run.go b/command/run.go
index d7596a9..37e8cb9 100644
--- a/command/run.go
+++ b/command/run.go
@@ -102,14 +102,12 @@ func (r Run) Run(ctx context.Context) error {
 	for line := range scanner.Scan() {
 		lineno++
 
-		// support inline comments
-		line = strings.Split(line, " #")[0]
-
 		line = strings.TrimSpace(line)
 		if line == "" {
 			continue
 		}
 
+		// skip comment lines
 		if strings.HasPrefix(line, "#") {
 			continue
 		}
diff --git a/storage/s3.go b/storage/s3.go
index 5eac70b..b211cf5 100644
--- a/storage/s3.go
+++ b/storage/s3.go
@@ -53,12 +53,12 @@ var globalSessionCache = &SessionCache{
 // S3 is a storage type which interacts with S3API, DownloaderAPI and
 // UploaderAPI.
 type S3 struct {
-	api         s3iface.S3API
-	downloader  s3manageriface.DownloaderAPI
-	uploader    s3manageriface.UploaderAPI
-	endpointURL urlpkg.URL
-	dryRun      bool
-	v1Enabled   bool
+	api              s3iface.S3API
+	downloader       s3manageriface.DownloaderAPI
+	uploader         s3manageriface.UploaderAPI
+	endpointURL      urlpkg.URL
+	dryRun           bool
+	useListObjectsV1 bool
 }
 
 func parseEndpoint(endpoint string) (urlpkg.URL, error) {
@@ -91,12 +91,12 @@ func newS3Storage(ctx context.Context, opts Options) (*S3, error) {
 	}
 
 	return &S3{
-		api:         s3.New(awsSession),
-		downloader:  s3manager.NewDownloader(awsSession),
-		uploader:    s3manager.NewUploader(awsSession),
-		endpointURL: endpointURL,
-		dryRun:      opts.DryRun,
-		v1Enabled:   opts.APIv1Enabled,
+		api:              s3.New(awsSession),
+		downloader:       s3manager.NewDownloader(awsSession),
+		uploader:         s3manager.NewUploader(awsSession),
+		endpointURL:      endpointURL,
+		dryRun:           opts.DryRun,
+		useListObjectsV1: opts.UseListObjectsV1,
 	}, nil
 }
 
@@ -127,12 +127,10 @@ func (s *S3) Stat(ctx context.Context, url *url.URL) (*Object, error) {
 // keys. If no object found or an error is encountered during this period,
 // it sends these errors to object channel.
 func (s *S3) List(ctx context.Context, url *url.URL, _ bool) <-chan *Object {
-	if isGoogleEndpoint(s.endpointURL) {
-		return s.listObjects(ctx, url)
-	}
-	if s.v1Enabled {
+	if isGoogleEndpoint(s.endpointURL) || s.useListObjectsV1 {
 		return s.listObjects(ctx, url)
 	}
+
 	return s.listObjectsV2(ctx, url)
 }
 
diff --git a/storage/storage.go b/storage/storage.go
index 7f8c3fe..e8e82ab 100644
--- a/storage/storage.go
+++ b/storage/storage.go
@@ -48,14 +48,14 @@ func NewLocalClient(opts Options) *Filesystem {
 
 func NewRemoteClient(ctx context.Context, url *url.URL, opts Options) (*S3, error) {
 	newOpts := Options{
-		MaxRetries:    opts.MaxRetries,
-		Endpoint:      opts.Endpoint,
-		NoVerifySSL:   opts.NoVerifySSL,
-		DryRun:        opts.DryRun,
-		NoSignRequest: opts.NoSignRequest,
-		APIv1Enabled:  opts.APIv1Enabled,
-		bucket:        url.Bucket,
-		region:        opts.region,
+		MaxRetries:       opts.MaxRetries,
+		Endpoint:         opts.Endpoint,
+		NoVerifySSL:      opts.NoVerifySSL,
+		DryRun:           opts.DryRun,
+		NoSignRequest:    opts.NoSignRequest,
+		UseListObjectsV1: opts.UseListObjectsV1,
+		bucket:           url.Bucket,
+		region:           opts.region,
 	}
 	return newS3Storage(ctx, newOpts)
 }
@@ -69,14 +69,14 @@ func NewClient(ctx context.Context, url *url.URL, opts Options) (Storage, error)
 
 // Options stores configuration for storage.
 type Options struct {
-	MaxRetries    int
-	Endpoint      string
-	NoVerifySSL   bool
-	DryRun        bool
-	NoSignRequest bool
-	APIv1Enabled  bool
-	bucket        string
-	region        string
+	MaxRetries       int
+	Endpoint         string
+	NoVerifySSL      bool
+	DryRun           bool
+	NoSignRequest    bool
+	UseListObjectsV1 bool
+	bucket           string
+	region           string
 }
 
 func (o *Options) SetRegion(region string) {


Test patch:
diff --git a/e2e/run_test.go b/e2e/run_test.go
index 94d3f2b..b695222 100644
--- a/e2e/run_test.go
+++ b/e2e/run_test.go
@@ -25,9 +25,10 @@ func TestRunFromStdin(t *testing.T) {
 
 	input := strings.NewReader(
 		strings.Join([]string{
-			fmt.Sprintf("ls s3://%v/file1.txt", bucket),
 			" # this is a comment",
-			fmt.Sprintf("ls s3://%v/file2.txt # this is an inline comment", bucket),
+			fmt.Sprintf("ls s3://%v/file1.txt", bucket),
+			"# this one too",
+			fmt.Sprintf("ls s3://%v/file2.txt", bucket),
 		}, "\n"),
 	)
 	cmd := s5cmd("run")
@@ -43,6 +44,35 @@ func TestRunFromStdin(t *testing.T) {
 	assertLines(t, result.Stderr(), map[int]compareFunc{})
 }
 
+func TestRunFromStdinIssue309(t *testing.T) {
+	t.Parallel()
+
+	bucket := s3BucketFromTestName(t)
+
+	s3client, s5cmd, cleanup := setup(t)
+	defer cleanup()
+
+	createBucket(t, s3client, bucket)
+	putFile(t, s3client, bucket, "test #3 /bar.jpg", "content")
+
+	input := strings.NewReader(
+		strings.Join([]string{
+			" # this is a comment",
+			fmt.Sprintf("mv 's3://%v/test #3 /bar.jpg' 's3://%v/test #3/bar.jpg'", bucket, bucket),
+		}, "\n"),
+	)
+	cmd := s5cmd("run")
+	result := icmd.RunCmd(cmd, icmd.WithStdin(input))
+
+	result.Assert(t, icmd.Success)
+
+	assertLines(t, result.Stdout(), map[int]compareFunc{
+		0: suffix("mv s3://test-run-from-stdin-issue-309/test #3 /bar.jpg s3://test-run-from-stdin-issue-309/test #3/bar.jpg"),
+	})
+
+	assertLines(t, result.Stderr(), map[int]compareFunc{})
+}
+
 func TestRunFromStdinWithErrors(t *testing.T) {
 	t.Parallel()
 
diff --git a/storage/s3_test.go b/storage/s3_test.go
index f0cbd98..b4a049c 100644
--- a/storage/s3_test.go
+++ b/storage/s3_test.go
@@ -139,7 +139,7 @@ func TestS3ListURL(t *testing.T) {
 		api: mockApi,
 	}
 
-	mockApi.Handlers.Send.Clear() // mock sending
+	mockApi.Handlers.Send.Clear()
 	mockApi.Handlers.Unmarshal.Clear()
 	mockApi.Handlers.UnmarshalMeta.Clear()
 	mockApi.Handlers.ValidateResponse.Clear()
@@ -241,7 +241,7 @@ func TestS3ListNoItemFound(t *testing.T) {
 		api: mockApi,
 	}
 
-	mockApi.Handlers.Send.Clear() // mock sending
+	mockApi.Handlers.Send.Clear()
 	mockApi.Handlers.Unmarshal.Clear()
 	mockApi.Handlers.UnmarshalMeta.Clear()
 	mockApi.Handlers.ValidateResponse.Clear()
@@ -457,7 +457,7 @@ func TestS3Retry(t *testing.T) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 
-			mockApi.Handlers.Send.Clear() // mock sending
+			mockApi.Handlers.Send.Clear()
 			mockApi.Handlers.Unmarshal.Clear()
 			mockApi.Handlers.UnmarshalMeta.Clear()
 			mockApi.Handlers.ValidateResponse.Clear()
@@ -985,6 +985,57 @@ func TestSessionAutoRegion(t *testing.T) {
 	}
 }
 
+func TestS3ListObjectsAPIVersions(t *testing.T) {
+	url, err := url.New("s3://bucket/key")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+
+	mockApi := s3.New(unit.Session)
+	mockS3 := &S3{api: mockApi}
+
+	mockApi.Handlers.Send.Clear()
+	mockApi.Handlers.Unmarshal.Clear()
+	mockApi.Handlers.UnmarshalMeta.Clear()
+	mockApi.Handlers.ValidateResponse.Clear()
+
+	t.Run("list-objects-v2", func(t *testing.T) {
+		var got interface{}
+		mockApi.Handlers.ValidateResponse.PushBack(func(r *request.Request) {
+			got = r.Data
+		})
+
+		ctx := context.Background()
+		mockS3.useListObjectsV1 = false
+		for range mockS3.List(ctx, url, false) {
+		}
+
+		expected := &s3.ListObjectsV2Output{}
+
+		if reflect.TypeOf(expected) != reflect.TypeOf(got) {
+			t.Errorf("expected %T, got: %T", expected, got)
+		}
+	})
+
+	t.Run("list-objects-v1", func(t *testing.T) {
+		var got interface{}
+		mockApi.Handlers.ValidateResponse.PushBack(func(r *request.Request) {
+			got = r.Data
+		})
+
+		ctx := context.Background()
+		mockS3.useListObjectsV1 = true
+		for range mockS3.List(ctx, url, false) {
+		}
+
+		expected := &s3.ListObjectsOutput{}
+
+		if reflect.TypeOf(expected) != reflect.TypeOf(got) {
+			t.Errorf("expected %T, got: %T", expected, got)
+		}
+	})
+}
+
 func valueAtPath(i interface{}, s string) interface{} {
 	v, err := awsutil.ValuesAtPath(i, s)
 	if err != nil || len(v) == 0 {