Repository: peak/s5cmd (go)

Issue:
Some errors do not result in a non-zero exit code
## System info

**OS:** Alpine Linux 3.13.1
**s5cmd version:** v1.2.1-3fc01af

## Observations

The host is resolvable and reachable, but credentials are not passed through envs:

```shell
/ $ s5cmd --log error --endpoint-url https://minio.localhost cp "s3://tmp/*" tmp/
ERROR session: fetching region failed: NoCredentialProviders: no valid providers in chain. Deprecated.
	For verbose messaging see aws.Config.CredentialsChainVerboseErrors
ERROR "cp s3://tmp/* tmp/": NoCredentialProviders: no valid providers in chain. Deprecated. For verbose messaging see aws.Config.CredentialsChainVerboseErrors
/ $ echo $?
0
```

The host was resolvable, unreachable:
```shell
/ $ s5cmd --log error --endpoint-url https://minio.localhost cp "s3://tmp/*" tmp/
ERROR session: fetching region failed: RequestError: send request failed
caused by: dial tcp 255.255.255.254:443: connect: connection refused
ERROR "cp s3://tmp/* tmp/": RequestError: send request failed caused by: Get "https://minio.localhost/tmp?list-type=2&prefix=": dial tcp 255.255.255.254:443: connect: connection refused
/ $ echo $?
0
```

The host is unresolvable:
```shell
/ $ s5cmd --log error --endpoint-url https://bvsdfdjsfs cp "s3://tmp/*" tmp/
ERROR session: fetching region failed: RequestError: send request failed
caused by: dial tcp: lookup bvsdfdjsfs on 127.0.0.1:53: no such host
ERROR "cp s3://tmp/* tmp/": RequestError: send request failed caused by: Get "https://bvsdfdjsfs/tmp?list-type=2&prefix=": dial tcp: lookup bvsdfdjsfs on 127.0.0.1:53: no such host
/ $ echo $?
0
```

All of that means that we can't be sure that pipelines fail in case there's something wrong with `s3cmd cp`.

Pull request description:
command: append object error to multierror to return non-zero exit code error
* Creating two error objects instead of one, is to avoid data race. Since there is a goroutine running which tries to write `merror` object, there might be a data race for `merror` object in `cp`, `rm` and `select` commands. We can create 1 error object for the goroutine and 1 for the main routine, and append them at the end of the execution. 

Tested #304 's first case, it returns exit code 1. 

Resolves #304 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index db198dc..26767e2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,25 @@
 # Changelog
 
