Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add "snap add-cap" command and API #208
Conversation
zyga
added some commits
Dec 2, 2015
chipaca
reviewed
Dec 2, 2015
| + } | ||
| + return resultOk["capabilities"], nil | ||
| + default: | ||
| + return nil, fmt.Errorf("%s: expected sync response, got %s", errPrefix, rsp.Type) |
chipaca
reviewed
Dec 2, 2015
| + case "sync": | ||
| + return nil | ||
| + default: | ||
| + return fmt.Errorf("cannot obtain capabilities: expected sync response, got %s", rsp.Type) |
chipaca
Dec 2, 2015
Member
and %q on the second one here (I know i've said this for one of these two in the previous review... ;-)
chipaca
reviewed
Dec 2, 2015
| - return nil, fmt.Errorf("cannot obtain capabilities: failed to unmarshal response: %v", err) | ||
| + switch rsp.Type { | ||
| + case "error": | ||
| + return rsp.processErrorResponse() |
zyga
Dec 3, 2015
Contributor
Fixed with merges with trunk and some simplification borrowed from other client methods
|
|
niemeyer
reviewed
Dec 2, 2015
| + Type: "t", | ||
| + Attrs: map[string]string{"k": "v"}, | ||
| + } | ||
| + err := cs.cli.AddCapability(cap) |
niemeyer
Dec 2, 2015
Contributor
Please replace all of the code in AddCapability with return nil. Does the test still pass? If so, what is being tested?
zyga
Dec 3, 2015
Contributor
I see your point. Since the method returns nil on success I find it hard to write a compelling test. I'm open to suggestions.
Edit: what I mean by this is that there are no useful side effects we can observe in the test. One thing we can try to do is to just see the POST request being made.
zyga
Dec 3, 2015
Contributor
And fixed, thanks for @chipaca for the tip on how to decipher the body :-)
niemeyer
reviewed
Dec 2, 2015
| + | ||
| +func init() { | ||
| + _, err := parser.AddCommand("add-cap", shortAddCapHelp, longAddCapHelp, &cmdAddCap{ | ||
| + addCapOptions{ |
niemeyer
Dec 2, 2015
Contributor
There seems to be too much spacing here when this would all fit in a single line comfortably, and in fact none of that is needed.. &cmdAppCap{} is exactly the same as &cmdAppCap{addCapOptions{Attrs: nil}}.
niemeyer
reviewed
Dec 2, 2015
| + // Verify (external) | ||
| + c.Check(rsp.Type, check.Equals, ResponseTypeError) | ||
| + c.Check(rsp.Status, check.Equals, 400) | ||
| + c.Check(rsp.Result.(*errorResult).Msg, check.Matches, `can't add capability`) |
niemeyer
Dec 2, 2015
Contributor
Can this please be replaced by an ErrorMatches on rsp.err(), and do a Equals check on the whole message, testing that it contains the relevant information we expect (including the fact that the name clashed, and what it was).
chipaca
Dec 3, 2015
Member
this was gustavo getting confused with similar problems in client and daemon. Ignore this particular comment :-)
|
Nice branch. A couple of comments, and LGTM with them addressed. |
zyga
added some commits
Dec 3, 2015
zyga commentedDec 2, 2015
This branch depends on #204
Similar to "get all capabilities" branch earlier, this branch adds the REST API, client API and command line interface for adding capabilities to snappy daemon.