Address PR feedback

Author: efekarakus

ecs-cli/modules/cli/local/network/generate_mock.go

@@ -13,4 +13,4 @@
 
 package network
 
-//go:generate /Users/karakuse/go/src/github.com/aws/amazon-ecs-cli/scripts/mockgen.sh github.com/aws/amazon-ecs-cli/ecs-cli/modules/cli/local/network IdempotentLocalEndpointsStarter mock_network/network.go
+//go:generate mockgen.sh github.com/aws/amazon-ecs-cli/ecs-cli/modules/cli/local/network LocalEndpointsStarter mock_network/network.go

ecs-cli/modules/cli/local/network/mock_network/network.go

@@ -20,33 +20,28 @@ import (
 	"strings"
 	"time"
 
-	"github.com/docker/docker/client"
-
+	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
-
 	"github.com/docker/docker/api/types/filters"
-
+	"github.com/docker/docker/api/types/network"
+	"github.com/docker/docker/client"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
-
-	"github.com/docker/docker/api/types/network"
-
-	"github.com/docker/docker/api/types"
 )
 
-// IdempotentLocalEndpointsStarter groups Docker functions to create the ECS local network and
+// LocalEndpointsStarter groups Docker functions to create the ECS local network and
 // the Local Container Endpoints container if they don't exist.
