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.

Sign up for GitHub

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

Jump to bottom

interfaces/builtin: refine the content interface rules using $SLOT #2712

Merged
merged 4 commits into from
Jan 30, 2017
Merged

interfaces/builtin: refine the content interface rules using $SLOT #2712

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
15f6574
improve the content base declarations rules using $SLOT and $MISSING
pedronis Jan 25, 2017
9535c01
after discussion, content label attribute was always meant to be mand…
pedronis Jan 25, 2017
826a779
Merge remote-tracking branch 'upstream/master' into iface-content-rules
pedronis Jan 30, 2017
7700ee8
a bit more readable
pedronis Jan 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 21 additions & 2 deletions interfaces/builtin/basedeclaration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,28 @@ slots:
slot-snap-type:
- app
- gadget
allow-connection:
-
plug-attributes:
content: $SLOT(content)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meaning to say that the connection should be allowed if the content attribute of the plug matches the content attribute of the slot? It seems so, but want to make doubly sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

-
plug-attributes:
content: $MISSING
slot-attributes:
content: $MISSING
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. that doesn't seem right. What would be a content interface with a missing "content" field? I thought the $MISSING case was for a future need.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is meant to address the 'default' from https://github.com/snapcore/snapd/wiki/Interfaces#content?

  • content (slot): reference to plug side of connection. Defaults to local slot name
  • content (plug): reference to slot side of connection. Defaults to local plug name

That said, it seems that the first alternation addresses the default case just fine, unless I am missing something:

plug-attributes:
    content: $SLOT(content)

allow-auto-connection:
plug-publisher-id:
- $SLOT_PUBLISHER_ID
-
plug-publisher-id:
- $SLOT_PUBLISHER_ID
plug-attributes:
content: $SLOT(content)
-
plug-publisher-id:
- $SLOT_PUBLISHER_ID
plug-attributes:
content: $MISSING
slot-attributes:
content: $MISSING
cups-control:
allow-installation:
slot-snap-type:
Expand Down
152 changes: 152 additions & 0 deletions interfaces/builtin/basedeclaration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,78 @@ func (s *baseDeclSuite) TestAutoConnectionContent(c *C) {
cand := s.connectCand(c, "content", "", "")
err := cand.CheckAutoConnect()
c.Check(err, NotNil)

slotDecl1 := s.mockSnapDecl(c, "slot-snap", "slot-snap-id", "pub1", "")
plugDecl1 := s.mockSnapDecl(c, "plug-snap", "plug-snap-id", "pub1", "")
plugDecl2 := s.mockSnapDecl(c, "plug-snap", "plug-snap-id", "pub2", "")

// same publisher, same content
cand = s.connectCand(c, "stuff", `name: slot-snap
slots:
stuff:
interface: content
content: mk1`, `name: plug-snap
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really weird to my eye (the backticks for the two strings). If this is valid style, fine, but it seems it could be more clearly written.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a valid style, and we use it here and there already

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with jdstrand.. this is super hard to read properly. Can we please make it:

cand = s.connectCand(c, "stuff", `
name: slot-snap
...
`, `
name: plug-snap
...
`)

plugs:
stuff:
interface: content
content: mk1
`)
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl1
err = cand.CheckAutoConnect()
c.Check(err, IsNil)

// different publisher, same content
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl2
err = cand.CheckAutoConnect()
c.Check(err, NotNil)

// same publisher, different content
tests := []struct {
slotContent string
plugContent string
}{
{"mk1", "mk2"},
{"mk1", ""},
{"", "mk1"},
}
for _, t := range tests {
slotSnippet := ""
if t.slotContent != "" {
slotSnippet = "content: " + t.slotContent
}
plugSnippet := ""
if t.plugContent != "" {
plugSnippet = "content: " + t.plugContent
}
cand := s.connectCand(c, "stuff", fmt.Sprintf(`name: slot-snap
slots:
stuff:
interface: content
%s`, slotSnippet), fmt.Sprintf(`name: plug-snap
plugs:
stuff:
interface: content
%s`, plugSnippet))
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl1
err = cand.CheckAutoConnect()
c.Check(err, NotNil)
}

// same publisher, no content
cand = s.connectCand(c, "stuff", `name: slot-snap
slots:
stuff:
interface: content`, `name: plug-snap
plugs:
stuff:
interface: content`)
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl1
err = cand.CheckAutoConnect()
c.Check(err, IsNil)
}

func (s *baseDeclSuite) TestAutoConnectionLxdSupportOverride(c *C) {
Expand Down Expand Up @@ -569,3 +641,83 @@ func (s *baseDeclSuite) TestSanity(c *C) {
}
}
}

func (s *baseDeclSuite) TestConnectionContent(c *C) {
// we let connect explicitly as long as content matches (or is absent on both sides)

cand := s.connectCand(c, "content", "", "")
err := cand.Check()
c.Check(err, IsNil)

slotDecl1 := s.mockSnapDecl(c, "slot-snap", "slot-snap-id", "pub1", "")
plugDecl1 := s.mockSnapDecl(c, "plug-snap", "plug-snap-id", "pub1", "")
plugDecl2 := s.mockSnapDecl(c, "plug-snap", "plug-snap-id", "pub2", "")

// same publisher, same content
cand = s.connectCand(c, "stuff", `name: slot-snap
slots:
stuff:
interface: content
content: mk1`, `name: plug-snap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here and below.

plugs:
stuff:
interface: content
content: mk1
`)
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl1
err = cand.Check()
c.Check(err, IsNil)

// different publisher, same content
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl2
err = cand.Check()
c.Check(err, IsNil)

// same publisher, different content
tests := []struct {
slotContent string
plugContent string
}{
{"mk1", "mk2"},
{"mk1", ""},
{"", "mk1"},
}
for _, t := range tests {
slotSnippet := ""
if t.slotContent != "" {
slotSnippet = "content: " + t.slotContent
}
plugSnippet := ""
if t.plugContent != "" {
plugSnippet = "content: " + t.plugContent
}
cand := s.connectCand(c, "stuff", fmt.Sprintf(`name: slot-snap
slots:
stuff:
interface: content
%s`, slotSnippet), fmt.Sprintf(`name: plug-snap
plugs:
stuff:
interface: content
%s`, plugSnippet))
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl1
err = cand.Check()
c.Check(err, NotNil)
}

// same publisher, no content
cand = s.connectCand(c, "stuff", `name: slot-snap
slots:
stuff:
interface: content`, `name: plug-snap
plugs:
stuff:
interface: content`)
cand.SlotSnapDeclaration = slotDecl1
cand.PlugSnapDeclaration = plugDecl1
err = cand.Check()
c.Check(err, IsNil)
}
3 changes: 2 additions & 1 deletion interfaces/builtin/content.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,5 +184,6 @@ func (iface *ContentInterface) PermanentPlugSnippet(plug *interfaces.Plug, secur
}

func (iface *ContentInterface) AutoConnect(plug *interfaces.Plug, slot *interfaces.Slot) bool {
return plug.Attrs["content"] == slot.Attrs["content"]
// allow what declarations allowed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/

return true
}