Merge lp:~jamesodhunt/snappy/list-show-reboot-message into lp:~snappy-dev/snappy/snappy-moved-to-github

[James Hunt]

* cmd/snappy-go/cmd_list.go: showInstalled(): Add required reboot
  message when appropriate.
* partition/bootloader.go: Added GetRootFSLabel() and GetOtherRootFSLabel() to interface.
* partition/partition.go: NextBootIsOther(): Call GetOtherRootFSLabel(),
  not GetRootFSName() to allow test to succeed.

[lp:~jamesodhunt/snappy/list-show-reboot-message]

156. By Michael Vogt on 2015-02-10

Allow installing local snap package via snappy install ./foo.snap

[Michael Vogt (mvo)]

Thanks for this branch.

Some question:

The reboot message is only shown in showInstalledList(). Is this intended, i.e. showInstalledList() will not show it? If that is not intended, its probably better to move the code into a "generateRebootInfo" (or something similar) and then two list functions can do something "if generateRebootInfo() { fmt.Println(generateRebootInfo); }". It might be worthwhile to talk to Sergio if thats something useful for webdm and should be moved into the generic code, but thats for later (not this MP :)

The code is specific for the "core" part. Would it make sense to just remove that check? AFAICT there is no reason to have it other than that we know that its just core that will set this. The new CLI spec unfortunately also does not specify what kind of information should be displayed on a reboot, probably good to clarify that too.

And a needs-fixing:

The partition/* changes are unrelated to the title of this MP and its not clear to me why they are here. I would prefer it in a separate MP with a (short) explanation of what its doing.

review: Needs Fixing

[lp:~jamesodhunt/snappy/list-show-reboot-message]

157. By James Hunt on 2015-02-10

* cmd/snappy-go/cmd_list.go: showInstalled(): Add required reboot
  message when appropriate.

[James Hunt (jamesodhunt)]

> Some question:
>
> The reboot message is only shown in showInstalledList(). Is this
> intended, i.e. showInstalledList() will not show it?
"Yes", because as you've alluded to, the spec is not fully prescriptive
yet. My understanding is that the go port should align where possible to
the original python implementation unless contradicted by the spec.
Since the spec doesn't show details of "list -a", we should apply the
same format as the old "snappy versions -a". The spec does show the
output for "snappy list --updates" though and no message is present.
I'll query this, but for now I think the code is conformant.

> If that is not
> intended, its probably better to move the code into a
> "generateRebootInfo" (or something similar) and then two list functions
> can do something "if generateRebootInfo() {
> fmt.Println(generateRebootInfo); }". It might be worthwhile to talk to
> Sergio if thats something useful for webdm and should be moved into the
> generic code, but thats for later (not this MP :)
Let's handle that with a follow-on MP if the spec is updated on this
point.

> The code is specific for the "core" part. Would it make sense to just
> remove that check? AFAICT there is no reason to have it other than that
> we know that its just core that will set this.
Agreed - changed.

> The new CLI spec
> unfortunately also does not specify what kind of information should be
> displayed on a reboot, probably good to clarify that too.
Will do.

>
> And a needs-fixing:
>
> The partition/* changes are unrelated to the title of this MP and its
> not clear to me why they are here. I would prefer it in a separate MP
> with a (short) explanation of what its doing.
They are there to make the reboot message correct :) They fix a bug
where we were comparing a disk name with a disk label (incorrect).

I've removed those changes now and will raise a follow-on MP and make it depend on this one...

[James Hunt (jamesodhunt)]

Code is now in place that makes this branch redundant.

review: Disapprove

Unmerged revisions

157. By James Hunt on 2015-02-10

* cmd/snappy-go/cmd_list.go: showInstalled(): Add required reboot
  message when appropriate.

=== modified file 'cmd/snappy-go/cmd_list.go'
--- cmd/snappy-go/cmd_list.go	2015-01-26 14:13:15 +0000
+++ cmd/snappy-go/cmd_list.go	2015-02-10 09:56:12 +0000
@@ -52,12 +52,37 @@
 	w := tabwriter.NewWriter(o, 5, 3, 1, ' ', 0)
 	defer w.Flush()
 
+	// Initialise to handle systems without a provisioned "other"
+	otherVersion := "0"
+	currentVersion := "0"
+	otherName := ""
+	needsReboot := false
+
 	fmt.Fprintln(w, "Name\tVersion\tSummary\t")
 	for _, part := range installed {
+
+		if part.NeedsReboot() {
+			needsReboot = true
+		}
+		if part.IsActive() {
+			currentVersion = part.Version()
+		} else {
+			otherVersion = part.Version()
+			otherName = part.Name()
+		}
+
 		if showAll || part.IsActive() {
 			fmt.Fprintln(w, fmt.Sprintf("%s\t%s\t%s\t", part.Name(), part.Version(), part.Description()))
 		}
 	}
+
+	if needsReboot {
+		if snappy.VersionCompare(otherVersion, currentVersion) > 0 {
+			fmt.Fprintln(w, fmt.Sprintf("Reboot to use the new %s.", otherName))
+		} else {
+			fmt.Fprintln(w, fmt.Sprintf("Reboot to use %s version %s.", otherName, otherVersion))
+		}
+	}
 }
 
 func showUpdatesList(installed []snappy.Part, updates []snappy.Part, showAll bool, o io.Writer) {