snap: only show "next" refresh time if its after the hold time #5789
Conversation
We got a bugreport that the the output of `snap refresh --time` is confusing when the "core.refresh.hold" option is used. The issue here is that "next" may display a time before the hold time set by the user. The reason is that the internal next will always be calculated and then when its about to run we check against the hold time. But this is something internal so an easy fix to avoid confusion is to simply hide "next" if its before "hold". See https://forum.snapcraft.io/t/7230
|
doing this though we lose information, if the user resets the hold that will be the next time, maybe we should display the next but mark it somehow, as "held next: TIME", or "next: TIME (but held)", or "next: (TIME)" in parenthesis |
|
@pedronis Yeah, the fact that |
| if next.Before(hold) { | ||
| fmt.Fprintf(Stdout, "next: %s (but held)\n", x.fmtTime(next)) | ||
| } else { | ||
| fmt.Fprintf(Stdout, "next: %s\n", x.fmtTime(next)) |
There was a problem hiding this comment.
This branch does not seem to be tested.
There was a problem hiding this comment.
The next: %s\n output is tested in TestRefreshTimer and TestRefreshLegacyTime (or do you mean something else maybe?).
cmd/snap/cmd_snap_op.go
Outdated
| fmt.Fprintf(Stdout, "next: %s\n", x.fmtTime(t)) | ||
| // only show "next" if its after "hold" to not confuse users | ||
| if !next.IsZero() { | ||
| if next.Before(hold) { |
There was a problem hiding this comment.
Before || Equal? Maybe it does not matter as it's running at the ensure loop pace anyway.
There was a problem hiding this comment.
Yes, this is correct. I doubt its a big deal in practise given the precision of time.Time but its correct so I added it.
Codecov Report
@@ Coverage Diff @@
## master #5789 +/- ##
==========================================
- Coverage 79.13% 78.96% -0.17%
==========================================
Files 530 538 +8
Lines 40412 41941 +1529
==========================================
+ Hits 31980 33120 +1140
- Misses 5845 6108 +263
- Partials 2587 2713 +126
Continue to review full report at Codecov.
|
We got a bugreport that the the output of
snap refresh --timeis confusing when the "core.refresh.hold" option is used. The
issue here is that "next" may display a time before the hold
time set by the user. The reason is that the internal next
will always be calculated and then when its about to run we
check against the hold time. But this is something internal
so an easy fix to avoid confusion is to simply hide "next"
if its before "hold".
See https://forum.snapcraft.io/t/7230