Skip to content
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

Merged
merged 10 commits into from
May 22, 2018
9 changes: 5 additions & 4 deletions cmd/snap/cmd_interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

"github.com/snapcore/snapd/client"
"github.com/snapcore/snapd/i18n"
"github.com/snapcore/snapd/snap"

"github.com/jessevdk/go-flags"
)
Expand Down Expand Up @@ -120,9 +121,9 @@ func (x *cmdInterface) showOneInterface(iface *client.Interface) {
labelPart = fmt.Sprintf(" (%s)", plug.Label)
}
if plug.Name == iface.Name {
fmt.Fprintf(w, " - %s%s", plug.Snap, labelPart)
fmt.Fprintf(w, " - %s%s", snap.Nickname(plug.Snap), labelPart)
} else {
fmt.Fprintf(w, ` - %s:%s%s`, plug.Snap, plug.Name, labelPart)
fmt.Fprintf(w, ` - %s:%s%s`, snap.Nickname(plug.Snap), plug.Name, labelPart)
}
// Print a colon which will make the snap:plug element a key-value
// yaml object so that we can write the attributes.
Expand All @@ -142,9 +143,9 @@ func (x *cmdInterface) showOneInterface(iface *client.Interface) {
labelPart = fmt.Sprintf(" (%s)", slot.Label)
}
if slot.Name == iface.Name {
fmt.Fprintf(w, " - %s%s", slot.Snap, labelPart)
fmt.Fprintf(w, " - %s%s", snap.Nickname(slot.Snap), labelPart)
} else {
fmt.Fprintf(w, ` - %s:%s%s`, slot.Snap, slot.Name, labelPart)
fmt.Fprintf(w, ` - %s:%s%s`, snap.Nickname(slot.Snap), slot.Name, labelPart)
}
// Print a colon which will make the snap:slot element a key-value
// yaml object so that we can write the attributes.
Expand Down
2 changes: 1 addition & 1 deletion cmd/snap/cmd_interface_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func (s *SnapSuite) TestInterfaceDetails(c *C) {
" - deepin-music\n" +
" - http\n" +
"slots:\n" +
" - core\n"
" - system\n"
c.Assert(s.Stdout(), Equals, expectedStdout)
c.Assert(s.Stderr(), Equals, "")
}
Expand Down
51 changes: 41 additions & 10 deletions cmd/snap/cmd_interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"fmt"

"github.com/snapcore/snapd/i18n"
"github.com/snapcore/snapd/snap"

"github.com/jessevdk/go-flags"
)
Expand Down Expand Up @@ -67,6 +68,13 @@ func init() {
}})
}

func maybeAsNickname(name string, useNick bool) string {
if useNick {
return snap.Nickname(name)
}
return name
}

