Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Caps api remove capability #211
Conversation
zyga
added some commits
Dec 2, 2015
chipaca
reviewed
Dec 2, 2015
| +// Capability finds and returns the Capability with the given name or nil if it | ||
| +// is not found. | ||
| +func (r *Repository) Capability(name string) *Capability { | ||
| + return r.caps[name] |
zyga
Dec 2, 2015
Contributor
Fixed, had to push with --force because I had some junk staged for an unrelated discussion. Nothing in the "past" above has changed. I've only added the two locking patches below.
zyga
added some commits
Dec 2, 2015
niemeyer
reviewed
Dec 3, 2015
| - var result map[string]map[string]Capability | ||
| - if err := json.Unmarshal(rsp.Result, &result); err != nil { | ||
| - return nil, fmt.Errorf("cannot obtain capabilities: failed to unmarshal response: %v", err) | ||
| + switch rsp.Type { |
niemeyer
Dec 3, 2015
Contributor
This can use the same if err := rsp.err(); err != nil trick mentioned in the underlying branch.
niemeyer
reviewed
Dec 3, 2015
| + "type": "sync", | ||
| + "result": { } | ||
| + }` | ||
| + err := cs.cli.RemoveCapability("n") |
niemeyer
Dec 3, 2015
Contributor
As mentioned in the other branch, would this test pass if the whole body is replaced by return nil?
|
A couple of comments, but LGTM. Please wait for @chipaca. |
zyga
added some commits
Dec 3, 2015
mvo5
reviewed
Dec 3, 2015
| +func (client *Client) RemoveCapability(name string) error { | ||
| + errPrefix := "cannot remove capability" | ||
| + var rsp response | ||
| + if err := client.do("DELETE", fmt.Sprintf("/1.0/capabilities/%s", name), nil, &rsp); err != nil { |
mvo5
Dec 3, 2015
Collaborator
Do we need to be concerned about quoting here? I assume the anser is no, but I would like to be confirmed
zyga
Dec 3, 2015
Contributor
Hmm, in theory the client can send anything and the server won't care. Capability names are constrained to identifier-like names.
Do you suggest that we use %q there?
|
LGTM |
zyga commentedDec 2, 2015
This depends on #208