New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/snap: snap refresh --time with new and legacy schedules #4487
Changes from 10 commits
fb2477d
df3c49a
a287fad
c6640ab
e15d52e
f639f2c
cd5e439
4207b13
37f9fdb
48f33d5
6944163
a80b9f2
79c5f26
174ddef
1013951
b33c38a
c1fc08c
8d6b922
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -665,7 +665,11 @@ func (x *cmdRefresh) showRefreshTimes() error { | |
return err | ||
} | ||
|
||
fmt.Fprintf(Stdout, "schedule: %s\n", sysinfo.Refresh.Schedule) | ||
if sysinfo.Refresh.Schedule != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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 We should also probably show an (internal) error if no refresh schedule is set at all (and add a test for this). |
||
fmt.Fprintf(Stdout, "schedule: %s\n", sysinfo.Refresh.Schedule) | ||
} else { | ||
fmt.Fprintf(Stdout, "timer: %s\n", sysinfo.Refresh.Timer) | ||
} | ||
if sysinfo.Refresh.Last != "" { | ||
fmt.Fprintf(Stdout, "last: %s\n", sysinfo.Refresh.Last) | ||
} else { | ||
|
@@ -717,7 +721,6 @@ func (x *cmdRefresh) Execute([]string) error { | |
if x.asksForMode() || x.asksForChannel() { | ||
return errors.New(i18n.G("--time does not take mode nor channel flags")) | ||
} | ||
|
||
return x.showRefreshTimes() | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -270,7 +270,7 @@ func sysInfo(c *Command, r *http.Request, user *auth.UserState) Response { | |
defer st.Unlock() | ||
nextRefresh := snapMgr.NextRefresh() | ||
lastRefresh, _ := snapMgr.LastRefresh() | ||
refreshScheduleStr, err := snapMgr.RefreshSchedule() | ||
refreshScheduleStr, legacySchedule, err := snapMgr.RefreshSchedule() | ||
if err != nil { | ||
return InternalError("cannot get refresh schedule: %s", err) | ||
} | ||
|
@@ -279,6 +279,16 @@ func sysInfo(c *Command, r *http.Request, user *auth.UserState) Response { | |
return InternalError("cannot get user auth data: %s", err) | ||
} | ||
|
||
refreshInfo := client.RefreshInfo{ | ||
Last: formatRefreshTime(lastRefresh), | ||
Next: formatRefreshTime(nextRefresh), | ||
} | ||
if !legacySchedule { | ||
refreshInfo.Timer = refreshScheduleStr | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} else { | ||
refreshInfo.Schedule = refreshScheduleStr | ||
} | ||
|
||
m := map[string]interface{}{ | ||
"series": release.Series, | ||
"version": c.d.Version, | ||
|
@@ -290,11 +300,7 @@ func sysInfo(c *Command, r *http.Request, user *auth.UserState) Response { | |
"snap-mount-dir": dirs.SnapMountDir, | ||
"snap-bin-dir": dirs.SnapBinariesDir, | ||
}, | ||
"refresh": client.RefreshInfo{ | ||
Schedule: refreshScheduleStr, | ||
Last: formatRefreshTime(lastRefresh), | ||
Next: formatRefreshTime(nextRefresh), | ||
}, | ||
"refresh": refreshInfo, | ||
} | ||
// NOTE: Right now we don't have a good way to differentiate if we | ||
// only have partial confinement (ala AppArmor disabled and Seccomp | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,11 +59,12 @@ func newAutoRefresh(st *state.State) *autoRefresh { | |
} | ||
} | ||
|
||
// RefreshSchedule will return a user visible string with the current | ||
// schedule for the automatic refreshes. | ||
func (m *autoRefresh) RefreshSchedule() (string, error) { | ||
_, scheduleStr, err := m.refreshScheduleWithDefaultsFallback() | ||
return scheduleStr, err | ||
// 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would name the return values now to make clear what the "bool" stands for, something like |
||
_, scheduleStr, legacy, err := m.refreshScheduleWithDefaultsFallback() | ||
return scheduleStr, legacy, err | ||
} | ||
|
||
// NextRefresh returns when the next automatic refresh will happen. | ||
|
@@ -107,7 +108,7 @@ func (m *autoRefresh) Ensure() error { | |
return err | ||
} | ||
|
||
refreshSchedule, refreshScheduleStr, err := m.refreshScheduleWithDefaultsFallback() | ||
refreshSchedule, refreshScheduleStr, _, err := m.refreshScheduleWithDefaultsFallback() | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -163,44 +164,44 @@ func (m *autoRefresh) Ensure() error { | |
// | ||
// TODO: we can remove the refreshSchedule reset because we have validation | ||
// of the schedule now. | ||
func (m *autoRefresh) refreshScheduleWithDefaultsFallback() (ts []*timeutil.Schedule, scheduleAsStr string, err error) { | ||
func (m *autoRefresh) refreshScheduleWithDefaultsFallback() (ts []*timeutil.Schedule, scheduleAsStr string, legacy bool, err error) { | ||
if refreshScheduleManaged(m.state) { | ||
if m.lastRefreshSchedule != "managed" { | ||
logger.Noticef("refresh.schedule is managed via the snapd-control interface") | ||
m.lastRefreshSchedule = "managed" | ||
} | ||
return nil, "managed", nil | ||
return nil, "managed", false, nil | ||
} | ||
|
||
tr := config.NewTransaction(m.state) | ||
|
||
// try the new refresh.timer config option first | ||
err = tr.Get("core", "refresh.timer", &scheduleAsStr) | ||
if err != nil && !config.IsNoOption(err) { | ||
return nil, "", err | ||
return nil, "", false, err | ||
} | ||
if scheduleAsStr != "" { | ||
ts, err = timeutil.ParseSchedule(scheduleAsStr) | ||
if err != nil { | ||
logger.Noticef("cannot use refresh.timer configuration: %s", err) | ||
return refreshScheduleDefault() | ||
} | ||
return ts, scheduleAsStr, nil | ||
return ts, scheduleAsStr, false, nil | ||
} | ||
|
||
// fallback to legacy refresh.schedule setting when the new | ||
// config option is not set | ||
err = tr.Get("core", "refresh.schedule", &scheduleAsStr) | ||
if err != nil && !config.IsNoOption(err) { | ||
return nil, "", err | ||
return nil, "", false, err | ||
} | ||
if scheduleAsStr != "" { | ||
ts, err = timeutil.ParseLegacySchedule(scheduleAsStr) | ||
if err != nil { | ||
logger.Noticef("cannot use refresh.schedule configuration: %s", err) | ||
return refreshScheduleDefault() | ||
} | ||
return ts, scheduleAsStr, nil | ||
return ts, scheduleAsStr, true, nil | ||
} | ||
|
||
return refreshScheduleDefault() | ||
|
@@ -244,13 +245,13 @@ func (m *autoRefresh) launchAutoRefresh() error { | |
return nil | ||
} | ||
|
||
func refreshScheduleDefault() (ts []*timeutil.Schedule, scheduleStr string, err error) { | ||
func refreshScheduleDefault() (ts []*timeutil.Schedule, scheduleStr string, legacy bool, err error) { | ||
refreshSchedule, err := timeutil.ParseSchedule(defaultRefreshSchedule) | ||
if err != nil { | ||
panic(fmt.Sprintf("defaultRefreshSchedule cannot be parsed: %s", err)) | ||
} | ||
|
||
return refreshSchedule, defaultRefreshSchedule, nil | ||
return refreshSchedule, defaultRefreshSchedule, false, nil | ||
} | ||
|
||
func autoRefreshInFlight(st *state.State) bool { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: |
||
// display to a user and a flag indicating whether the schedule is a legacy one. | ||
// The caller should be holding the state lock. | ||
func (m *SnapManager) RefreshSchedule() (string, error) { | ||
func (m *SnapManager) RefreshSchedule() (string, bool, error) { | ||
return m.autoRefresh.RefreshSchedule() | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick^10) I would put schedule and timer close to each other, probably move schedule at the end (under timer).