+## not released yet
+
+#### Features
+
+- Added new `--raw` flag to `cp` and `rm` commands. It disables the wildcard operations. It is useful when only an object contains glob characters wants to be downloaded. ([#235](https://github.com/peak/s5cmd/issues/235))
+- Added new `--cache-control` and `--expires` flags to `cp` and `rm` commands. It adds support for setting cache control and expires header to S3 objects. ([#318](https://github.com/peak/s5cmd/pull/318)) [@tombokombo](https://github.com/tombokombo)
+- Added new `select` command. It allows to select JSON records from objects using SQL expressions. ([#299](https://github.com/peak/s5cmd/issues/299)) [@skeggse](https://github.com/skeggse)
+- Added new `--force-glacier-transfer` flag to `cp` command. This flag forces transfer of GLACIER objects whether they are restored or not. ([#206](https://github.com/peak/s5cmd/issues/206))
+- Added new `--source-region` and `destination-region` flags to `cp` command. It allows overriding bucket region. ([#262](https://github.com/peak/s5cmd/issues/262)) [@kemege](https://github.com/kemege)
+- Added new `rb` command which allows users to remove buckets from command line. ([#303](https://github.com/peak/s5cmd/issues/303)).
+
+#### Improvements
+
+- Added new installation option MacPorts. ([#311](https://github.com/peak/s5cmd/pull/311)) [@manojkarthick](https://github.com/manojkarthick)
+
+#### Bugfixes
+
+- Fixed a bug where errors did not result a non-zero exit code. ([#304](https://github.com/peak/s5cmd/issues/304))
+- Change the order of precedence in URL expansion in file system. Glob (*) expansion have precedence over directory expansion. ([#322](https://github.com/peak/s5cmd/pull/322))
+
 ## v1.3.0 - 1 Jul 2021
 
 #### Features
diff --git a/command/cp.go b/command/cp.go
index 81cab7e..68ac115 100644
--- a/command/cp.go
+++ b/command/cp.go
@@ -290,9 +290,13 @@ func (c Copy) Run(ctx context.Context) error {
 
 	waiter := parallel.NewWaiter()
 
+	// create two different error objects instead of single object to avoid the
+	// data race for merror object, since there is a goroutine running,
+	// there might be a data race for a single error object.
 	var (
-		merror    error
-		errDoneCh = make(chan bool)
+		merrorWaiter  error // for the errors from waiter
+		merrorObjects error // for the errors from object channel
+		errDoneCh     = make(chan bool)
 	)
 
 	go func() {
@@ -305,7 +309,7 @@ func (c Copy) Run(ctx context.Context) error {
 				os.Exit(1)
 			}
 			printError(c.fullCommand, c.op, err)
-			merror = multierror.Append(merror, err)
+			merrorWaiter = multierror.Append(merrorWaiter, err)
 		}
 	}()
 
@@ -321,12 +325,14 @@ func (c Copy) Run(ctx context.Context) error {
 		}
 
 		if err := object.Err; err != nil {
+			merrorObjects = multierror.Append(merrorObjects, err)
 			printError(c.fullCommand, c.op, err)
 			continue
 		}
 
 		if object.StorageClass.IsGlacier() && !c.forceGlacierTransfer {
 			err := fmt.Errorf("object '%v' is on Glacier storage", object)
+			merrorObjects = multierror.Append(merrorObjects, err)
 			printError(c.fullCommand, c.op, err)
 			continue
 		}
@@ -351,7 +357,7 @@ func (c Copy) Run(ctx context.Context) error {
 	waiter.Wait()
 	<-errDoneCh
 
-	return merror
+	return multierror.Append(merrorWaiter, merrorObjects).ErrorOrNil()
 }
 
 func (c Copy) prepareCopyTask(
diff --git a/command/rm.go b/command/rm.go
index 98f56ea..81242f2 100644
--- a/command/rm.go
+++ b/command/rm.go
@@ -97,6 +97,14 @@ func (d Delete) Run(ctx context.Context) error {
 
 	objch := expandSources(ctx, client, false, srcurls...)
 
+	// create two different error objects instead of single object to avoid the
+	// data race for merror object, since there is a goroutine running,
+	// there might be a data race for a single error object.
+	var (
+		merrorObjects error
+		merrorResult  error
+	)
+
 	// do object->url transformation
 	urlch := make(chan *url.URL)
 	go func() {
@@ -108,6 +116,7 @@ func (d Delete) Run(ctx context.Context) error {
 			}
 
 			if err := object.Err; err != nil {
+				merrorObjects = multierror.Append(merrorObjects, err)
 				printError(d.fullCommand, d.op, err)
 				continue
 			}
@@ -117,14 +126,13 @@ func (d Delete) Run(ctx context.Context) error {
 
 	resultch := client.MultiDelete(ctx, urlch)
 
-	var merror error
 	for obj := range resultch {
 		if err := obj.Err; err != nil {
 			if errorpkg.IsCancelation(obj.Err) {
 				continue
 			}
 
-			merror = multierror.Append(merror, obj.Err)
+			merrorResult = multierror.Append(merrorResult, obj.Err)
 			printError(d.fullCommand, d.op, obj.Err)
 			continue
 		}
@@ -136,7 +144,7 @@ func (d Delete) Run(ctx context.Context) error {
 		log.Info(msg)
 	}
 
-	return merror
+	return multierror.Append(merrorResult, merrorObjects).ErrorOrNil()
 }
 
 // newSources creates object URL list from given sources.
diff --git a/command/select.go b/command/select.go
index fe50933..225c3ad 100644
--- a/command/select.go
+++ b/command/select.go
@@ -114,7 +114,13 @@ func (s Select) Run(ctx context.Context) error {
 		return err
 	}
 
-	var merror error
+	// create two different error objects instead of single object to avoid the
+	// data race for merror object, since there is a goroutine running,
+	// there might be a data race for a single error object.
+	var (
+		merrorWaiter  error
+		merrorObjects error
+	)
 
 	waiter := parallel.NewWaiter()
 	errDoneCh := make(chan bool)
@@ -125,7 +131,7 @@ func (s Select) Run(ctx context.Context) error {
 		defer close(errDoneCh)
 		for err := range waiter.Err() {
 			printError(s.fullCommand, s.op, err)
-			merror = multierror.Append(merror, err)
+			merrorWaiter = multierror.Append(merrorWaiter, err)
 		}
 	}()
 
@@ -156,12 +162,14 @@ func (s Select) Run(ctx context.Context) error {
 		}
 
 		if err := object.Err; err != nil {
+			merrorObjects = multierror.Append(merrorObjects, err)
 			printError(s.fullCommand, s.op, err)
 			continue
 		}
 
 		if object.StorageClass.IsGlacier() {
 			err := fmt.Errorf("object '%v' is on Glacier storage", object)
+			merrorObjects = multierror.Append(merrorObjects, err)
 			printError(s.fullCommand, s.op, err)
 			continue
 		}
@@ -174,7 +182,7 @@ func (s Select) Run(ctx context.Context) error {
 	<-errDoneCh
 	<-writeDoneCh
 
-	return merror
+	return multierror.Append(merrorWaiter, merrorObjects).ErrorOrNil()
 }
 
 func (s Select) prepareTask(ctx context.Context, client *storage.S3, url *url.URL, resultCh chan<- json.RawMessage) func() error {


Test patch:
diff --git a/e2e/cp_test.go b/e2e/cp_test.go
index 375aa1f..8471813 100644
--- a/e2e/cp_test.go
+++ b/e2e/cp_test.go
@@ -3249,6 +3249,7 @@ func TestCopyMultipleS3ObjectsToS3WithRawMode(t *testing.T) {
 		"file*.txt": "this is a test file 1",
 	}
 
+	// assert s3 objects in destination.
 	for filename, content := range expectedFiles {
 		assert.Assert(t, ensureS3Object(s3client, destBucket, filename, content))
 	}
@@ -3346,3 +3347,28 @@ func TestCopyRawModeAllowDestinationWithoutPrefix(t *testing.T) {
 		t.Errorf("testfile*.txt not exist in S3 bucket %v\n", dst)
 	}
 }
+
+func TestCopyExpectExitCode1OnUnreachableHost(t *testing.T) {
+	t.Parallel()
+
+	const bucket = "bucket"
+
+	_, s5cmd, cleanup := setup(t, withEndpointURL("nonExistingEndpointURL"))
+	defer cleanup()
+
+	folderLayout := []fs.PathOp{
+		fs.WithFile("testfile.txt", "this is a test file 1"),
+	}
+
+	workdir := fs.NewDir(t, "somedir", folderLayout...)
+	defer workdir.Remove()
+
+	src := fmt.Sprintf("s3://%s/*", bucket)
+	src = filepath.ToSlash(src)
+	dst := fmt.Sprintf("%v/", workdir.Path())
+
+	cmd := s5cmd("-r", "0", "cp", src, dst)
+	result := icmd.RunCmd(cmd)
+
+	result.Assert(t, icmd.Expected{ExitCode: 1})
+}
diff --git a/e2e/util_test.go b/e2e/util_test.go
index b0fded2..1c6b6f7 100644
--- a/e2e/util_test.go
+++ b/e2e/util_test.go
@@ -55,7 +55,8 @@ func init() {
 }
 
 type setupOpts struct {
-	s3backend string
+	s3backend   string
+	endpointURL string
 }
 
 type option func(*setupOpts)
@@ -66,6 +67,12 @@ func withS3Backend(backend string) option {
 	}
 }
 
+func withEndpointURL(url string) option {
+	return func(opts *setupOpts) {
+		opts.endpointURL = url
+	}
+}
+
 func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, func()) {
 	t.Helper()
 
@@ -77,7 +84,7 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, f
 		option(opts)
 	}
 
-	endpoint, workdir, cleanup := server(t, opts.s3backend)
+	endpoint, workdir, cleanup := server(t, opts)
 
 	client := s3client(t, storage.Options{
 		Endpoint:    endpoint,
@@ -87,7 +94,7 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd, f
 	return client, s5cmd(workdir, endpoint), cleanup
 }
 
-func server(t *testing.T, s3backend string) (string, string, func()) {
+func server(t *testing.T, setupOptions *setupOpts) (string, string, func()) {
 	t.Helper()
 
 	// testdir := fs.NewDir() tries to create a new directory which
@@ -109,13 +116,15 @@ func server(t *testing.T, s3backend string) (string, string, func()) {
 		s3LogLevel = "info" // aws has no level other than 'debug'
 	}
 
-	endpoint, dbcleanup := s3ServerEndpoint(t, testdir, s3LogLevel, s3backend)
+	endpoint, dbcleanup := s3ServerEndpoint(t, testdir, s3LogLevel, setupOptions.s3backend)
 
 	cleanup := func() {
 		testdir.Remove()
 		dbcleanup()
 	}
-
+	if setupOptions.endpointURL != "" {
+		endpoint = setupOptions.endpointURL
+	}
 	return endpoint, workdir, cleanup
 }
 
@@ -158,6 +167,7 @@ func s5cmd(workdir, endpoint string) func(args ...string) icmd.Cmd {
 		)
 		cmd.Env = env
 		cmd.Dir = workdir
+
 		return cmd
 	}
 }