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: add the interface command #3399
Conversation
zyga
added some commits
May 23, 2017
Some checks were not successful
codecov-io
commented
May 24, 2017
•
Edited 1 time
-
codecov-io
Jun 6, 2017
-
Jun 6, 2017
Codecov Report
@@ Coverage Diff @@
## master #3399 +/- ##
=========================================
+ Coverage 75.06% 75.2% +0.13%
=========================================
Files 383 384 +1
Lines 33238 33538 +300
=========================================
+ Hits 24951 25222 +271
- Misses 6477 6494 +17
- Partials 1810 1822 +12
Continue to review full report at Codecov.
|
| +type InterfaceInfo struct { | ||
| + MetaData InterfaceMetaData `json:"meta-data"` | ||
| + Plugs []Plug `json:"plugs,omitempty"` | ||
| + Slots []Slot `json:"slots,omitempty"` |
niemeyer
May 29, 2017
Contributor
Can we merge InterfaceInfo and InterfaceMetaData into a single type named Interface?
Note that the current Interfaces type holds Plugs and Slots, exactly the same as above.
| // InterfaceMetaData contains meta-data about a given interface type. | ||
| type InterfaceMetaData struct { | ||
| - Description string `json:"description,omitempty"` | ||
| + Summary string `json:"summary,omitempty"` |
| - Description string `json:"description,omitempty"` | ||
| + Summary string `json:"summary,omitempty"` | ||
| + Description string `json:"description,omitempty"` | ||
| + DocumentationURL string `json:"documentation-url,omitempty"` |
niemeyer
May 29, 2017
Contributor
DocURL (doc-url)? I'm generally not the one suggesting such shortenings, but that's a very common prefix, 10 less letters, and no reading loss.
| @@ -82,6 +91,12 @@ func (client *Client) Interfaces() (interfaces Interfaces, err error) { | ||
| return | ||
| } | ||
| +// InterfaceInfos returns all interfaces, their meta-data, plugs and slots. | ||
| +func (client *Client) InterfaceInfos() (infos map[string]InterfaceInfo, err error) { |
niemeyer
May 29, 2017
Contributor
This is sort of getting back to the prior design, except in a different endpoint. How about:
- Interface(name string) (*Interface, error)
- InterfaceNames() ([]string, error)
?
zyga
May 30, 2017
Contributor
On second thought. How would this work with the current implementation that just displays a list of name + summary? We'd have to make 1+N calls to do that (one to get the names, then one for each name).
How about we return some basic meta-data (summary) alongside the name from InterfaceNames()? (perhaps it should be renamed to be appropriate)
zyga
Jun 5, 2017
Contributor
I did something like this (separate methods) but still allowed the "list" method to return both name and summary.
zyga
added some commits
May 30, 2017
Some checks were not successful
All checks have passed
All checks have passed
| + snap interface | MATCH 'core-support\s+ special permissions for the core snap' | ||
| + snap interface --all | MATCH 'classic-support\s+ special permissions for the classic snap' | ||
| + snap interface core-support > out.yaml | ||
| + diff -u out.yaml snap-interface-core-support.yaml |
mvo5
Jun 7, 2017
Collaborator
When I was reading this I was wondering where snap-interface-core-support.yaml comes from, maybe just create it in the task.yaml itself via a here shell construct?
| +// Interface holds information about a given interface and its instances. | ||
| +type Interface struct { | ||
| + Name string `json:"name,omitempty"` | ||
| + Summary string `json:"summary,omitempty"` | ||
| Description string `json:"description,omitempty"` |
niemeyer
Jun 9, 2017
Contributor
I suggest dropping this for the time being. Summary + DocURL should cover it.
| Description string `json:"description,omitempty"` | ||
| + DocsURL string `json:"docs-url,omitempty"` |
| + DocsURL string `json:"docs-url,omitempty"` | ||
| + Plugs []Plug `json:"plugs,omitempty"` | ||
| + Slots []Slot `json:"slots,omitempty"` | ||
| + Used bool `json:"used,omitempty"` |
niemeyer
Jun 9, 2017
Contributor
The term is too vague given the context. Does it mean existing plugs? slots? connections?
Also, it most likely means the client is receiving data it doesn't need to. Better to filter upfront I think.
zyga
Jun 9, 2017
Contributor
This is so that we can ask for snap interface --all and see anything our snapd understands, even if we don't use it.
| +// InterfaceIndex returns the list of all interface names. | ||
| +// | ||
| +// The result will only have three fields populated, Name, Summary and Used. | ||
| +func (client *Client) InterfaceIndex() (ifaces []Interface, err error) { |
niemeyer
Jun 9, 2017
Contributor
/v2/snaps
/v2/interfaces
/v2/changes
/v2/users
/v2/sections
/v2/aliases
@zyga @chipaca Something smelling strange about /v2/interface. We should try to preserve some sort of consistency in the API across the several endpoints.
Looking through it, here is a proposal:
- Rename the current Interfaces method to Connections (we can worry about the type later)
- Use
Interfacesto return []*Interface; note the pointer result. Every type should be a pointer in these cases unless you have a good reason to believe return by value is the right thing to do (again, see existing cases) - Introduce
InterfacesOptionsas{Doc, Plugs, Slots, Connected bool}, meaning whether to return documentation, plugs, slots, and whether to return only connected interfaces. - We can provide "connected" via a "select" field, analogous to the one we use for snaps, and then allow specifying "select=all" so that we preserve the result as-is when "select" is not provided, preserving compatibility with existing clients (including the current Interfaces method which will now become Connections). Eventually we can obsolete that result as we'll likely kill the "interfaces" command and introduce a more polished "connections" one, per our conversations.
- We should also support a "names" parameter in the Interfaces method analogous to the List method ("interfaces" parameter in the HTTP request, if we want to preserve the convention established by snaps). This would allow only a subselection of interfaces to be listed.
What do you think?
zyga
Jul 17, 2017
Contributor
This makes total sense, on 2nd read, with fresh head. I'm mid-way through the implementation. Hopefully will be in a reviewable state by EOD today.
| +} | ||
| + | ||
| +// Interface returns all interfaces, their meta-data, plugs and slots. | ||
| +func (client *Client) Interface(name string) (iface Interface, err error) { |
| + } `positional-args:"true"` | ||
| +} | ||
| + | ||
| +var shortInterfaceHelp = i18n.G("Lists interfaces in the system") |
| + | ||
| +var shortInterfaceHelp = i18n.G("Lists interfaces in the system") | ||
| +var longInterfaceHelp = i18n.G(` | ||
| +The interface command lists interfaces available in the system. |
niemeyer
Jun 9, 2017
Contributor
The interface command shows details of snap interfaces.
If no interface name is provided, a list of interface names with at least
one connection is shown, or a list of all interfaces if --all is provided.
| +The interface command lists interfaces available in the system. | ||
| + | ||
| +By default a list of all used interfaces, along with a short summary, is | ||
| +displayed. Use the --all option to include unused interfaces. |
niemeyer
Jun 9, 2017
Contributor
Per note above, instead of unused we need to say what it really means.
| +By default a list of all used interfaces, along with a short summary, is | ||
| +displayed. Use the --all option to include unused interfaces. | ||
| + | ||
| +$ snap interfaces [--attrs] <interface> |
niemeyer
Jun 9, 2017
Contributor
This and the paragraph below may be dropped. It's just restating the documentation of --attrs.
| + return err | ||
| + } | ||
| + if len(ifaces) == 0 { | ||
| + return fmt.Errorf(i18n.G("no interfaces found")) |
zyga
Jun 9, 2017
Contributor
It can happen before you pull in the core snap, no snaps, no interfaces used.
niemeyer
Jun 19, 2017
Contributor
Yeah, so it'd be nicer to be more clear about what's going on, instead of making the user wonder.
niemeyer
Jun 19, 2017
Contributor
"no interfaces currently connected" would probably be better, for example.
| + if iface.Summary != "" { | ||
| + fmt.Fprintf(w, "summary:\t%s\n", iface.Summary) | ||
| + } | ||
| + if iface.Description != "" { |
| + fmt.Fprintf(w, "description: |\n%s\n", formatDescr(iface.Description, termWidth)) | ||
| + } | ||
| + if iface.DocsURL != "" { | ||
| + fmt.Fprintf(w, "docs-url:\t%s\n", iface.DocsURL) |
niemeyer
Jun 9, 2017
Contributor
I think this can say:
("documentation:\t%s\n", iface.DocURL)
Perhaps even dropping those \t? Not sure if they're helping much in this case.
| + } | ||
| + if len(iface.Plugs) > 0 { | ||
| + fmt.Fprintf(w, "plugs:\n") | ||
| + for _, plug := range iface.Plugs { |
niemeyer
Jun 9, 2017
Contributor
This might be much nicer as:
plugs:
- mysnap:myplug
- othesnap:otherplug
slots:
- blah:blah
zyga
Jul 14, 2017
•
Edited 1 time
-
zyga
Jul 14, 2017
Contributor
-
Jul 14, 2017
This would preclude us from displaying attributes or even useful things like the label. Let me experiment to see what I can do to display both.
EDIT: OK, I think you will be happy, it's exactly as you specified but I can still show label and attributes if there are some.
|
@zyga This is ready for some love. |
zyga
referenced this pull request
Jul 10, 2017
Closed
overlord: populate interface label with summary #3575
|
If this needs to be reworked, should this PR be closed meanwhile? |
chipaca
added
the
Decaying
label
Jul 10, 2017
|
Yes, I'll close it and re-open once everything is addressed. |
zyga
closed this
Jul 10, 2017
zyga
added some commits
Jul 14, 2017
All checks have failed
|
@zyga I'm reopening this and pulling it into the review sprint, as you seemed interested in focusing on this next. |
niemeyer
reopened this
Jul 17, 2017
zyga
added some commits
Jul 17, 2017
zyga
removed
the
Decaying
label
Jul 24, 2017
|
I think this is now ready for a 2nd review. |
zyga
added some commits
Jul 17, 2017
All checks have failed
All checks have failed
All checks have passed
niemeyer
approved these changes
Jul 25, 2017
I have tried to provide concrete suggestions for how this would LGTM.. if these sound good to you, feel free to move on. Otherwise, let's please talk and agree on something by tomorrow the latest.
| -// Interfaces returns all plugs, slots and their connections. | ||
| -func (client *Client) Interfaces() (interfaces Interfaces, err error) { | ||
| +// Connections returns all plugs, slots and their connections. | ||
| +func (client *Client) Connections() (interfaces Interfaces, err error) { |
niemeyer
Jul 25, 2017
Contributor
Let's please take out the return value names, and go ahead and rename Interfaces (the type) to Connections as well. This will read much better I think.
| _, err = client.doSync("GET", "/v2/interfaces", nil, nil, nil, &interfaces) | ||
| return | ||
| } | ||
| +// InterfaceQueryOptions represents opt-in elements include in responses. | ||
| +type InterfaceQueryOptions struct { |
niemeyer
Jul 25, 2017
Contributor
FindOptions, ListOptions, SnapCtlOptions... perhaps just InterfaceOptions?
| +// InterfaceQueryOptions represents opt-in elements include in responses. | ||
| +type InterfaceQueryOptions struct { | ||
| + Names []string | ||
| + Doc, Plugs, Slots, Connected bool |
niemeyer
Jul 25, 2017
Contributor
For such a long sequence, breaking it down into independent lines will make the code read better. Repeating the bool is less of a problem than the awkwardness of how this looks now.
| + } | ||
| + if opts != nil { | ||
| + if opts.Doc { | ||
| + query.Set("doc", "yes") // Return documentation of each selected interface. |
| + if err != nil { | ||
| + return err | ||
| + } | ||
| + if len(ifaces) == 0 { |
niemeyer
Jul 25, 2017
Contributor
Shouldn't that be returned by the server? What happens if there are two names, and one doesn't exist? It sounds like an error.
zyga
Jul 26, 2017
Contributor
I was looking at snap list which has similar client-side behavior. We could change the response format so that unknown interfaces can be reported but if we do so we should probably use the same approach for other listings.
| + fmt.Fprintf(w, "\n") | ||
| + } | ||
| + if plug.Label != "" { | ||
| + fmt.Fprintf(w, " label:\t%s\n", plug.Label) |
niemeyer
Jul 25, 2017
Contributor
I suspect the output will look quite displeasing and repetitive once we have tons of interfaces with labels.
Imagine:
plugs:
- foo:bar:
label: Foo Bar
- foo:baz:
label: Foo Baz
- foo:buz:
label: Foo Buz
I suggest something like this instead:
- foo:bar (Foo Bar)
- baz:buz (Baz Buz)
- bom:bam (Bom Bam)
zyga
Jul 25, 2017
Contributor
Oh, even better. Thanks!
I think we can still stick key-value attributes below (for when we actually have any).
zyga
Jul 26, 2017
Contributor
I also tweaked the attributes so that the word attributes is not shown, have a look. I like it a lot.
| +} | ||
| + | ||
| +func (x *cmdInterface) showAttrs(w io.Writer, attrs map[string]interface{}, indent string) { | ||
| + if len(attrs) == 0 || !x.ShowAttrs { |
niemeyer
Jul 25, 2017
Contributor
It's less confusing and less error prone to have the !x.ShowAttrs happening at the call site instead: to not show attrs, don't call show attrs. The len(attrs) == 0 can stay here, though, as this is just the zero case of the function.
| @@ -80,6 +80,112 @@ func (r *Repository) AddInterface(i Interface) error { | ||
| return nil | ||
| } | ||
| +// QueryOptions describes options for QueryInterfaces. |
niemeyer
Jul 25, 2017
Contributor
The terminology in the repository is getting a bit wild.. a few hints:
- We have tons of "queries" in the repository already. Why is that different?
- What is the difference between Info and MetaData? Note we just fixed that in the client.
I think we should change some of the existing API to also talk about "connections", but we don't have to do that now so we can both focus on more interesting problems. That said, I'll try to provide some ideas below for how to not make the API too wild for now either.
zyga
Jul 26, 2017
Contributor
I agree about connections, not just the new API end-point and new snap subcommand but also as a general term.
I think I applied all the suggestions below. Let me know if I missed anything.
| +// Plugs: return information about plugs. | ||
| +// Slots: return information about slots. | ||
| +// Connected: only consider interfaces with at least one connection. | ||
| +type QueryOptions struct { |
niemeyer
Jul 25, 2017
Contributor
One solution might be to organize that new functionality in terms of "information about interfaces", which is something we don't have yet. For that, we can call this type "InfoOptions".
| +// Connected: only consider interfaces with at least one connection. | ||
| +type QueryOptions struct { | ||
| + Names []string | ||
| + Doc, Plugs, Slots, Connected bool |
niemeyer
Jul 25, 2017
Contributor
Same as in the client, let's please split it down into independent lines.
| + Doc, Plugs, Slots, Connected bool | ||
| +} | ||
| + | ||
| +func (r *Repository) queryInterface(iface Interface, opts *QueryOptions) *InterfaceInfo { |
| + | ||
| +func (r *Repository) queryInterface(iface Interface, opts *QueryOptions) *InterfaceInfo { | ||
| + // NOTE: QueryOptions.Connected is handled by QueryInterfaces | ||
| + md := MetaDataOf(iface) |
niemeyer
Jul 25, 2017
Contributor
Same as for the client, we need to either unify metadata and info, or give MetaData a better name. As a way to move forward and not have to refactor too much right away, I suggest calling MetaData as StaticInfo, which gives a more clear delta between the place of each of these in our world: StaticInfo is the static details provided at interface declaration time, and which we do have in Info as well.
zyga
Jul 26, 2017
Contributor
I like StaticInfo, it was conveying the "bag of stuff baked into the binary" idea I had before better than the generic meta-data.
| + // NOTE: QueryOptions.Connected is handled by QueryInterfaces | ||
| + md := MetaDataOf(iface) | ||
| + ifaceName := iface.Name() | ||
| + ii := &InterfaceInfo{ |
niemeyer
Jul 25, 2017
Contributor
Let's please rename InterfaceInfo to Info. This is already interfaces.Info.
| + ifaceName := iface.Name() | ||
| + ii := &InterfaceInfo{ | ||
| + Name: ifaceName, | ||
| + MetaData: MetaData{Summary: md.Summary}, |
| +// If names is empty then all interfaces are considered. Query options decide | ||
| +// which data to return but can also skip interfaces without connections. See | ||
| +// the documentation of QueryOptions for details. | ||
| +func (r *Repository) QueryInterfaces(opts *QueryOptions) []*InterfaceInfo { |
niemeyer
Jul 25, 2017
Contributor
This can be the Info method, taking InfoOptions in and returning []*interfaces.Info.
zyga
added some commits
Jul 26, 2017
Some checks were not successful
|
Merging, the issue is unrelated to the PR (likely master broke with the release of stable core snap) |
zyga commentedMay 24, 2017
•
Edited 1 time
-
zyga
Jul 24, 2017
This branch adds a new command that displays interface information. By default it shows all interfaces
that have at least one connection. When invoked with an interface name it displays details of that
particular interface, such as the plugs and slots, labels and attributes (if requested).