Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: implement new REST API: GET /2.0/interfaces #559
Conversation
zyga
added some commits
Feb 29, 2016
niemeyer
changed the title from
interfaces,daemon,docs,client,cmd/snap: implement new REST API: GET /2.0/interfaces
to
many: implement new REST API: GET /2.0/interfaces
Mar 1, 2016
niemeyer
reviewed
Mar 1, 2016
| @@ -26,30 +26,44 @@ import ( | ||
| // Plug represents a capacity offered by a snap. | ||
| type Plug struct { | ||
| + Snap string `json:"snap"` | ||
| // NOTE: the discrepancy between go and json field name is intentional. |
niemeyer
reviewed
Mar 1, 2016
| } | ||
| // Slot represents the potential of a given snap to connect to a given plug. | ||
| type Slot struct { | ||
| + Snap string `json:"snap"` | ||
| // NOTE: the discrepancy between go and json field name is intentional. |
niemeyer
reviewed
Mar 1, 2016
| - Plug | ||
| - Connections []Slot `json:"connections"` | ||
| +// InterfaceConnections contains information about all plugs, slots and their connections | ||
| +type InterfaceConnections struct { |
niemeyer
Mar 1, 2016
Contributor
s/InterfaceConnections/Interfaces/
This lists plugs and slots, whether connected or not.
niemeyer
reviewed
Mar 1, 2016
| -// AllPlugs returns information about all the plugs and their connections. | ||
| -func (client *Client) AllPlugs() (connections []PlugConnections, err error) { | ||
| +// InterfaceConnections returns information about all the plugs, slots and their connections. | ||
| +func (client *Client) InterfaceConnections() (connections InterfaceConnections, err error) { |
niemeyer
reviewed
Mar 1, 2016
| - }) | ||
| - } | ||
| - return SyncResponse(plugs) | ||
| +// getInterfaces returns a response with a list of all the plugs and slots and their connections. |
niemeyer
Mar 1, 2016
Contributor
"returns a response with a list of all the plugs and slots" => "returns all plugs and slots"
niemeyer
reviewed
Mar 1, 2016
| @@ -59,8 +73,8 @@ type InterfaceAction struct { | ||
| Slot *Slot `json:"slot,omitempty"` | ||
| } | ||
| -// AllPlugs returns information about all the plugs and their connections. | ||
| -func (client *Client) AllPlugs() (connections []PlugConnections, err error) { | ||
| +// InterfaceConnections returns information about all the plugs, slots and their connections. |
niemeyer
reviewed
Mar 1, 2016
| - fmt.Fprintf(w, "%s:%s\t", plug.Snap, plug.Plug.Name) | ||
| + // The OS snap (always ubuntu-core) is special and enable abbreviated | ||
| + // display syntax on the plug-side of the connection. | ||
| + if plug.Snap != "ubuntu-core" { |
niemeyer
Mar 1, 2016
Contributor
Nitpick suggestion for coding style: in most cases it's easier for humans to read branching logic on the positive side.
So, first I'd list the "snap is ubuntu-core" one, then the alternative.
niemeyer
reviewed
Mar 1, 2016
| } | ||
| -type bySlotSnapAndName []*Slot | ||
| +// InterfaceConnections returns object holding a lists of all the plugs and slots and their connections. |
niemeyer
Mar 1, 2016
Contributor
// Interfaces returns all plugs, slots, and their connections.
I wasn't considering having that and SlotRef/PlugRef in the internal APIs, but maybe that's a good idea.
niemeyer
reviewed
Mar 1, 2016
| + Name string `json:"slot"` | ||
| +} | ||
| + | ||
| +// InterfaceConnections contains information about all plugs, slots and their connections |
niemeyer
Mar 1, 2016
Contributor
// Interfaces holds information about a list of plugs and slots, and their connections.
niemeyer
reviewed
Mar 1, 2016
| - | ||
| -type byPlugSnapAndName []*Plug | ||
| +// SlotConnections returns all of the plugs that are connected a given slot. | ||
| +func (r *Repository) SlotConnections(snapName, slotName string) []*Plug { |
niemeyer
Mar 1, 2016
Contributor
That makes no sense if Plug and Slot have a list of Connections in it. Let's talk online on this one.
zyga
Mar 2, 2016
Contributor
I've fixed everything else. I see why this is controversial, let's talk as suggested and find a better way to do this.
zyga
added some commits
Mar 2, 2016
|
LGTM |
mvo5
reviewed
Mar 3, 2016
| } | ||
| -type bySlotSnapAndName []*Slot | ||
| +// Interfaces returns object holding a lists of all the plugs and slots and their connections. | ||
| +func (r *Repository) Interfaces() *Interfaces { |
mvo5
Mar 3, 2016
Collaborator
Pardon my ignorance here, the branch is a bit long. Is this covered directly by tests? It seems like it is worthwhile to test if its not already.
zyga
Mar 3, 2016
Contributor
I have test for this, ironically piled up in a separate branch so that size doesn't grow beyond reviewable bounds.
mvo5
reviewed
Mar 3, 2016
| + | ||
| +package interfaces | ||
| + | ||
| +type bySlotRef []SlotRef |
mvo5
Mar 3, 2016
Collaborator
Is this covered by tests? If not we may consider it - TBH I'm sitting a bit on the fence, its so simple tests look almost like overkill. But then they are probably easy enough (and quick enough) to add :)
|
This looks good to me, I just have two questions and we need to know what to do about SlotConnections(). |
|
@mvo5 SlotConnections and a few other methods are now going away as they are entirely replaced by Interfaces(). I have this piled up after this branch lands. |
zyga commentedMar 1, 2016
This branch implements the new REST API for listing interface connections.
The /2.0/interfaces endpoint now returns an object with two lists, one for plugs and one for slots. This change allows clients to discover disconnected slots. Each of the plugs and slots also contains a list of connections (possibly empty). Each connection is described by a plug or slot "reference", which holds only two identifier fields, "snap" and either "plug" or "slot".
There are supporting changes in the backend, client API and client commands (snap interfaces).