snap: do exit 0 on install/remove if that snap is already installed or already removed

client/client.go

@@ -320,6 +320,10 @@ const (
 	ErrorKindTermsNotAccepted  = "terms-not-accepted"
 	ErrorKindNoPaymentMethods  = "no-payment-methods"
 	ErrorKindPaymentDeclined   = "payment-declined"
+
+	ErrorKindSnapAlreadyInstalled = "snap-already-installed"
+	ErrorKindSnapNotInstalled     = "snap-not-installed"
+	ErrorKindNoUpdateAvailable    = "snap-no-update-available"
 )
 
 // IsTwoFactorError returns whether the given error is due to problems

cmd/snap/cmd_snap_op.go

@@ -170,6 +170,10 @@ func (x *cmdRemove) removeOne(opts *client.SnapOptions) error {
 
 	cli := Client()
 	changeID, err := cli.Remove(name, opts)
+	if e, ok := err.(*client.Error); ok && e.Kind == client.ErrorKindSnapNotInstalled {
+		fmt.Fprintf(Stderr, e.Message+"\n")
+		return nil
+	}
 	if err != nil {
 		return err
 	}
@@ -201,8 +205,17 @@ func (x *cmdRemove) removeMany(opts *client.SnapOptions) error {
 		return err
 	}
 
+	seen := make(map[string]bool)
 	for _, name := range removed {
 		fmt.Fprintf(Stdout, i18n.G("%s removed\n"), name)
+		seen[name] = true
+	}
+	for _, name := range names {
+		if !seen[name] {
+			// FIXME: this is the only reason why a name can be
+			// skipped, but it does feel awkward
+			fmt.Fprintf(Stdout, i18n.G("%s not installed\n"), name)
+		}
 	}
 
 	return nil
@@ -361,6 +374,10 @@ func (x *cmdInstall) installOne(name string, opts *client.SnapOptions) error {
 	} else {
 		changeID, err = cli.Install(name, opts)
 	}
+	if e, ok := err.(*client.Error); ok && e.Kind == client.ErrorKindSnapAlreadyInstalled {
+		fmt.Fprintf(Stderr, e.Message+"\n")
+		return nil
+	}
 	if err != nil {
 		return err
 	}
@@ -408,7 +425,22 @@ func (x *cmdInstall) installMany(names []string, opts *client.SnapOptions) error
 	}
 
 	if len(installed) > 0 {
-		return showDone(installed, "install")
+		if err := showDone(installed, "install"); err != nil {
+			return err
+		}
+	}
+
+	// show skipped
+	seen := make(map[string]bool)
+	for _, name := range installed {
+		seen[name] = true
+	}
+	for _, name := range names {
+		if !seen[name] {
+			// FIXME: this is the only reason why a name can be
+			// skipped, but it does feel awkward
+			fmt.Fprintf(Stdout, i18n.G("%s already installed\n"), name)
+		}
 	}
 
 	return nil
