Better app install error reporting

The `--pull` flag is removed, we align with what the cli is doing:
searching locally first, then pulling if not found. The user needs to
explicitly pull an app image if they want to install the latest version.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

Author: rumpl

README.md

@@ -234,9 +234,9 @@ Removing network hello_default
 
 ## Installation
 
-**Note**: Docker app is a _command line_ plugin (not be confused with docker _engine_ plugins), extending the `docker` command with `app` sub-commands. 
-It requires [Docker CLI](https://download.docker.com) 19.03.0 or later with experimental features enabled. 
-Either set environment variable `DOCKER_CLI_EXPERIMENTAL=enabled` 
+**Note**: Docker app is a _command line_ plugin (not be confused with docker _engine_ plugins), extending the `docker` command with `app` sub-commands.
+It requires [Docker CLI](https://download.docker.com) 19.03.0 or later with experimental features enabled.
+Either set environment variable `DOCKER_CLI_EXPERIMENTAL=enabled`
 or update your [docker CLI configuration](https://docs.docker.com/engine/reference/commandline/cli/#experimental-features).
 
 **Note**: Docker-app can't be installed using the `docker plugin install` command (yet)
@@ -307,10 +307,7 @@ $ docker app inspect myhubuser/myimage
 
 The first time a command is executed against a given image name the bundle is
 pulled from the registry and put in the local bundle store. You can pre-populate
-this store by running `docker app pull myhubuser/myimage:latest`. All commands
-manipulating a package also accept a `--pull` flag to force pulling the bundle
-from the registry, even if it is present in the local store. This can be useful
-when you are repeatedly pushing a bundle on the same tag.
+this store by running `docker app pull myhubuser/myimage:latest`.
 
 ### Multi-arch applications
 

e2e/pushpull_test.go

@@ -144,7 +144,7 @@ func TestPushInstall(t *testing.T) {
 		cmd.Command = dockerCli.Command("app", "push", "--tag", ref, filepath.Join("testdata", "push-pull", "push-pull.dockerapp"))
 		icmd.RunCmd(cmd).Assert(t, icmd.Success)
 
-		cmd.Command = dockerCli.Command("app", "install", ref, "--pull", "--name", t.Name())
+		cmd.Command = dockerCli.Command("app", "install", ref, "--name", t.Name())
 		icmd.RunCmd(cmd).Assert(t, icmd.Success)
 		cmd.Command = dockerCli.Command("service", "ls")
 		assert.Check(t, cmp.Contains(icmd.RunCmd(cmd).Assert(t, icmd.Success).Combined(), ref))
@@ -164,7 +164,7 @@ func TestPushPullInstall(t *testing.T) {
 		// stop the registry
 		info.stopRegistry()
 
-		// install without --pull should succeed (rely on local store)
+		// install from local store
 		cmd.Command = dockerCli.Command("app", "install", ref+tag, "--name", t.Name())
 		icmd.RunCmd(cmd).Assert(t, icmd.Success)
 		cmd.Command = dockerCli.Command("service", "ls")
@@ -177,9 +177,15 @@ func TestPushPullInstall(t *testing.T) {
 				fmt.Sprintf(`%s\s+push-pull \(1.1.0-beta1\)\s+install\s+success\s+.+second[s]?\sago\s+.+second[s]?\sago\s+%s`, t.Name(), ref+tag),
 			})
 
-		// install with --pull should fail (registry is stopped)
-		cmd.Command = dockerCli.Command("app", "install", "--pull", ref, "--name", t.Name()+"2")
-		assert.Check(t, cmp.Contains(icmd.RunCmd(cmd).Assert(t, icmd.Expected{ExitCode: 1}).Combined(), "failed to resolve bundle manifest"))
+		// install should fail (registry is stopped)
+		cmd.Command = dockerCli.Command("app", "install", "unknown")
+		//nolint: lll
+		expected := `Unable to find application image "unknown:latest" locally
+Unable to find application "unknown": failed to resolve bundle manifest "docker.io/library/unknown:latest": pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed`
+		icmd.RunCmd(cmd).Assert(t, icmd.Expected{
+			ExitCode: 1,
+			Err:      expected,
+		})
 	})
 }
 
@@ -198,7 +204,7 @@ func TestPushInstallBundle(t *testing.T) {
 			cmd.Command = dockerCli.Command("app", "push", "--tag", ref, "a-simple-app:1.0.0")
 			icmd.RunCmd(cmd).Assert(t, icmd.Success)
 
-			cmd.Command = dockerCli.Command("app", "install", ref, "--pull", "--name", name)
+			cmd.Command = dockerCli.Command("app", "install", ref, "--name", name)
 			icmd.RunCmd(cmd).Assert(t, icmd.Success)
 			cmd.Command = dockerCli.Command("service", "ls")
 			assert.Check(t, cmp.Contains(icmd.RunCmd(cmd).Assert(t, icmd.Success).Combined(), ref))
@@ -218,7 +224,7 @@ func TestPushInstallBundle(t *testing.T) {
 			cmd.Command = dockerCli.Command("app", "push", "--tag", ref2, ref+":latest")
 			icmd.RunCmd(cmd).Assert(t, icmd.Success)
 
-			cmd.Command = dockerCli.Command("app", "install", ref2, "--pull", "--name", name)
+			cmd.Command = dockerCli.Command("app", "install", ref2, "--name", name)
 			icmd.RunCmd(cmd).Assert(t, icmd.Success)
 			cmd.Command = dockerCli.Command("service", "ls")
 			assert.Check(t, cmp.Contains(icmd.RunCmd(cmd).Assert(t, icmd.Success).Combined(), ref2))
@@ -245,7 +251,7 @@ func TestPushInstallBundle(t *testing.T) {
 			// remove the bundle from the bundle store to be sure it won't be used instead of registry
 			cleanupIsolatedStore()
 			// install from the registry
-			cmd.Command = dockerCli.Command("app", "install", ref2, "--pull", "--name", name)
+			cmd.Command = dockerCli.Command("app", "install", ref2, "--name", name)
 			icmd.RunCmd(cmd).Assert(t, icmd.Success)
 			cmd.Command = dockerCli.Command("service", "ls")
 			assert.Check(t, cmp.Contains(icmd.RunCmd(cmd).Assert(t, icmd.Success).Combined(), ref))

internal/commands/cnab.go

@@ -17,13 +17,15 @@ import (
 	"github.com/deislabs/cnab-go/driver"
 	dockerDriver "github.com/deislabs/cnab-go/driver/docker"
 	"github.com/docker/app/internal"
+	"github.com/docker/app/internal/log"
 	"github.com/docker/app/internal/packager"
 	appstore "github.com/docker/app/internal/store"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/cli/context/docker"
 	"github.com/docker/cli/cli/context/store"
 	contextstore "github.com/docker/cli/cli/context/store"
+	"github.com/docker/cnab-to-oci/remotes"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
@@ -255,7 +257,7 @@ func loadBundleFromFile(filename string) (*bundle.Bundle, error) {
 //resolveBundle looks for a CNAB bundle which can be in a Docker App Package format or
 // a bundle stored locally or in the bundle store. It returns a built or found bundle,
 // a reference to the bundle if it is found in the bundlestore, and an error.
-func resolveBundle(dockerCli command.Cli, bundleStore appstore.BundleStore, name string, pullRef bool) (*bundle.Bundle, string, error) {
+func resolveBundle(dockerCli command.Cli, bundleStore appstore.BundleStore, name string) (*bundle.Bundle, string, error) {
 	// resolution logic:
 	// - if there is a docker-app package in working directory, or an http:// / https:// prefix, use packager.Extract result
 	// - the name has a .json or .cnab extension and refers to an existing file or web resource: load the bundle
@@ -264,41 +266,58 @@ func resolveBundle(dockerCli command.Cli, bundleStore appstore.BundleStore, name
 	name, kind := getAppNameKind(name)
 	switch kind {
 	case nameKindFile:
-		if pullRef {
-			return nil, "", errors.Errorf("%s: cannot pull when referencing a file based app", name)
-		}
 		if strings.HasSuffix(name, internal.AppExtension) {
 			return extractAndLoadAppBasedBundle(dockerCli, name)
 		}
 		bndl, err := loadBundleFromFile(name)
 		return bndl, "", err
 	case nameKindDir, nameKindEmpty:
-		if pullRef {
-			if kind == nameKindDir {
-				return nil, "", errors.Errorf("%s: cannot pull when referencing a directory based app", name)
-			}
-			return nil, "", errors.Errorf("cannot pull when referencing a directory based app")
-		}
 		return extractAndLoadAppBasedBundle(dockerCli, name)
 	case nameKindReference:
-		bndl, tagRef, err := getLocalBundle(dockerCli, bundleStore, name, pullRef)
+		bndl, tagRef, err := getBundle(dockerCli, bundleStore, name)
+		if err != nil {
+			return nil, "", err
+		}
 		return bndl, tagRef.String(), err
 	}
 	return nil, "", fmt.Errorf("could not resolve bundle %q", name)
 }
 
-func getLocalBundle(dockerCli command.Cli, bundleStore appstore.BundleStore, name string, pullRef bool) (*bundle.Bundle, reference.Named, error) {
+func getBundle(dockerCli command.Cli, bundleStore appstore.BundleStore, name string) (*bundle.Bundle, reference.Named, error) {
 	ref, err := reference.ParseNormalizedNamed(name)
 	if err != nil {
 		return nil, nil, errors.Wrap(err, name)
 	}
 	tagRef := reference.TagNameOnly(ref)
+
+	bndl, err := bundleStore.Read(tagRef)
+	if err != nil {
+		fmt.Fprintf(dockerCli.Err(), "Unable to find application image %q locally\n", reference.FamiliarString(tagRef))
+
+		bndl, err = pullBundle(dockerCli, bundleStore, tagRef)
+		if err != nil {
+			return nil, nil, err
+		}
+		return bndl, tagRef, nil
+	}
+
+	return bndl, tagRef, nil
+}
+
+func pullBundle(dockerCli command.Cli, bundleStore appstore.BundleStore, tagRef reference.Named) (*bundle.Bundle, error) {
 	insecureRegistries, err := insecureRegistriesFromEngine(dockerCli)
 	if err != nil {
-		return nil, tagRef, fmt.Errorf("could not retrieve insecure registries: %v", err)
+		return nil, fmt.Errorf("could not retrieve insecure registries: %v", err)
+	}
+
+	bndl, err := remotes.Pull(log.WithLogContext(context.Background()), reference.TagNameOnly(tagRef), remotes.CreateResolver(dockerCli.ConfigFile(), insecureRegistries...))
+	if err != nil {
+		return nil, err
+	}
+	if err := bundleStore.Store(tagRef, bndl); err != nil {
+		return nil, err
 	}
-	bndl, err := bundleStore.LookupOrPullBundle(tagRef, pullRef, dockerCli.ConfigFile(), insecureRegistries)
-	return bndl, tagRef, err
+	return bndl, nil
 }
 
 func requiredClaimBindMount(c claim.Claim, targetContextName string, dockerCli command.Cli) (bindMount, error) {
@@ -354,8 +373,7 @@ func isDockerHostLocal(host string) bool {
 	return host == "" || strings.HasPrefix(host, "unix://") || strings.HasPrefix(host, "npipe://")
 }
 
-func prepareCustomAction(actionName string, dockerCli command.Cli, appname string, stdout io.Writer,
-	pullOpts pullOptions, paramsOpts parametersOptions) (*action.RunCustom, *appstore.Installation, *bytes.Buffer, error) {
+func prepareCustomAction(actionName string, dockerCli command.Cli, appname string, stdout io.Writer, paramsOpts parametersOptions) (*action.RunCustom, *appstore.Installation, *bytes.Buffer, error) {
 	s, err := appstore.NewApplicationStore(config.Dir())
 	if err != nil {
 		return nil, nil, nil, err
@@ -364,7 +382,7 @@ func prepareCustomAction(actionName string, dockerCli command.Cli, appname strin
 	if err != nil {
 		return nil, nil, nil, err
 	}
-	bundle, ref, err := resolveBundle(dockerCli, bundleStore, appname, pullOpts.pull)
+	bundle, ref, err := resolveBundle(dockerCli, bundleStore, appname)
 	if err != nil {
 		return nil, nil, nil, err
 	}

internal/commands/image/tag_test.go

@@ -7,7 +7,6 @@ import (
 	"gotest.tools/assert"
 
 	"github.com/deislabs/cnab-go/bundle"
-	"github.com/docker/cli/cli/config/configfile"
 	"github.com/docker/distribution/reference"
 )
 
@@ -44,10 +43,6 @@ func (b *bundleStoreStub) Remove(ref reference.Named) error {
 	return nil
 }
 
-func (b *bundleStoreStub) LookupOrPullBundle(ref reference.Named, pullRef bool, config *configfile.ConfigFile, insecureRegistries []string) (*bundle.Bundle, error) {
-	return nil, nil
-}
-
 var mockedBundleStore = &bundleStoreStub{}
 
 func TestInvalidSourceReference(t *testing.T) {

internal/commands/inspect.go

@@ -43,7 +43,7 @@ func runInspect(dockerCli command.Cli, appname string, opts inspectOptions) erro
 	if err != nil {
 		return err
 	}
-	bndl, ref, err := getLocalBundle(dockerCli, bundleStore, appname, false)
+	bndl, ref, err := getBundle(dockerCli, bundleStore, appname)
 
 	if err != nil {
 		return err

internal/commands/install.go

@@ -10,14 +10,14 @@ import (
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/docker/pkg/namesgenerator"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
 
 type installOptions struct {
 	parametersOptions
 	credentialOptions
-	pullOptions
 	orchestrator  string
 	kubeNamespace string
 	stackName     string
@@ -59,7 +59,6 @@ func installCmd(dockerCli command.Cli) *cobra.Command {
 	}
 	opts.parametersOptions.addFlags(cmd.Flags())
 	opts.credentialOptions.addFlags(cmd.Flags())
-	opts.pullOptions.addFlags(cmd.Flags())
 	cmd.Flags().StringVar(&opts.orchestrator, "orchestrator", "", "Orchestrator to install on (swarm, kubernetes)")
 	cmd.Flags().StringVar(&opts.kubeNamespace, "namespace", "default", "Kubernetes namespace to install into")
 	cmd.Flags().StringVar(&opts.stackName, "name", "", "Assign a name to the installation")
@@ -68,7 +67,6 @@ func installCmd(dockerCli command.Cli) *cobra.Command {
 }
 
 func runInstall(dockerCli command.Cli, appname string, opts installOptions) error {
-	defer muteDockerCli(dockerCli)()
 	opts.SetDefaultTargetContext(dockerCli)
 
 	bind, err := requiredBindMount(opts.targetContext, opts.orchestrator, dockerCli.ContextStore())
@@ -80,9 +78,9 @@ func runInstall(dockerCli command.Cli, appname string, opts installOptions) erro
 		return err
 	}
 
-	bndl, ref, err := resolveBundle(dockerCli, bundleStore, appname, opts.pull)
+	bndl, ref, err := resolveBundle(dockerCli, bundleStore, appname)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "Unable to find application %q", appname)
 	}
 	if err := bndl.Validate(); err != nil {
 		return err
@@ -131,7 +129,10 @@ func runInstall(dockerCli command.Cli, appname string, opts installOptions) erro
 	inst := &action.Install{
 		Driver: driverImpl,
 	}
-	err = inst.Run(&installation.Claim, creds, os.Stdout)
+	{
+		defer muteDockerCli(dockerCli)()
+		err = inst.Run(&installation.Claim, creds, os.Stdout)
+	}
 	// Even if the installation failed, the installation is persisted with its failure status,
 	// so any installation needs a clean uninstallation.
 	err2 := installationStore.Store(installation)

internal/commands/pull.go

@@ -8,6 +8,7 @@ import (
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/config"
+	"github.com/docker/distribution/reference"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 )
@@ -34,8 +35,13 @@ func runPull(dockerCli command.Cli, name string) error {
 	if err != nil {
 		return err
 	}
+	ref, err := reference.ParseNormalizedNamed(name)
+	if err != nil {
+		return errors.Wrap(err, name)
+	}
+	tagRef := reference.TagNameOnly(ref)
 
-	bndl, ref, err := getLocalBundle(dockerCli, bundleStore, name, true)
+	bndl, err := pullBundle(dockerCli, bundleStore, tagRef)
 	if err != nil {
 		return errors.Wrap(err, name)
 	}

internal/commands/push.go

@@ -102,7 +102,7 @@ func resolveReferenceAndBundle(dockerCli command.Cli, name string) (*bundle.Bund
 		return nil, "", err
 	}
 
-	bndl, ref, err := resolveBundle(dockerCli, bundleStore, name, false)
+	bndl, ref, err := resolveBundle(dockerCli, bundleStore, name)
 	if err != nil {
 		return nil, "", err
 	}

internal/commands/render.go

@@ -13,8 +13,6 @@ import (
 
 type renderOptions struct {
 	parametersOptions
-	pullOptions
-
 	formatDriver string
 	renderOutput string
 }
@@ -32,7 +30,6 @@ func renderCmd(dockerCli command.Cli) *cobra.Command {
 		},
 	}
 	opts.parametersOptions.addFlags(cmd.Flags())
-	opts.pullOptions.addFlags(cmd.Flags())
 	cmd.Flags().StringVarP(&opts.renderOutput, "output", "o", "-", "Output file")
 	cmd.Flags().StringVar(&opts.formatDriver, "formatter", "yaml", "Configure the output format (yaml|json)")
 
@@ -52,7 +49,7 @@ func runRender(dockerCli command.Cli, appname string, opts renderOptions) error
 		w = f
 	}
 
-	action, installation, errBuf, err := prepareCustomAction(internal.ActionRenderName, dockerCli, appname, w, opts.pullOptions, opts.parametersOptions)
+	action, installation, errBuf, err := prepareCustomAction(internal.ActionRenderName, dockerCli, appname, w, opts.parametersOptions)
 	if err != nil {
 		return err
 	}

internal/commands/root.go

@@ -149,14 +149,6 @@ func (o *credentialOptions) CredentialSetOpts(dockerCli command.Cli, credentialS
 	}
 }
 
-type pullOptions struct {
-	pull bool
-}
-
-func (o *pullOptions) addFlags(flags *pflag.FlagSet) {
-	flags.BoolVar(&o.pull, "pull", false, "Pull the bundle")
-}
-
 // insecureRegistriesFromEngine reads the registry configuration from the daemon and returns
 // a list of all insecure ones.
 func insecureRegistriesFromEngine(dockerCli command.Cli) ([]string, error) {