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: abbreviated forms of disconnect #2066
Conversation
zyga
and others
added some commits
Apr 18, 2016
zyga
referenced this pull request
Oct 3, 2016
Closed
cmd, disconnect: abbreviated forms of disconnect #2046
zyga
added some commits
Oct 3, 2016
| -func (r *Repository) disconnectEverythingFromSnap(slotSnapName string) error { | ||
| - if _, ok := r.slots[slotSnapName]; !ok { | ||
| - return fmt.Errorf("cannot disconnect plug from snap %q, no such snap", slotSnapName) | ||
| +// ResolveDisconnectAll finds all of the connections that would be disconnected by DisconnectAll() |
niemeyer
Oct 3, 2016
•
Edited 1 time
-
niemeyer
Oct 3, 2016
Contributor
-
Oct 3, 2016
That sounds strange. It's this function that defines what DisconnectAll should disconnect, not the other way around. Suggestion for rename and documentation:
// Connected returns references for all connections that are currently
// established with the provided plug or slot.
// DisconnectAll disconnects all provided connection references.
niemeyer
Oct 3, 2016
Contributor
Updated comment. Connected might be good enough, actually. This is much less magical than ResolveConnect, as there isn't much "resolving" going on.
| - for plug := range r.slotPlugs[slot] { | ||
| + var conns []ConnRef | ||
| + if plugOrSlotName == "" { | ||
| + // "wildcard" mode, affect all the plugs or slots in this snap |
niemeyer
Oct 3, 2016
Contributor
Do we have any real use cases for this today? We don't want this in the CLI, so if there are no other use cases we should probably just error for now.
zyga
Oct 4, 2016
Contributor
It doesn't seem that we do. I've simplified the code and removed this case.
| + return conns, nil | ||
| +} | ||
| + | ||
| +// DisconnectAll disconnects all of the given connections, returning the list of affected snap names. |
niemeyer
Oct 3, 2016
Contributor
Why are we returning a list of snap names? We asked the repository itself to resolve the list of affected connections for us, so we already have it at hand are actually handing it in. Feels strange to get this list out from this function.
zyga
Oct 3, 2016
Contributor
You are right, we don't have to get this back from the function. I was just transcribing things mechanically. I'll get rid of it and move the relevant logic to ifacestate where we actually need it.
zyga
added some commits
Oct 4, 2016
|
Please update task.yaml following this change from John: https://github.com/chipaca/snappy/blob/7e5c62e78c2888d766b99673c6b4858f907246d7/tests/main/snap-connect/task.yaml |
stolowski
reviewed
Oct 4, 2016
Looks good overall. One minor comment below about the strictness of snap/plug/snap/slot argument checking.
| + return err | ||
| + } | ||
| + affectedConns = []interfaces.ConnRef{{*plugRef, *slotRef}} | ||
| + } else if plugRef.Name != "" { |
stolowski
Oct 4, 2016
Contributor
Should we try to explicitly list all the valid cases in the if/else clauses here? In this case for example user could provide plug name and slot snap name, hoping that this is going to narrow the effect of disconnect down, but in fact I think it would be silently ignored?
zyga
Oct 4, 2016
Contributor
This would result in an error from repo but I can easily make the test more obvious. I think you are also right since we are multiplexing to one of two functions depending on argument layout.
Done, have a look please.
zyga
added some commits
Oct 4, 2016
|
LGTM. +1 |
| - | ||
| -Disconnects all plugs from the provided snap. | ||
| +Disconnects everything from the provided plug or slot. | ||
| +The snap name may be omitted for the OS snap. |
| - | ||
| -Disconnects all plugs from the provided snap. | ||
| +Disconnects everything from the provided plug or slot. | ||
| +The snap name may be omitted for the OS snap. |
| + case r.slots["ubuntu-core"] != nil: | ||
| + snapName = "ubuntu-core" | ||
| + default: | ||
| + return nil, fmt.Errorf("cannot resolve disconnect, snap name is empty") |
niemeyer
Oct 4, 2016
•
Edited 1 time
-
niemeyer
Oct 4, 2016
Contributor
-
Oct 4, 2016
s/cannot resolve disconnect, // (not a disconnection in this context)
| + } | ||
| + var conns []ConnRef | ||
| + if plugOrSlotName == "" { | ||
| + return nil, fmt.Errorf("cannot resolve disconnect, plug or slot name is empty") |
zyga commentedOct 3, 2016
This picks up #2046
WIP