Repository: docker/compose-cli
Pull request #711 (2020-10-01 15:29:27)

ACI tests : Wait for http status and retry if necessary. 
We were getting 500 errors because database is not ready initially

Signed-off-by: Guillaume Tardif <guillaume.tardif@docker.com>

**What I did**
* do not only retry GET on errors, also retry if status is not the expected one.

**Related issue**
https://github.com/docker/compose-cli/runs/1193172850

<!-- optional tests
You can add a / mention to run tests executed by default only on main branch : 
* `test-aci` to run ACI E2E tests
* `test-ecs` to run ECS E2E tests
* `test-windows` to run tests & E2E tests on windows
-->
/test-aci

**(not mandatory) A picture of a cute animal, if possible in relation with what you did**



Patch:
diff --git a/Makefile b/Makefile
index ef5205fc..6eca2482 100644
--- a/Makefile
+++ b/Makefile
@@ -96,8 +96,6 @@ validate-go-mod: ## Validate go.mod and go.sum are up-to-date
 
 validate: validate-go-mod validate-headers ## Validate sources
 
-pre-commit: all lint validate test e2e-local
-
 help: ## Show help
 	@echo Please specify a build target. The choices are:
 	@grep -E '^[0-9a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'
diff --git a/cli/cmd/run/run.go b/cli/cmd/run/run.go
index e1088475..4dc1b5af 100644
--- a/cli/cmd/run/run.go
+++ b/cli/cmd/run/run.go
@@ -56,7 +56,6 @@ func Command(contextType string) *cobra.Command {
 	cmd.Flags().Float64Var(&opts.Cpus, "cpus", 1., "Number of CPUs")
 	cmd.Flags().VarP(&opts.Memory, "memory", "m", "Memory limit")
 	cmd.Flags().StringArrayVarP(&opts.Environment, "env", "e", []string{}, "Set environment variables")
-	cmd.Flags().StringArrayVar(&opts.EnvironmentFiles, "envFile", []string{}, "Path to environment files to be translated as environment variables")
 	cmd.Flags().StringVarP(&opts.RestartPolicyCondition, "restart", "", containers.RestartPolicyNone, "Restart policy to apply when a container exits")
 
 	if contextType == store.AciContextType {
diff --git a/cli/options/run/opts.go b/cli/options/run/opts.go
index 9a3130aa..d38c7fe4 100644
--- a/cli/options/run/opts.go
+++ b/cli/options/run/opts.go
@@ -21,8 +21,6 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/docker/cli/opts"
-
 	"github.com/docker/compose-cli/utils"
 
 	"github.com/docker/docker/pkg/namesgenerator"
@@ -43,7 +41,6 @@ type Opts struct {
 	Memory                 formatter.MemBytes
 	Detach                 bool
 	Environment            []string
-	EnvironmentFiles       []string
 	RestartPolicyCondition string
 	DomainName             string
 }
@@ -69,15 +66,6 @@ func (r *Opts) ToContainerConfig(image string) (containers.ContainerConfig, erro
 		return containers.ContainerConfig{}, err
 	}
 
-	envVars := r.Environment
-	for _, f := range r.EnvironmentFiles {
-		vars, err := opts.ParseEnvFile(f)
-		if err != nil {
-			return containers.ContainerConfig{}, err
-		}
-		envVars = append(envVars, vars...)
-	}
-
 	return containers.ContainerConfig{
 		ID:                     r.Name,
 		Image:                  image,
@@ -87,7 +75,7 @@ func (r *Opts) ToContainerConfig(image string) (containers.ContainerConfig, erro
 		Volumes:                r.Volumes,
 		MemLimit:               r.Memory,
 		CPULimit:               r.Cpus,
-		Environment:            envVars,
+		Environment:            r.Environment,
 		RestartPolicyCondition: restartPolicy,
 		DomainName:             r.DomainName,
 	}, nil


Test patch:
diff --git a/cli/cmd/run/run_test.go b/cli/cmd/run/run_test.go
index 789fe400..78b23c54 100644
--- a/cli/cmd/run/run_test.go
+++ b/cli/cmd/run/run_test.go
@@ -23,8 +23,6 @@ import (
 
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/golden"
-
-	"github.com/docker/compose-cli/cli/options/run"
 )
 
 func TestHelp(t *testing.T) {
@@ -42,24 +40,3 @@ func TestHelpNoDomainFlag(t *testing.T) {
 	_ = c.Help()
 	assert.Assert(t, !strings.Contains(b.String(), "domainname"))
 }
-
-func TestRunEnvironmentFiles(t *testing.T) {
-	runOpts := run.Opts{
-		Environment: []string{
-			"VAR=1",
-		},
-		EnvironmentFiles: []string{
-			"testdata/runtest1.env",
-			"testdata/runtest2.env",
-		},
-	}
-	containerConfig, err := runOpts.ToContainerConfig("test")
-	assert.NilError(t, err)
-	assert.DeepEqual(t, containerConfig.Environment, []string{
-		"VAR=1",
-		"FIRST_VAR=\"firstValue\"",
-		"SECOND_VAR=secondValue",
-		"THIRD_VAR=2",
-		"FOURTH_VAR=fourthValue",
-	})
-}
diff --git a/cli/cmd/run/testdata/run-help.golden b/cli/cmd/run/testdata/run-help.golden
index 71692ccd..178c63b0 100644
--- a/cli/cmd/run/testdata/run-help.golden
+++ b/cli/cmd/run/testdata/run-help.golden
@@ -8,7 +8,6 @@ Flags:
   -d, --detach                Run container in background and print container ID
       --domainname string     Container NIS domain name
   -e, --env stringArray       Set environment variables
-      --envFile stringArray   Path to environment files to be translated as environment variables
   -l, --label stringArray     Set meta data on a container
   -m, --memory bytes          Memory limit
       --name string           Assign a name to the container
diff --git a/cli/cmd/run/testdata/runtest1.env b/cli/cmd/run/testdata/runtest1.env
deleted file mode 100644
index 495661e3..00000000
--- a/cli/cmd/run/testdata/runtest1.env
+++ /dev/null
@@ -1,2 +0,0 @@
-FIRST_VAR="firstValue"
-SECOND_VAR=secondValue
diff --git a/cli/cmd/run/testdata/runtest2.env b/cli/cmd/run/testdata/runtest2.env
deleted file mode 100644
index d6182fbf..00000000
--- a/cli/cmd/run/testdata/runtest2.env
+++ /dev/null
@@ -1,2 +0,0 @@
-THIRD_VAR=2
-FOURTH_VAR=fourthValue
diff --git a/tests/aci-e2e/e2e-aci_test.go b/tests/aci-e2e/e2e-aci_test.go
index 4d08e92d..aa51a47f 100644
--- a/tests/aci-e2e/e2e-aci_test.go
+++ b/tests/aci-e2e/e2e-aci_test.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"io/ioutil"
 	"net/http"
 	"net/url"
 	"os"
@@ -31,8 +30,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/docker/compose-cli/aci/convert"
-
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/icmd"
@@ -43,6 +40,7 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 
 	"github.com/docker/compose-cli/aci"
+	"github.com/docker/compose-cli/aci/convert"
 	"github.com/docker/compose-cli/aci/login"
 	"github.com/docker/compose-cli/api/containers"
 	"github.com/docker/compose-cli/context/store"
@@ -252,12 +250,8 @@ func TestContainerRunVolume(t *testing.T) {
 	})
 
 	t.Run("http get", func(t *testing.T) {
-		r, err := HTTPGetWithRetry(endpoint, 3)
-		assert.NilError(t, err)
-		assert.Equal(t, r.StatusCode, http.StatusOK)
-		b, err := ioutil.ReadAll(r.Body)
-		assert.NilError(t, err)
-		assert.Assert(t, strings.Contains(string(b), testFileContent), "Actual content: "+string(b))
+		output := HTTPGetWithRetry(t, endpoint, http.StatusOK, 2*time.Second, 20*time.Second)
+		assert.Assert(t, strings.Contains(output, testFileContent), "Actual content: "+output)
 	})
 
 	t.Run("logs", func(t *testing.T) {
@@ -363,9 +357,9 @@ func TestContainerRunAttached(t *testing.T) {
 			"--restart", "on-failure",
 			"--memory", "0.1G", "--cpus", "0.1",
 			"-p", "80:80",
+			"nginx",
 			"--domainname",
 			dnsLabelName,
-			"nginx",
 		)
 		followLogsProcess = icmd.StartCmd(cmd)
 
@@ -499,17 +493,12 @@ func TestComposeUpUpdate(t *testing.T) {
 		assert.Assert(t, is.Len(containerInspect.Ports, 1))
 		endpoint := fmt.Sprintf("http://%s:%d", containerInspect.Ports[0].HostIP, containerInspect.Ports[0].HostPort)
 
-		r, err := HTTPGetWithRetry(endpoint+"/words/noun", 3)
-		assert.NilError(t, err)
-		assert.Equal(t, r.StatusCode, http.StatusOK)
-		b, err := ioutil.ReadAll(r.Body)
-		assert.NilError(t, err)
-		assert.Assert(t, strings.Contains(string(b), `"word":`))
+		output := HTTPGetWithRetry(t, endpoint+"/words/noun", http.StatusOK, 2*time.Second, 20*time.Second)
+
+		assert.Assert(t, strings.Contains(output, `"word":`))
 
 		endpoint = fmt.Sprintf("http://%s:%d", fqdn, containerInspect.Ports[0].HostPort)
-		r, err = HTTPGetWithRetry(endpoint+"/words/noun", 3)
-		assert.NilError(t, err)
-		assert.Equal(t, r.StatusCode, http.StatusOK)
+		HTTPGetWithRetry(t, endpoint+"/words/noun", http.StatusOK, 2*time.Second, 20*time.Second)
 	})
 
 	t.Run("compose ps", func(t *testing.T) {
@@ -577,14 +566,7 @@ func TestComposeUpUpdate(t *testing.T) {
 				assert.Equal(t, containerInspect.Ports[0].HostPort, uint32(8080))
 				assert.Equal(t, containerInspect.Ports[0].ContainerPort, uint32(8080))
 			}
-			checkUp := func(t poll.LogT) poll.Result {
-				r, _ := http.Get(endpoint + route)
-				if r != nil && r.StatusCode == http.StatusOK {
-					return poll.Success()
-				}
-				return poll.Continue("Waiting for container to serve request")
-			}
-			poll.WaitOn(t, checkUp, poll.WithDelay(1*time.Second), poll.WithTimeout(60*time.Second))
+			HTTPGetWithRetry(t, endpoint+route, http.StatusOK, 1*time.Second, 60*time.Second)
 
 			res = c.RunDockerCmd("ps")
 			p := containerInspect.Ports[0]
diff --git a/tests/framework/e2e.go b/tests/framework/e2e.go
index 1e79c18c..d15313cf 100644
--- a/tests/framework/e2e.go
+++ b/tests/framework/e2e.go
@@ -34,6 +34,7 @@ import (
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/icmd"
+	"gotest.tools/v3/poll"
 
 	"github.com/docker/compose-cli/api/containers"
 )
@@ -201,19 +202,28 @@ func ParseContainerInspect(stdout string) (*containers.Container, error) {
 }
 
 // HTTPGetWithRetry performs an HTTP GET on an `endpoint`.
-// In the case of an error it retries the same request after a 5 second sleep,
-// returning the error if count of `tries` is reached
-func HTTPGetWithRetry(endpoint string, tries int) (*http.Response, error) {
+// In the case of an error or the response status is not the expeted one, it retries the same request,
+// returning the response body as a string (empty if we could not reach it)
+func HTTPGetWithRetry(t *testing.T, endpoint string, expectedStatus int, retryDelay time.Duration, timeout time.Duration) string {
 	var (
 		r   *http.Response
 		err error
 	)
-	for t := 0; t < tries; t++ {
+	checkUp := func(t poll.LogT) poll.Result {
 		r, err = http.Get(endpoint)
-		if err == nil || t == tries-1 {
-			break
+		if err != nil {
+			return poll.Continue("reaching %q: Error %s", endpoint, err.Error())
+		}
+		if r.StatusCode == expectedStatus {
+			return poll.Success()
 		}
-		time.Sleep(5 * time.Second)
+		return poll.Continue("reaching %q: %d != %d", endpoint, r.StatusCode, expectedStatus)
+	}
+	poll.WaitOn(t, checkUp, poll.WithDelay(retryDelay), poll.WithTimeout(timeout))
+	if r != nil {
+		b, err := ioutil.ReadAll(r.Body)
+		assert.NilError(t, err)
+		return string(b)
 	}
-	return r, err
+	return ""
 }