cmd/snap: snap refresh --time with new and legacy schedules #4487

bboozzoo · 2018-01-15T11:27:46Z

Add --timer command line switch to snap refresh command. The switch replaces
--time (now hidden). Passing --time switch, the legacy format of the output is
preserved. When using --timer instead, the output clearly indicates that what is
returned is the new time schedule.

Add --timer command line switch to `snap refresh` command. The switch replaces --time (now hidden). Passing --time switch, the legacy format of the output is preserved. When using --timer instead, the output clearly indicates that what is returned is the new time schedule. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

codecov-io · 2018-01-15T12:06:09Z

Codecov Report

Merging #4487 into master will decrease coverage by 0.1%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##           master    #4487      +/-   ##
==========================================
- Coverage   78.28%   78.18%   -0.11%     
==========================================
  Files         453      455       +2     
  Lines       32334    32466     +132     
==========================================
+ Hits        25312    25382      +70     
- Misses       4923     4970      +47     
- Partials     2099     2114      +15

Impacted Files	Coverage Δ
client/client.go	`80.78% <ø> (ø)`	⬆️
overlord/snapstate/snapmgr.go	`87.72% <100%> (ø)`	⬆️
daemon/api.go	`74.65% <100%> (+0.04%)`	⬆️
overlord/snapstate/autorefresh.go	`78.86% <83.33%> (ø)`	⬆️
cmd/snap/cmd_snap_op.go	`67.63% <85.71%> (+0.33%)`	⬆️
interfaces/dbus/backend.go	`57.97% <0%> (-4.53%)`	⬇️
overlord/hookstate/hookmgr.go	`72% <0%> (-1.15%)`	⬇️
userd/launcher.go	`68.42% <0%> (ø)`	⬆️
interfaces/builtin/desktop.go	`87.5% <0%> (ø)`	⬆️
cmd/snap/cmd_run.go	`69.04% <0%> (ø)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b69fca...8d6b922. Read the comment docs.

mvo5 · 2018-01-16T10:08:35Z

A bit of a meta-question: shouldn't client.SysInfo() have the data if its the new-style of legacy schedule? Or is this purely about the prefix string (schedule or timer)? I guess I wonder if it makes sense to have something like: schedule: new-style-timer (which will happen with this PR afaict).

stolowski

LGTM, but please see the little nitpick, +1

stolowski · 2018-01-16T16:57:37Z

cmd/snap/cmd_snap_op.go

 		if x.asksForMode() || x.asksForChannel() {
-			return errors.New(i18n.G("--time does not take mode nor channel flags"))
+			err := errors.New(i18n.G("--timer does not take mode nor channel flags"))


Nitpick: no need for err variable, just:

if x.Time return errors.New(...) } return errors.New(...) // --timer error '''

Fixed, thanks

zyga

LGTM

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

niemeyer · 2018-01-23T12:59:49Z

cmd/snap/cmd_snap_op.go

@@ -599,7 +599,8 @@ type cmdRefresh struct {

 	Revision         string `long:"revision"`
 	List             bool   `long:"list"`
-	Time             bool   `long:"time"`
+	Time             bool   `long:"time" hidden:"yes"`
+	Timer            bool   `long:"timer"`


Looking at the PR, there's a gut feeling that this is error prone. The difference between "time" and "timer" is too subtle, and people will end up using one or the other without realizing, and getting weird behavior. I suggest just introducing the new behavior, and only displaying schedule if one is indeed set and no timer is set.

There's a very small chance that this will break someone that was trying to get the old schedule even when nothing was set, but this risk is one-off and better than the risk of having error prone behavior forever.

…efresh-timer-wip

…y format Let the callers know whether the refresh schedule was defined using a legacy format. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ules Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Adjust refresh info returned in sysinfo endpoint depending on the use of the modern 'refresh.timer' or the legacy 'refresh.schedule' settings. When the new refresh.timer setting is used only the 'timer' field will be sent. Similarly, when the legacy setting is in effect, the 'timer' field will not be sent and 'schedule' will be send instead. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…h info Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…efresh-timer-wip

niemeyer · 2018-01-24T15:22:29Z

cmd/snap/cmd_snap_op.go

+		timerOrSchedulePrefix = "schedule"
+		timerOrSchedule = sysinfo.Refresh.Schedule
+	}
+	fmt.Fprintf(Stdout, "%s: %s\n", timerOrSchedulePrefix, timerOrSchedule)


That's quite verbose for what's essentially this:

if sysinfo.Refresh.Schedule != "" { fmt.Fprintf(Stdout, "schedule: %s", sysinfo.Refresh.Schedule } else { fmt.Fprintf(Stdout, "timer: %s", sysinfo.Refresh.Timer) }

Note that the server should never send a schedule if timer is set, or the reverse.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga

Tests need adjustments:

+ '[' true = false ']'
+ snap install test-snapd-control-consumer
test-snapd-control-consumer 1.0 from 'testrootorg' installed
Snap test-snapd-control-consumer is no longer tracking stable.
+ snap interfaces
Slot                      Plug
[snip]
:snapd-control            test-snapd-control-consumer,test-snapd-control-consumer:snapd-control-with-manage
+ echo 'When the snapd-control-with-manage plug is connected'
When the snapd-control-with-manage plug is connected
+ snap connect test-snapd-control-consumer:snapd-control-with-manage
+ echo 'Then the system knows it can be set to managed'
Then the system knows it can be set to managed
+ snap debug can-manage-refreshes
+ MATCH true
+ echo 'Then the core refresh.schedule can be set to '\''managed'\'''
Then the core refresh.schedule can be set to 'managed'
+ snap set core refresh.schedule=managed
+ journalctl -u snapd
+ grep 'cannot parse "managed"'
+ snap refresh --time
+ MATCH 'schedule: managed'
error: pattern not found, got:
timer: managed
last: 2018-01-24T15:50:00Z
next: 2018-01-25T19:22:00Z

…figuration setting Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mvo5

Thanks for this update! Looks good, I put some nitpicks inside but really no blockers, feel free to ignore.

mvo5 · 2018-01-25T10:13:05Z

client/client.go

-	Schedule string `json:"schedule"`
+	// Schedule contains the legacy refresh.schedule setting. The field is
+	// omitted when the legacy setting is not used.
+	Schedule string `json:"schedule,omitempty"`


(nitpick^10) I would put schedule and timer close to each other, probably move schedule at the end (under timer).

mvo5 · 2018-01-25T10:14:44Z

cmd/snap/cmd_snap_op.go

@@ -665,7 +665,11 @@ func (x *cmdRefresh) showRefreshTimes() error {
 		return err
 	}

-	fmt.Fprintf(Stdout, "schedule: %s\n", sysinfo.Refresh.Schedule)
+	if sysinfo.Refresh.Schedule != "" {


(nitpick^2) I wonder if this should be the other way around (even though it does not make a practical difference I suppose). I.e. to make clear that sysinfo.Refresh.Timer is the thing that always wins if it is set?

We should also probably show an (internal) error if no refresh schedule is set at all (and add a test for this).

mvo5 · 2018-01-25T10:19:03Z

overlord/snapstate/autorefresh.go

+// RefreshSchedule will return a user visible string with the current schedule
+// for the automatic refreshes and a flag indicating whether the schedule is a
+// legacy one.
+func (m *autoRefresh) RefreshSchedule() (string, bool, error) {


I would name the return values now to make clear what the "bool" stands for, something like func ... (schedule string, legacy bool, err error)

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…schedule are empty Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…efresh-timer-wip

niemeyer

LGTM with just trivials, thanks!

niemeyer · 2018-01-26T11:31:12Z

cmd/snap/cmd_snap_op.go

+	} else if sysinfo.Refresh.Schedule != "" {
+		fmt.Fprintf(Stdout, "schedule: %s\n", sysinfo.Refresh.Schedule)
+	} else {
+		return errors.New("internal error, both refresh.timer and refresh.schedule are empty")


Either this is incorrect, or the documentation on the fields above is incorrect, since they disagree. The documentation for the fields says that for both fields, the value is omitted if not in use. That seems to imply that it is possible for them both to be empty if the user hasn't set either.

As an aside, in those cases I think we use a colon after internal error ("internal error: ...").

niemeyer · 2018-01-26T11:34:26Z

daemon/api.go

+		Next: formatRefreshTime(nextRefresh),
+	}
+	if !legacySchedule {
+		refreshInfo.Timer = refreshScheduleStr


This looks good. Seems to be the field documentation that is wrong. I suggest just dropping the "The field is omitted" part of both comments. The desired intention there seems to be implied by omitempty already.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2018-01-29T09:03:19Z

Is this good to be merged?

zyga

LGTM with one word change, feel free to push in another branch if you want.

zyga · 2018-01-29T09:16:06Z

overlord/snapstate/snapmgr.go

@@ -366,10 +366,10 @@ func (m *SnapManager) LastRefresh() (time.Time, error) {
 	return m.autoRefresh.LastRefresh()
 }

-// RefreshSchedule returns the current refresh schedule as a string
-// suitable to display to a user.
+// RefreshSchedule returns the current refresh schedule as a string suitable to


Nitpick: suitable for

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2018-01-29T15:18:18Z

merge?

bboozzoo requested a review from niemeyer January 15, 2018 11:27

stolowski approved these changes Jan 16, 2018

View reviewed changes

zyga approved these changes Jan 17, 2018

View reviewed changes

cmd/snap: simplify --time[r] & channel/mode error handling

df3c49a

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

niemeyer requested changes Jan 23, 2018

View reviewed changes

bboozzoo added 6 commits January 24, 2018 13:35

Merge remote-tracking branch 'origin/master' into bboozzoo/cli-snap-r…

a287fad

…efresh-timer-wip

overlord/snapstate: indicate whether the refresh schedule is in legac…

c6640ab

…y format Let the callers know whether the refresh schedule was defined using a legacy format. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

client: update RefreshInfo to carry both the new and the legacy sched…

e15d52e

…ules Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd/snap: drop --timer, adjust the prefix based on contents of refres…

cd5e439

…h info Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/cli-snap-r…

4207b13

…efresh-timer-wip

bboozzoo changed the title ~~cmd/snap: snap refresh --timer, hide --time~~ cmd/snap: snap refresh --time with new and legacy schedules Jan 24, 2018

niemeyer requested changes Jan 24, 2018

View reviewed changes

bboozzoo added 2 commits January 24, 2018 16:27

cmd/snap: simplify timer/schedule check

37f9fdb

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

daemon: extend api tests to cover new and legacy schedules both set

48f33d5

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga requested changes Jan 25, 2018

View reviewed changes

overlord/snapstate: 'managed' is set on a legacy refresh.schedule con…

6944163

…figuration setting Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mvo5 reviewed Jan 25, 2018

View reviewed changes

bboozzoo added 4 commits January 25, 2018 12:21

overlord/snapstate: use named return args in RefreshSchedule()

a80b9f2

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

client: reshuffle Timer and Schedule fields in RefreshInfo

79c5f26

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd/snap: raise internal error in refresh --time if both timer and …

174ddef

…schedule are empty Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Merge remote-tracking branch 'origin/master' into bboozzoo/cli-snap-r…

1013951

…efresh-timer-wip

niemeyer approved these changes Jan 26, 2018

View reviewed changes

bboozzoo added 2 commits January 26, 2018 12:49

client: refine comments on RefreshInfo fields

b33c38a

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd/snap: fine tune internal error messages

c1fc08c

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mvo5 approved these changes Jan 29, 2018

View reviewed changes

zyga approved these changes Jan 29, 2018

View reviewed changes

overlord/snapstate: fix typo in the comments

8d6b922

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga merged commit 2dd7827 into snapcore:master Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/snap: snap refresh --time with new and legacy schedules #4487

cmd/snap: snap refresh --time with new and legacy schedules #4487

bboozzoo commented Jan 15, 2018

codecov-io commented Jan 15, 2018 •

edited

mvo5 commented Jan 16, 2018

stolowski left a comment

stolowski Jan 16, 2018

bboozzoo Jan 22, 2018

zyga left a comment

niemeyer Jan 23, 2018

niemeyer Jan 24, 2018

zyga left a comment

mvo5 left a comment

mvo5 Jan 25, 2018

mvo5 Jan 25, 2018

mvo5 Jan 25, 2018

niemeyer left a comment

niemeyer Jan 26, 2018

niemeyer Jan 26, 2018

bboozzoo commented Jan 29, 2018

zyga left a comment

zyga Jan 29, 2018

bboozzoo commented Jan 29, 2018

cmd/snap: snap refresh --time with new and legacy schedules #4487

cmd/snap: snap refresh --time with new and legacy schedules #4487

Conversation

bboozzoo commented Jan 15, 2018

codecov-io commented Jan 15, 2018 • edited

Codecov Report

mvo5 commented Jan 16, 2018

stolowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Jan 29, 2018

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Jan 29, 2018

codecov-io commented Jan 15, 2018 •

edited