@@ -483,6 +515,10 @@ func refreshMany(snaps []string, opts *client.SnapOptions) error {
 func refreshOne(name string, opts *client.SnapOptions) error {
 	cli := Client()
 	changeID, err := cli.Refresh(name, opts)
+	if e, ok := err.(*client.Error); ok && e.Kind == client.ErrorKindNoUpdateAvailable {
+		fmt.Fprintf(Stderr, e.Message+"\n")
+		return nil
+	}
 	if err != nil {
 		return err
 	}

daemon/api.go

@@ -971,6 +971,40 @@ func (inst *snapInstruction) dispatch() snapActionFunc {
 	return snapInstructionDispTable[inst.Action]
 }
 
+func (inst *snapInstruction) errToResponse(err error) Response {
+	if _, ok := err.(*snap.AlreadyInstalledError); ok {
+		return SyncResponse(&resp{
+			Type: ResponseTypeError,
+			Result: &errorResult{
+				Message: err.Error(),
+				Kind:    errorKindSnapAlreadyInstalled,
+			},
+			Status: http.StatusBadRequest,
+		}, nil)
+	}
+	if _, ok := err.(*snap.NotInstalledError); ok {
+		return SyncResponse(&resp{
+			Type: ResponseTypeError,
+			Result: &errorResult{
+				Message: err.Error(),
+				Kind:    errorKindSnapNotInstalled,
+			},
+			Status: http.StatusBadRequest,
+		}, nil)
+	}
+	if _, ok := err.(*snap.NoUpdateAvailableError); ok {
+		return SyncResponse(&resp{
+			Type: ResponseTypeError,
+			Result: &errorResult{
+				Message: err.Error(),
+				Kind:    errorKindSnapNoUpdateAvailable,
+			},
+			Status: http.StatusBadRequest,
+		}, nil)
+	}
+	return BadRequest("cannot %s %q: %v", inst.Action, inst.Snaps[0], err)
+}
+
 func postSnap(c *Command, r *http.Request, user *auth.UserState) Response {
 	route := c.d.router.Get(stateChangeCmd.Path)
 	if route == nil {
@@ -1001,7 +1035,7 @@ func postSnap(c *Command, r *http.Request, user *auth.UserState) Response {
 
 	msg, tsets, err := impl(&inst, state)
 	if err != nil {
-		return BadRequest("cannot %s %q: %v", inst.Action, inst.Snaps[0], err)
+		return inst.errToResponse(err)
 	}
 
 	chg := newChange(state, inst.Action+"-snap", msg, tsets, inst.Snaps)

daemon/response.go

@@ -127,6 +127,10 @@ const (
 	errorKindTermsNotAccepted  = errorKind("terms-not-accepted")
 	errorKindNoPaymentMethods  = errorKind("no-payment-methods")
 	errorKindPaymentDeclined   = errorKind("payment-declined")
+
+	errorKindSnapAlreadyInstalled  = errorKind("snap-already-installed")
+	errorKindSnapNotInstalled      = errorKind("snap-not-installed")
+	errorKindSnapNoUpdateAvailable = errorKind("snap-no-update-available")
 )
 
 type errorValue interface{}

overlord/snapstate/snapmgr.go

@@ -256,7 +256,7 @@ func updateInfo(st *state.State, snapst *SnapState, channel string, userID int,
 		return nil, fmt.Errorf("cannot get refresh information for snap %q: %s", curInfo.Name(), err)
 	}
 	if len(res) == 0 {
-		return nil, fmt.Errorf("snap %q has no updates available", curInfo.Name())
+		return nil, &snap.NoUpdateAvailableError{curInfo.Name()}
 	}
 	return res[0], nil
 }

overlord/snapstate/snapstate.go

@@ -276,7 +276,7 @@ func Install(st *state.State, name, channel string, revision snap.Revision, user
 		return nil, err
 	}
 	if snapst.HasCurrent() {
-		return nil, fmt.Errorf("snap %q already installed", name)
+		return nil, &snap.AlreadyInstalledError{name}
 	}
 
 	snapInfo, err := snapInfo(st, name, channel, revision, userID, flags)
@@ -647,7 +647,7 @@ func Remove(st *state.State, name string, revision snap.Revision) (*state.TaskSe
 	}
 
 	if !snapst.HasCurrent() {
-		return nil, fmt.Errorf("cannot find snap %q", name)
+		return nil, &snap.NotInstalledError{name, snap.R(0)}
 	}
 
 	if err := checkChangeConflict(st, name, nil); err != nil {
@@ -674,7 +674,7 @@ func Remove(st *state.State, name string, revision snap.Revision) (*state.TaskSe
 		}
 
 		if !revisionInSequence(&snapst, revision) {
-			return nil, fmt.Errorf("revision %s of snap %q is not installed", revision, name)
+			return nil, &snap.NotInstalledError{name, revision}
 		}
 	}
 
@@ -943,14 +943,18 @@ func KernelInfo(st *state.State) (*snap.Info, error) {
 // InstallMany installs everything from the given list of names.
 // Note that the state must be locked by the caller.
 func InstallMany(st *state.State, names []string, userID int) ([]string, []*state.TaskSet, error) {
-	installed := make([]string, len(names))
+	installed := make([]string, 0, len(names))
 	tasksets := make([]*state.TaskSet, 0, len(names))
-	for i, name := range names {
+	for _, name := range names {
 		ts, err := Install(st, name, "", snap.R(0), userID, Flags{})
+		// FIXME: is this expected behavior?
+		if _, ok := err.(*snap.AlreadyInstalledError); ok {
+			continue
+		}
 		if err != nil {
 			return nil, nil, err
 		}
-		installed[i] = name
+		installed = append(installed, name)
 		tasksets = append(tasksets, ts)
 	}
 
@@ -960,14 +964,18 @@ func InstallMany(st *state.State, names []string, userID int) ([]string, []*stat
 // RemoveMany removes everything from the given list of names.
 // Note that the state must be locked by the caller.
 func RemoveMany(st *state.State, names []string) ([]string, []*state.TaskSet, error) {
-	removed := make([]string, len(names))
+	removed := make([]string, 0, len(names))
 	tasksets := make([]*state.TaskSet, 0, len(names))
-	for i, name := range names {
+	for _, name := range names {
 		ts, err := Remove(st, name, snap.R(0))
+		// FIXME: is this expected behavior?
+		if _, ok := err.(*snap.NotInstalledError); ok {
+			continue
+		}
 		if err != nil {
 			return nil, nil, err
 		}
-		removed[i] = name
+		removed = append(removed, name)
 		tasksets = append(tasksets, ts)
 	}
 

snap/errors.go

@@ -0,0 +1,50 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2014-2016 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package snap
+
+import "fmt"
+
+type AlreadyInstalledError struct {
+	Snap string
+}
+
+func (e AlreadyInstalledError) Error() string {
+	return fmt.Sprintf("snap %q is already installed", e.Snap)
+}
+
+type NotInstalledError struct {
+	Snap string
+	Rev  Revision
+}
+
+func (e NotInstalledError) Error() string {
+	if e.Rev.Unset() {
+		return fmt.Sprintf("snap %q is not installed", e.Snap)
+	}
+	return fmt.Sprintf("revision %s of snap %q is not installed", e.Rev, e.Snap)
+}
+
+type NoUpdateAvailableError struct {
+	Snap string
+}
+
+func (e NoUpdateAvailableError) Error() string {
+	return fmt.Sprintf("snap %q has no updates available", e.Snap)
+}

tests/main/install-errors/task.yaml

@@ -44,8 +44,7 @@ execute: |
 
     echo "============================================"
 
-    echo "Install a snap already installed fails"
-    if snap install $SNAP_NAME; then
-        echo "Trying to install an already installed snap should fail"
-        exit 1
-    fi
+    echo "Install a snap that is already installed shows a message"
+    echo "but does "exit 0" (LP: #1622782)"
+    snap install $SNAP_NAME 2> stderr.out
+    cat stderr.out | MATCH "snap \"$SNAP_NAME\" is already installed"

tests/main/op-remove/task.yaml

@@ -22,3 +22,8 @@ execute: |
 
     echo "Then the two revisions are removed from disk"
     [ $(snap_revisions basic) = "0" ]
+
+    echo "When the snap is removed again, snap exits with status 0"
+    snap remove basic 2> stderr.out
+    cat stderr.out | MATCH 'snap "basic" is not installed'
+

tests/main/refresh/task.yaml

@@ -60,3 +60,7 @@ execute: |
     echo "Then the new version is listed"
     expected="$SNAP_NAME +$SNAP_VERSION_PATTERN"
     snap list | grep -Pzq "$expected"
+
+    echo "When a snap is refreshed and has no update it exit 0"
+    snap refresh $SNAP_NAME 2>stderr.out
+    cat stderr.out | MATCH "snap \"$SNAP_NAME\" has no updates available"

tests/main/remove-errors/task.yaml

@@ -1,13 +1,6 @@
 summary: Check remove command errors
 
 execute: |
-    echo "An error is returned when trying to remove an non installed snap"
-    if snap remove test-snapd-tools; then
-        echo "An error is expected when trying to remove a non installed snap"
-        exit 1
-    fi
-    echo "================================"
-
     echo "Given a core snap is installed"
     . "$TESTSLIB/snaps.sh"
     install_local test-snapd-tools