-type IdempotentLocalEndpointsStarter interface {
-	idempotentNetworkCreator
-	idempotentContainerStarter
+type LocalEndpointsStarter interface {
+	networkCreator
+	containerStarter
 }
 
-type idempotentNetworkCreator interface {
+type networkCreator interface {
 	NetworkInspect(ctx context.Context, networkID string, options types.NetworkInspectOptions) (types.NetworkResource, error)
 	NetworkCreate(ctx context.Context, name string, options types.NetworkCreate) (types.NetworkCreateResponse, error)
 }
 
-type idempotentContainerStarter interface {
+type containerStarter interface {
 	ContainerList(ctx context.Context, options types.ContainerListOptions) ([]types.Container, error)
 	ContainerCreate(ctx context.Context, config *container.Config,
 		hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig,
@@ -85,26 +80,24 @@ const (
 // already exists or the container is already running then the operation is a no-op.
 //
 // If there is any unexpected errors, we exit the program with a fatal log.
-func Setup(docker IdempotentLocalEndpointsStarter) {
-	setupLocalNetwork(docker)
-	setupLocalEndpointsContainer(docker)
+func Setup(dockerClient LocalEndpointsStarter) {
+	setupLocalNetwork(dockerClient)
+	setupLocalEndpointsContainer(dockerClient)
 }
 
-// setupLocalNetwork creates the local network if it doesn't already exist.
-func setupLocalNetwork(docker idempotentNetworkCreator) {
-	if localNetworkExists(docker) {
+func setupLocalNetwork(dockerClient networkCreator) {
+	if localNetworkExists(dockerClient) {
 		logrus.Infof("The network %s already exists", EcsLocalNetworkName)
 		return
 	}
-	createLocalNetwork(docker)
+	createLocalNetwork(dockerClient)
 }
 
-// localNetworkExists returns true if the local network already exists, false otherwise.
-func localNetworkExists(docker idempotentNetworkCreator) bool {
+func localNetworkExists(dockerClient networkCreator) bool {
 	ctx, cancel := context.WithTimeout(context.Background(), dockerTimeout)
 	defer cancel()
 
-	_, err := docker.NetworkInspect(ctx, EcsLocalNetworkName, types.NetworkInspectOptions{})
+	_, err := dockerClient.NetworkInspect(ctx, EcsLocalNetworkName, types.NetworkInspectOptions{})
 	if err != nil {
 		if client.IsErrNotFound(err) {
 			return false
@@ -115,12 +108,11 @@ func localNetworkExists(docker idempotentNetworkCreator) bool {
 	return true
 }
 
-// createLocalNetwork creates the local network.
-func createLocalNetwork(docker idempotentNetworkCreator) {
+func createLocalNetwork(dockerClient networkCreator) {
 	ctx, cancel := context.WithTimeout(context.Background(), dockerTimeout)
 	defer cancel()
 
-	resp, err := docker.NetworkCreate(ctx, EcsLocalNetworkName, types.NetworkCreate{
+	resp, err := dockerClient.NetworkCreate(ctx, EcsLocalNetworkName, types.NetworkCreate{
 		IPAM: &network.IPAM{
 			Config: []network.IPAMConfig{
 				{
@@ -135,21 +127,20 @@ func createLocalNetwork(docker idempotentNetworkCreator) {
 	logrus.Infof("Created network %s with ID %s", EcsLocalNetworkName, resp.ID)
 }
 
-// setupLocalEndpointsContainer creates the Local Endpoints container if one doesn't exist already.
-func setupLocalEndpointsContainer(docker idempotentContainerStarter) {
+func setupLocalEndpointsContainer(docker containerStarter) {
 	containerID := createLocalEndpointsContainer(docker)
 	startContainer(docker, containerID)
 }
 
 // createLocalEndpointsContainer returns the ID of the newly created container.
 // If the container already exists, returns the ID of the existing container.
-func createLocalEndpointsContainer(docker idempotentContainerStarter) string {
+func createLocalEndpointsContainer(dockerClient containerStarter) string {
 	ctx, cancel := context.WithTimeout(context.Background(), dockerTimeout)
 	defer cancel()
 
 	// See First Scenario in https://aws.amazon.com/blogs/compute/a-guide-to-locally-testing-containers-with-amazon-ecs-local-endpoints-and-docker-compose/
 	// for an explanation of these fields.
-	resp, err := docker.ContainerCreate(ctx,
+	resp, err := dockerClient.ContainerCreate(ctx,
 		&container.Config{
 			Image: localEndpointsImageName,
 			Env: []string{
@@ -178,7 +169,7 @@ func createLocalEndpointsContainer(docker idempotentContainerStarter) string {
 	if err != nil {
 		if strings.Contains(err.Error(), "Conflict") {
 			// We already created this container before since there is a name conflict, fetch its ID and return it.
-			containerID := localEndpointsContainerID(docker)
+			containerID := localEndpointsContainerID(dockerClient)
 			logrus.Infof("The %s container already exists with ID %s", localEndpointsContainerName, containerID)
 			return containerID
 		}
@@ -189,12 +180,11 @@ func createLocalEndpointsContainer(docker idempotentContainerStarter) string {
 	return resp.ID
 }
 
-// localEndpointsContainerID returns the ID of the Local Endpoints container.
-func localEndpointsContainerID(docker idempotentContainerStarter) string {
+func localEndpointsContainerID(dockerClient containerStarter) string {
 	ctx, cancel := context.WithTimeout(context.Background(), dockerTimeout)
 	defer cancel()
 
-	resp, err := docker.ContainerList(ctx, types.ContainerListOptions{
+	resp, err := dockerClient.ContainerList(ctx, types.ContainerListOptions{
 		All: true,
 		Filters: filters.NewArgs(
 			filters.Arg("name", localEndpointsContainerName),
@@ -203,16 +193,18 @@ func localEndpointsContainerID(docker idempotentContainerStarter) string {
 	if err != nil {
 		logrus.Fatalf("Failed to list containers with name %s due to %v", localEndpointsContainerName, err)
 	}
+	if len(resp) != 1 {
+		logrus.Fatalf("Expected to find one container named %s but found %d", localEndpointsContainerName, len(resp))
+	}
 	return resp[0].ID
 }
 
-// startContainer starts the container with the containerID.
-// If the container is already running, Docker does not return an error response.
-func startContainer(docker idempotentContainerStarter, containerID string) {
+func startContainer(dockerClient containerStarter, containerID string) {
 	ctx, cancel := context.WithTimeout(context.Background(), dockerTimeout)
 	defer cancel()
 
-	if err := docker.ContainerStart(ctx, containerID, types.ContainerStartOptions{}); err != nil {
+	// If the container is already running, Docker does not return an error response.
+	if err := dockerClient.ContainerStart(ctx, containerID, types.ContainerStartOptions{}); err != nil {
 		logrus.Fatalf("Failed to start container with ID %s due to %v", containerID, err)
 	}
 	logrus.Infof("Started container with ID %s", containerID)

ecs-cli/modules/cli/local/network/network.go

@@ -13,7 +13,7 @@ import (
 	"github.com/aws/amazon-ecs-cli/ecs-cli/modules/cli/local/network/mock_network"
 )
 
-type CallsConfigurer func(docker *mock_network.MockIdempotentLocalEndpointsStarter) *mock_network.MockIdempotentLocalEndpointsStarter
+type CallsConfigurer func(docker *mock_network.MockLocalEndpointsStarter) *mock_network.MockLocalEndpointsStarter
 
 type notFoundErr struct{}
 
@@ -33,7 +33,7 @@ func TestSetup(t *testing.T) {
 		configureCalls CallsConfigurer
 	}{
 		"new network and new container": {
-			configureCalls: func(docker *mock_network.MockIdempotentLocalEndpointsStarter) *mock_network.MockIdempotentLocalEndpointsStarter {
+			configureCalls: func(docker *mock_network.MockLocalEndpointsStarter) *mock_network.MockLocalEndpointsStarter {
 				containerID := "1234"
 				gomock.InOrder(
 					// We expect to create the network if it doesn't exist already.
@@ -53,7 +53,7 @@ func TestSetup(t *testing.T) {
 			},
 		},
 		"existing network and existing container": {
-			configureCalls: func(docker *mock_network.MockIdempotentLocalEndpointsStarter) *mock_network.MockIdempotentLocalEndpointsStarter {
+			configureCalls: func(docker *mock_network.MockLocalEndpointsStarter) *mock_network.MockLocalEndpointsStarter {
 				networkID := "abcd"
 				containerID := "1234"
 				gomock.InOrder(
@@ -87,9 +87,9 @@ func TestSetup(t *testing.T) {
 	}
 }
 
-func newMockLocalNetworkStarter(t *testing.T) *mock_network.MockIdempotentLocalEndpointsStarter {
+func newMockLocalNetworkStarter(t *testing.T) *mock_network.MockLocalEndpointsStarter {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
-	return mock_network.NewMockIdempotentLocalEndpointsStarter(ctrl)
+	return mock_network.NewMockLocalEndpointsStarter(ctrl)
 }