Merge pull request #2546 from thaJeztah/resolve_image_cleanup

Some minor refactoring and reformatting

cli/command/stack/swarm/deploy.go

@@ -25,25 +25,26 @@ const (
 func RunDeploy(dockerCli command.Cli, opts options.Deploy, cfg *composetypes.Config) error {
 	ctx := context.Background()
 
-	if err := validateResolveImageFlag(dockerCli, &opts); err != nil {
+	if err := validateResolveImageFlag(&opts); err != nil {
 		return err
 	}
-
-	return deployCompose(ctx, dockerCli, opts, cfg)
-}
-
-// validateResolveImageFlag validates the opts.resolveImage command line option
-// and also turns image resolution off if the version is older than 1.30
-func validateResolveImageFlag(dockerCli command.Cli, opts *options.Deploy) error {
-	if opts.ResolveImage != ResolveImageAlways && opts.ResolveImage != ResolveImageChanged && opts.ResolveImage != ResolveImageNever {
-		return errors.Errorf("Invalid option %s for flag --resolve-image", opts.ResolveImage)
-	}
 	// client side image resolution should not be done when the supported
 	// server version is older than 1.30
 	if versions.LessThan(dockerCli.Client().ClientVersion(), "1.30") {
 		opts.ResolveImage = ResolveImageNever
 	}
+
+	return deployCompose(ctx, dockerCli, opts, cfg)
+}
+
+// validateResolveImageFlag validates the opts.resolveImage command line option
+func validateResolveImageFlag(opts *options.Deploy) error {
+	switch opts.ResolveImage {
+	case ResolveImageAlways, ResolveImageChanged, ResolveImageNever:
 		return nil
+	default:
+		return errors.Errorf("Invalid option %s for flag --resolve-image", opts.ResolveImage)
+	}
 }
 
 // checkDaemonIsSwarmManager does an Info API call to verify that the daemon is

cli/command/stack/swarm/deploy_composefile.go

@@ -77,11 +77,7 @@ func getServicesDeclaredNetworks(serviceConfigs []composetypes.ServiceConfig) ma
 	return serviceNetworks
 }
 
-func validateExternalNetworks(
+func validateExternalNetworks(ctx context.Context, client dockerclient.NetworkAPIClient, externalNetworks []string) error {
-	ctx context.Context,
-	client dockerclient.NetworkAPIClient,
-	externalNetworks []string,
-) error {
 	for _, networkName := range externalNetworks {
 		if !container.NetworkMode(networkName).IsUserDefined() {
 			// Networks that are not user defined always exist on all nodes as
@@ -101,11 +97,7 @@ func validateExternalNetworks(
 	return nil
 }
 
-func createSecrets(
+func createSecrets(ctx context.Context, dockerCli command.Cli, secrets []swarm.SecretSpec) error {
-	ctx context.Context,
-	dockerCli command.Cli,
-	secrets []swarm.SecretSpec,
-) error {
 	client := dockerCli.Client()
 
 	for _, secretSpec := range secrets {
@@ -129,11 +121,7 @@ func createSecrets(
 	return nil
 }
 
-func createConfigs(
+func createConfigs(ctx context.Context, dockerCli command.Cli, configs []swarm.ConfigSpec) error {
-	ctx context.Context,
-	dockerCli command.Cli,
-	configs []swarm.ConfigSpec,
-) error {
 	client := dockerCli.Client()
 
 	for _, configSpec := range configs {
@@ -157,12 +145,7 @@ func createConfigs(
 	return nil
 }
 
-func createNetworks(
+func createNetworks(ctx context.Context, dockerCli command.Cli, namespace convert.Namespace, networks map[string]types.NetworkCreate) error {
-	ctx context.Context,
-	dockerCli command.Cli,
-	namespace convert.Namespace,
-	networks map[string]types.NetworkCreate,
-) error {
 	client := dockerCli.Client()
 
 	existingNetworks, err := getStackNetworks(ctx, client, namespace.Name())
@@ -192,14 +175,8 @@ func createNetworks(
 	return nil
 }
 
-func deployServices(
+// nolint: gocyclo
-	ctx context.Context,
+func deployServices(ctx context.Context, dockerCli command.Cli, services map[string]swarm.ServiceSpec, namespace convert.Namespace, sendAuth bool, resolveImage string) error {
-	dockerCli command.Cli,
-	services map[string]swarm.ServiceSpec,
-	namespace convert.Namespace,
-	sendAuth bool,
-	resolveImage string,
-) error {
 	apiClient := dockerCli.Client()
 	out := dockerCli.Out()
 
@@ -214,10 +191,12 @@ func deployServices(
 	}
 
 	for internalName, serviceSpec := range services {
-		name := namespace.Scope(internalName)
+		var (
+			name        = namespace.Scope(internalName)
+			image       = serviceSpec.TaskTemplate.ContainerSpec.Image
+			encodedAuth string
+		)
 
-		encodedAuth := ""
-		image := serviceSpec.TaskTemplate.ContainerSpec.Image
 		if sendAuth {
 			// Retrieve encoded auth token from the image reference
 			encodedAuth, err = command.RetrieveAuthTokenFromImage(ctx, dockerCli, image)
@@ -231,30 +210,37 @@ func deployServices(
 
 			updateOpts := types.ServiceUpdateOptions{EncodedRegistryAuth: encodedAuth}
 
-			switch {
+			switch resolveImage {
-			case resolveImage == ResolveImageAlways || (resolveImage == ResolveImageChanged && image != service.Spec.Labels[convert.LabelImage]):
+			case ResolveImageAlways:
 				// image should be updated by the server using QueryRegistry
 				updateOpts.QueryRegistry = true
-			case image == service.Spec.Labels[convert.LabelImage]:
+			case ResolveImageChanged:
+				if image != service.Spec.Labels[convert.LabelImage] {
+					// Query the registry to resolve digest for the updated image
+					updateOpts.QueryRegistry = true
+				} else {
 					// image has not changed; update the serviceSpec with the
 					// existing information that was set by QueryRegistry on the
 					// previous deploy. Otherwise this will trigger an incorrect
 					// service update.
 					serviceSpec.TaskTemplate.ContainerSpec.Image = service.Spec.TaskTemplate.ContainerSpec.Image
 				}
+			default:
+				if image == service.Spec.Labels[convert.LabelImage] {
+					// image has not changed; update the serviceSpec with the
+					// existing information that was set by QueryRegistry on the
+					// previous deploy. Otherwise this will trigger an incorrect
+					// service update.
+					serviceSpec.TaskTemplate.ContainerSpec.Image = service.Spec.TaskTemplate.ContainerSpec.Image
+				}
+			}
 
-			// Stack deploy does not have a `--force` option. Preserve existing ForceUpdate
+			// Stack deploy does not have a `--force` option. Preserve existing
-			// value so that tasks are not re-deployed if not updated.
+			// ForceUpdate value so that tasks are not re-deployed if not updated.
 			// TODO move this to API client?
 			serviceSpec.TaskTemplate.ForceUpdate = service.Spec.TaskTemplate.ForceUpdate
 
-			response, err := apiClient.ServiceUpdate(
+			response, err := apiClient.ServiceUpdate(ctx, service.ID, service.Version, serviceSpec, updateOpts)
-				ctx,
-				service.ID,
-				service.Version,
-				serviceSpec,
-				updateOpts,
-			)
 			if err != nil {
 				return errors.Wrapf(err, "failed to update service %s", name)
 			}

cli/command/stack/swarm/deploy_test.go

@@ -87,24 +87,26 @@ func TestServiceUpdateResolveImageChanged(t *testing.T) {
 
 	ctx := context.Background()
 
-	for _, testcase := range testcases {
+	for _, tc := range testcases {
-		t.Logf("Testing image %q", testcase.image)
+		tc := tc
+		t.Run(tc.image, func(t *testing.T) {
 			spec := map[string]swarm.ServiceSpec{
 				"myservice": {
 					TaskTemplate: swarm.TaskSpec{
 						ContainerSpec: &swarm.ContainerSpec{
-						Image: testcase.image,
+							Image: tc.image,
 						},
 					},
 				},
 			}
 			err := deployServices(ctx, client, spec, namespace, false, ResolveImageChanged)
 			assert.NilError(t, err)
-		assert.Check(t, is.Equal(receivedOptions.QueryRegistry, testcase.expectedQueryRegistry))
+			assert.Check(t, is.Equal(receivedOptions.QueryRegistry, tc.expectedQueryRegistry))
-		assert.Check(t, is.Equal(receivedService.TaskTemplate.ContainerSpec.Image, testcase.expectedImage))
+			assert.Check(t, is.Equal(receivedService.TaskTemplate.ContainerSpec.Image, tc.expectedImage))
-		assert.Check(t, is.Equal(receivedService.TaskTemplate.ForceUpdate, testcase.expectedForceUpdate))
+			assert.Check(t, is.Equal(receivedService.TaskTemplate.ForceUpdate, tc.expectedForceUpdate))
 
 			receivedService = swarm.ServiceSpec{}
 			receivedOptions = types.ServiceUpdateOptions{}
+		})
 	}
 }