Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snap: support for snap tasks --last=... #3217
Conversation
stolowski
added some commits
Apr 21, 2017
Some checks were not successful
mvo5
approved these changes
Apr 21, 2017
Looks good, one question/suggestion. Also wondering if we should integrate that into one of our spread tests.
| } | ||
| func (c *cmdChange) Execute([]string) error { | ||
| cli := Client() | ||
| return showChange(cli, c.Positional.ID) | ||
| } | ||
| +func findChangeByKind(changes []*client.Change, kind string) *client.Change { |
stolowski
added some commits
Apr 21, 2017
Some checks were not successful
Some checks were not successful
|
Added a few checks to existing spread tests. |
stolowski
added some commits
Apr 24, 2017
Some checks were not successful
All checks have passed
| + if c.LastChangeType != "" { | ||
| + kind := c.LastChangeType | ||
| + // our internal change types use "-snap" postfix but let user skip it and use short form. | ||
| + if kind == "refresh" || kind == "install" || kind == "remove" || kind == "connect" || kind == "disconnect" || kind == "configure" { |
| } `positional-args:"yes"` | ||
| } | ||
| func init() { | ||
| addCommand("changes", shortChangesHelp, longChangesHelp, func() flags.Commander { return &cmdChanges{} }, nil, nil) | ||
| addCommand("change", shortChangeHelp, longChangeHelp, func() flags.Commander { return &cmdChange{} }, nil, nil).hidden = true | ||
| - addCommand("tasks", shortChangeHelp, longChangeHelp, func() flags.Commander { return &cmdTasks{} }, nil, nil) | ||
| + addCommand("tasks", shortChangeHelp, longChangeHelp, func() flags.Commander { return &cmdTasks{} }, map[string]string{ | ||
| + "last": "Show last change of given type (install, refresh, remove etc.)", |
pedronis
Apr 25, 2017
Contributor
we should list "auto-refresh" here as well, it's an interesting one and not obviously a command
stolowski
added some commits
Apr 26, 2017
All checks have passed
pedronis
requested a review
from
niemeyer
Apr 26, 2017
| + } | ||
| + // sort by date, descending so that we will pick the most recent change of given kind. | ||
| + sort.Sort(sort.Reverse(changesByTime(changes))) | ||
| + chg := findFirstChangeByKind(changes, kind) |
pedronis
Apr 27, 2017
Contributor
given that we need to iterate to filter by kind anyway, we could just find the max spawnTime of a kind in one go, just a consideration
stolowski
Apr 28, 2017
•
Edited 1 time
-
stolowski
Apr 28, 2017
Contributor
-
Apr 28, 2017
Good idea. I was in a "functional programming" mindset doing that change ;). Done.
Some checks were not successful
| } `positional-args:"yes"` | ||
| } | ||
| func init() { | ||
| addCommand("changes", shortChangesHelp, longChangesHelp, func() flags.Commander { return &cmdChanges{} }, nil, nil) | ||
| addCommand("change", shortChangeHelp, longChangeHelp, func() flags.Commander { return &cmdChange{} }, nil, nil).hidden = true | ||
| - addCommand("tasks", shortChangeHelp, longChangeHelp, func() flags.Commander { return &cmdTasks{} }, nil, nil) | ||
| + addCommand("tasks", shortChangeHelp, longChangeHelp, func() flags.Commander { return &cmdTasks{} }, map[string]string{ | ||
| + "last": "Show last change of given type (install, refresh, remove, try, auto-refresh etc.)", |
| @@ -122,14 +125,55 @@ func (c *cmdChanges) Execute(args []string) error { | ||
| func (c *cmdTasks) Execute([]string) error { | ||
| cli := Client() | ||
| - return showChange(cli, c.Positional.ID) | ||
| + var id changeID | ||
| + if c.Positional.ID == "" && c.LastChangeType == "" { |
zyga
Apr 28, 2017
Contributor
This is a personal preference that may not be shared by the rest of the team but I prefer switch case style instead of the if-then-else tree:
switch {
case c.Positional.ID == "" && c.LastChangeType == "":
...
case c.Positional.ID != "" && c.LastChangeType == "":
...
case c.Positional.ID != "":
...
case c.LastChangeType != "":
...
}
You could also combine this with my suggestion below and merge the first two cases.
stolowski
Apr 28, 2017
Contributor
Thanks, good suggestion, I'm not used to this go-specific statements yet, but I like it. Done. I haven't however merged the two error case together, I think they should have separate error messages.
| + return fmt.Errorf(i18n.G("please provide change ID or type with --last=<type>")) | ||
| + } | ||
| + if c.Positional.ID != "" && c.LastChangeType != "" { | ||
| + return fmt.Errorf(i18n.G("change use ID and type together")) |
zyga
Apr 28, 2017
Contributor
the error message is somewhat confusing for me, how about reusing the exact same error message above? (perhaps emphasise the either by saying please provide either change ID or change type with --last=<type>
pedronis
Apr 28, 2017
Contributor
yes, "please provide..." or "please specify..." seems to be the style for this kind of errors we use
stolowski
Apr 28, 2017
Contributor
Thanks for spotting, indeed this comment is totally off, I'm not sure what I was thinking... I've changed it to "cannot use change ID and type together", this is in-line with other messages concerning mutually-exclusive options.
| + // our internal change types use "-snap" postfix but let user skip it and use short form. | ||
| + if kind == "refresh" || kind == "install" || kind == "remove" || kind == "connect" || kind == "disconnect" || kind == "configure" || kind == "try" { | ||
| + kind += "-snap" | ||
| + } |
pedronis
Apr 28, 2017
Contributor
this is not an exhaustive list of all kinds, just the ones for which kind == command+"-snap"
for example auto-refresh is an interesting one not of that form
stolowski
Apr 28, 2017
Contributor
Yes, this list acts as a "shortuct" or set of "aliases" for the user to help with common tasks. But you can ask for any other task if you know the internal type name.
stolowski
added some commits
Apr 28, 2017
stolowski commentedApr 21, 2017
Support for
snap tasks --last=<type>Type can be any of the change types, e.g. "install-snap"; for convienience I made it also understand a set of predefined verbs without "-snap" prefix.
Discussed here: https://forum.snapcraft.io/t/easy-way-to-get-last-change/246