-
Notifications
You must be signed in to change notification settings - Fork 575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
many: support 'system' nickname in interfaces #5080
many: support 'system' nickname in interfaces #5080
Conversation
When handling requests that act on something, make sure to accept snap nicknames seamlessly. When returning data on GET requests, do not apply the nicknames so as to not break the API stability. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #5080 +/- ##
==========================================
- Coverage 79.01% 79.01% -0.01%
==========================================
Files 488 489 +1
Lines 36477 36597 +120
==========================================
+ Hits 28823 28917 +94
- Misses 5369 5388 +19
- Partials 2285 2292 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
cmd/snap/cmd_interfaces.go
Outdated
@@ -100,19 +121,20 @@ func (x *cmdInterfaces) Execute(args []string) error { | |||
} | |||
// The OS snap is special and enable abbreviated | |||
// display syntax on the slot-side of the connection. | |||
if slot.Snap == "core" || slot.Snap == "ubuntu-core" { | |||
if slot.Snap == "core" || slot.Snap == "ubuntu-core" || slot.Snap == snap.Nickname("core") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a growing number of places that do check like this, I wonder if it's time for a helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT this is the only place where all 3 options appear.
cmd/snap/cmd_interfaces.go
Outdated
ok = wantedSnap == slot.Connections[i].Snap | ||
if !ok && wantedSnap == snap.Nickname(slot.Connections[i].Snap) { | ||
askedWithNick = true | ||
ok = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the above checks could use a helper:
matches, askedWithNick := isWantedSnap(snapname)
and the helper would internaly check nickaname as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable. Just a few points:
cmd/snap/cmd_interfaces.go
Outdated
fmt.Fprintf(w, ":%s\t", slot.Name) | ||
} else { | ||
fmt.Fprintf(w, "%s:%s\t", slot.Snap, slot.Name) | ||
fmt.Fprintf(w, "%s:%s\t", maybeAsNickname(slot.Snap, askedWithNick), slot.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't feel like we gain much from varying here based on the snap. Just feels more magic, which isn't necessarily good. In fact, the case for core, which is the system snap and the only nickname we care about right now, is already special case here in a custom branch right above. So the change seems to introduce extra complexity without much of a win. Plus, this whole command is a bit unfortunate.. we really need that "connections" command we discussed a few times.
So perhaps we should keep this simpler and more straightforward for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting I should drop maybeAsNickname()
here, as well as where connections and unconnected plugs are printed or just this particular location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this on IRC
daemon/api.go
Outdated
@@ -1852,6 +1852,13 @@ func changeInterfaces(c *Command, r *http.Request, user *auth.UserState) Respons | |||
st.Lock() | |||
defer st.Unlock() | |||
|
|||
for i := range a.Plugs { | |||
a.Plugs[i].Snap = systemCoreSnapUnalias(a.Plugs[i].Snap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"alias" means something else in the snap world. Perhaps;
- useSnapNick
- dropSnapNick
snap/info.go
Outdated
|
||
// Nickname returns the nickname for given snap name. If there is none, returns | ||
// the original name. | ||
func Nickname(snapName string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same function we have above? Can we use the same without cycles?
Also, perhaps similar to the above point:
- snap.UseNick
- snap.DropNick
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
|
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one.
This is ready for re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice! What I find interesting is that maybe we can use something like snap.UseNick,DropNick
also for upcoming installs. The problem is somewhat similar.
There was a certain time span between canonical#5080 being last built and merging it, meanwhile some incompatible changes entered master. Update the tests to accomodate for API changes. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
The series add support for
system
nickname in interface endpoints and commands. It is possible to connect/disconnect/list interfaces usingsystem
, eg:To preserve the API stability of snapd, the
system
nickname is silently accepted in operations that modify connections. However, endpoints that return connections/interfaces will not outputsystem
by default and instead usecore
. This is because the daemon cannot tell whether the client is aware of the nickname or not. For simplicity,snap
will stransformcore
tosystem
insnap interfaces
output whensystem
was passed in command line.snap interface
OTOH usessystem
by default in its output.