func (x *cmdInterfaces) Execute(args []string) error {
if len(args) > 0 {
return ErrExtraArgs
Expand All @@ -82,11 +90,24 @@ func (x *cmdInterfaces) Execute(args []string) error {
w := tabWriter()
defer w.Flush()
fmt.Fprintln(w, i18n.G("Slot\tPlug"))

wantedSnap := x.Positionals.Query.Snap

for _, slot := range ifaces.Slots {
if wanted := x.Positionals.Query.Snap; wanted != "" {
ok := wanted == slot.Snap
askedWithNick := false
if wantedSnap != "" {
ok := wantedSnap == slot.Snap
if !ok && wantedSnap == snap.Nickname(slot.Snap) {
askedWithNick = true
ok = true
}

for i := 0; i < len(slot.Connections) && !ok; i++ {
ok = wanted == slot.Connections[i].Snap
ok = wantedSnap == slot.Connections[i].Snap
if !ok && wantedSnap == snap.Nickname(slot.Connections[i].Snap) {
askedWithNick = true
ok = true
Copy link
Contributor

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.

}
}
if !ok {
continue
Expand All @@ -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") {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

}
for i := 0; i < len(slot.Connections); i++ {
if i > 0 {
fmt.Fprint(w, ",")
}
if slot.Connections[i].Name != slot.Name {
fmt.Fprintf(w, "%s:%s", slot.Connections[i].Snap, slot.Connections[i].Name)
fmt.Fprintf(w, "%s:%s", maybeAsNickname(slot.Connections[i].Snap, askedWithNick),
slot.Connections[i].Name)
} else {
fmt.Fprintf(w, "%s", slot.Connections[i].Snap)
fmt.Fprintf(w, "%s", maybeAsNickname(slot.Connections[i].Snap, askedWithNick))
}
}
// Display visual indicator for disconnected slots
Expand All @@ -124,8 +146,17 @@ func (x *cmdInterfaces) Execute(args []string) error {
// Plugs are treated differently. Since the loop above already printed each connected
// plug, the loop below focuses on printing just the disconnected plugs.
for _, plug := range ifaces.Plugs {
if x.Positionals.Query.Snap != "" && x.Positionals.Query.Snap != plug.Snap {
continue
maybeNick := plug.Snap
if wantedSnap != "" {
ok := wantedSnap == plug.Snap
nick := snap.Nickname(plug.Snap)
if !ok && wantedSnap == nick {
maybeNick = nick
ok = true
}
if !ok {
continue
}
}
if x.Positionals.Query.Name != "" && x.Positionals.Query.Name != plug.Name {
continue
Expand All @@ -135,7 +166,7 @@ func (x *cmdInterfaces) Execute(args []string) error {
}
// Display visual indicator for disconnected plugs.
if len(plug.Connections) == 0 {
fmt.Fprintf(w, "-\t%s:%s\n", plug.Snap, plug.Name)
fmt.Fprintf(w, "-\t%s:%s\n", maybeNick, plug.Name)
}
}
return nil
Expand Down
56 changes: 56 additions & 0 deletions cmd/snap/cmd_interfaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,62 @@ func (s *SnapSuite) TestConnectionsOfSpecificSnap(c *C) {
c.Assert(s.Stderr(), Equals, "")
}

func (s *SnapSuite) TestConnectionsOfSystemNicknameSnap(c *C) {
s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
c.Check(r.Method, Equals, "GET")
c.Check(r.URL.Path, Equals, "/v2/interfaces")
body, err := ioutil.ReadAll(r.Body)
c.Check(err, IsNil)
c.Check(body, DeepEquals, []byte{})
EncodeResponseBody(c, w, map[string]interface{}{
"type": "sync",
"result": client.Connections{
Slots: []client.Slot{
{
Snap: "core",
Name: "core-support",
Interface: "some-iface",
Connections: []client.PlugRef{{Snap: "core", Name: "core-support-plug"}},
}, {
Snap: "foo",
Name: "foo-slot",
Interface: "foo-slot-iface",
},
},
Plugs: []client.Plug{
{
Snap: "core",
Name: "core-support-plug",
Interface: "some-iface",
Connections: []client.SlotRef{{Snap: "core", Name: "core-support"}},
},
},
},
})
})
rest, err := Parser().ParseArgs([]string{"interfaces", "core"})
c.Assert(err, IsNil)
c.Assert(rest, DeepEquals, []string{})
expectedStdout := "" +
"Slot Plug\n" +
":core-support core:core-support-plug\n"
c.Assert(s.Stdout(), Equals, expectedStdout)
c.Assert(s.Stderr(), Equals, "")

s.ResetStdStreams()

// when called with system nickname, references to core will be replaced
// with nickname as well
rest, err = Parser().ParseArgs([]string{"interfaces", "system"})
c.Assert(err, IsNil)
c.Assert(rest, DeepEquals, []string{})
expectedStdoutSystem := "" +
"Slot Plug\n" +
":core-support system:core-support-plug\n"
c.Assert(s.Stdout(), Equals, expectedStdoutSystem)
c.Assert(s.Stderr(), Equals, "")
}

func (s *SnapSuite) TestConnectionsOfSpecificSnapAndSlot(c *C) {
s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
c.Check(r.Method, Equals, "GET")
Expand Down
14 changes: 14 additions & 0 deletions daemon/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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

}
for i := range a.Slots {
a.Slots[i].Snap = systemCoreSnapUnalias(a.Slots[i].Snap)
}

switch a.Action {
case "connect":
var connRef interfaces.ConnRef
Expand Down Expand Up @@ -2823,3 +2830,10 @@ func systemCoreSnapUnalias(name string) string {
}
return name
}

func systemCoreSnapAlias(name string) string {
if name == "core" {
return "system"
}
return name
}
10 changes: 10 additions & 0 deletions daemon/api_mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@ slots:
label: label
`

var coreProducerYaml = `
name: core
version: 1
slots:
slot:
interface: test
key: value
label: label
`

var differentProducerYaml = `
name: producer
version: 1
Expand Down
101 changes: 101 additions & 0 deletions daemon/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3836,6 +3836,54 @@ func (s *apiSuite) TestConnectPlugFailureNoSuchSlot(c *check.C) {
c.Assert(ifaces.Connections, check.HasLen, 0)
}

func (s *apiSuite) TestConnectCoreSystemAlias(c *check.C) {
revert := builtin.MockInterface(&ifacetest.TestInterface{InterfaceName: "test"})
defer revert()
d := s.daemon(c)

s.mockSnap(c, consumerYaml)
s.mockSnap(c, coreProducerYaml)

d.overlord.Loop()
defer d.overlord.Stop()

action := &interfaceAction{
Action: "connect",
Plugs: []plugJSON{{Snap: "consumer", Name: "plug"}},
Slots: []slotJSON{{Snap: "system", Name: "slot"}},
}
text, err := json.Marshal(action)
c.Assert(err, check.IsNil)
buf := bytes.NewBuffer(text)
req, err := http.NewRequest("POST", "/v2/interfaces", buf)
c.Assert(err, check.IsNil)
rec := httptest.NewRecorder()
interfacesCmd.POST(interfacesCmd, req, nil).ServeHTTP(rec, req)
c.Check(rec.Code, check.Equals, 202)
var body map[string]interface{}
err = json.Unmarshal(rec.Body.Bytes(), &body)
c.Check(err, check.IsNil)
id := body["change"].(string)

st := d.overlord.State()
st.Lock()
chg := st.Change(id)
st.Unlock()
c.Assert(chg, check.NotNil)

<-chg.Ready()

st.Lock()
err = chg.Err()
st.Unlock()
c.Assert(err, check.IsNil)

repo := d.overlord.InterfaceManager().Repository()
ifaces := repo.Interfaces()
c.Assert(ifaces.Connections, check.HasLen, 1)
c.Check(ifaces.Connections, check.DeepEquals, []*interfaces.ConnRef{{interfaces.PlugRef{Snap: "consumer", Name: "plug"}, interfaces.SlotRef{Snap: "core", Name: "slot"}}})
}

func (s *apiSuite) testDisconnect(c *check.C, plugSnap, plugName, slotSnap, slotName string) {
revert := builtin.MockInterface(&ifacetest.TestInterface{InterfaceName: "test"})
defer revert()
Expand Down Expand Up @@ -4040,6 +4088,59 @@ func (s *apiSuite) TestDisconnectPlugFailureNotConnected(c *check.C) {
})
}

func (s *apiSuite) TestDisconnectCoreSystemAlias(c *check.C) {
revert := builtin.MockInterface(&ifacetest.TestInterface{InterfaceName: "test"})
defer revert()
d := s.daemon(c)

s.mockSnap(c, consumerYaml)
s.mockSnap(c, coreProducerYaml)

repo := d.overlord.InterfaceManager().Repository()
connRef := interfaces.ConnRef{
PlugRef: interfaces.PlugRef{Snap: "consumer", Name: "plug"},
SlotRef: interfaces.SlotRef{Snap: "core", Name: "slot"},
}
c.Assert(repo.Connect(connRef), check.IsNil)

d.overlord.Loop()
defer d.overlord.Stop()

action := &interfaceAction{
Action: "disconnect",
Plugs: []plugJSON{{Snap: "consumer", Name: "plug"}},
Slots: []slotJSON{{Snap: "system", Name: "slot"}},
}
text, err := json.Marshal(action)
c.Assert(err, check.IsNil)
buf := bytes.NewBuffer(text)
req, err := http.NewRequest("POST", "/v2/interfaces", buf)
c.Assert(err, check.IsNil)
rec := httptest.NewRecorder()
interfacesCmd.POST(interfacesCmd, req, nil).ServeHTTP(rec, req)
c.Check(rec.Code, check.Equals, 202)
var body map[string]interface{}
err = json.Unmarshal(rec.Body.Bytes(), &body)
c.Check(err, check.IsNil)
id := body["change"].(string)

st := d.overlord.State()
st.Lock()
chg := st.Change(id)
st.Unlock()
c.Assert(chg, check.NotNil)

<-chg.Ready()

st.Lock()
err = chg.Err()
st.Unlock()
c.Assert(err, check.IsNil)

ifaces := repo.Interfaces()
c.Assert(ifaces.Connections, check.HasLen, 0)
}

func (s *apiSuite) TestUnsupportedInterfaceRequest(c *check.C) {
buf := bytes.NewBuffer([]byte(`garbage`))
req, err := http.NewRequest("POST", "/v2/interfaces", buf)
Expand Down
9 changes: 9 additions & 0 deletions snap/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -929,3 +929,12 @@ func JoinSnapApp(snap, app string) string {
}
return fmt.Sprintf("%s.%s", snap, app)
}

// Nickname returns the nickname for given snap name. If there is none, returns
// the original name.
func Nickname(snapName string) string {
Copy link
Contributor

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

if snapName == "core" {
return "system"
}
return snapName
}
6 changes: 6 additions & 0 deletions snap/info_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -982,3 +982,9 @@ func (s *infoSuite) TestStopModeTypeKillSignal(c *C) {
c.Check(snap.StopModeType(t.stopMode).KillSignal(), Equals, t.killSig)
}
}

func (s *infoSuite) TestNickname(c *C) {
c.Check(snap.Nickname("core"), Equals, "system")
c.Check(snap.Nickname("system"), Equals, "system")
c.Check(snap.Nickname("foo"), Equals, "foo")
}
4 changes: 2 additions & 2 deletions tests/main/snap-interface/snap-interface-core-support.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: core-support
summary: special permissions for the core snap
plugs:
- core:core-support-plug
- system:core-support-plug
slots:
- core
- system