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

pedronis · 2017-01-25T13:47:09Z

Allow connection and auto-connection only if plug and slot side content attributes match, using a $SLOT constraint.

Make the content attribute of the content interface mandatory temporarily until we have the right infrastructure to implement the initially intended defaults. (The previous behaviour letting leaving it out was unintended/buggy)

pedronis · 2017-01-25T13:47:19Z

based on #2698

niemeyer · 2017-01-25T14:02:47Z

interfaces/builtin/basedeclaration.go

+        plug-attributes:
+          content: $MISSING
+        slot-attributes:
+          content: $MISSING


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.

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)

niemeyer · 2017-01-25T14:03:55Z

interfaces/builtin/content.go

@@ -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"]


…atory

jdstrand · 2017-01-25T21:55:46Z

asserts/ifacedecls.go

+}
+
+var (
+	validEvalAttrMatcher = regexp.MustCompile(`^\$(SLOT|PLUG)\((.+)\)$`)


In the snap declarations spec, there is no reference to $SLOT and $PLUG. Are these meant to replace $same and/or $slot_publisher_id and $plug_publisher_id?

we never implemented $same, we implemented $SLOT_PUBLISHER_ID and $PLUG_PUBLISHER_ID those are used though only in the *-publisher-id: constraints.

$SLOT(dotted attr) and $PLUG(dotted attr) are meant for plug/slot-attributes constraints to retrieve the corresponding value of the attribute on the SLOT or on the PLUG,
mostly to implement $same (but Gustavo thinks they could be used in other cases, more generically)

I see from 15f6574 this is meant to say the $SLOT(something) in plug-attributes must match the 'something' attribute of the plug for this snap and that the $PLUG(else) in slot-attributes must match the 'else' attribute from the slot for this snap.

Can you update the specification for $SLOT/$PLUG/$MISSING?

@jdstrand I'll try to make suggestions there, I don't have edit rights

@pedronis mentioned that a later commit removed $MISSING, so I'll ignore it.

jdstrand · 2017-01-25T21:57:51Z

asserts/asserts.go

-	maxSupportedFormat[SnapDeclarationType.Name] = 1
+
+	// 1: plugs and slots
+	// 2: support for $SLOT()/$PLUG()/$MISSING


What is $MISSING meant to be used for?

we don't at the moment have a way to express the constraints that an attribute is omitted, $MISSING constraints the attribute not to be set

So $MISSING is for stating that the attribute must not be set at all? What would be an example use case of that? auto-connect only if an attribute is not set? If so, is $MISSING the right word; would $DEFAULT be better?

jdstrand · 2017-01-25T22:01:45Z

interfaces/builtin/basedeclaration.go

+    allow-connection:
+      -
+        plug-attributes:
+          content: $SLOT(content)


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.

jdstrand · 2017-01-25T22:02:56Z

I was asked to take a look at this, but am having to come up to speed on context and piece together what this is meant to address. In the future if you can provide a description for the PR that details the problem and how the change addresses the problem?

pedronis · 2017-01-25T22:15:45Z

@jdstrand I added a description of the new constraints to #2698

jdstrand · 2017-01-25T22:16:26Z

interfaces/builtin/basedeclaration_test.go

+slots:
+  stuff:
+    interface: content
+    content: mk1`, `name: plug-snap


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.

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

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 ... `)

jdstrand

Need a clarification on $MISSING in the base declaration, but the allow-connection for content: $SLOT(content) for plug-attributes looks good.

jdstrand · 2017-01-25T22:32:42Z

interfaces/builtin/basedeclaration.go

    allow-auto-connection:
      plug-publisher-id:
        - $SLOT_PUBLISHER_ID
+      plug-attributes:
+        content: $SLOT(content)


The updated base declaration changes without $MISSING LGTM.

jdstrand · 2017-01-25T22:34:47Z

FYI, I updated https://github.com/snapcore/snapd/wiki/Interfaces#content to remove the text stating that the 'content' attribute for the 'content' interface has a default value.

niemeyer

One superficial comment, and @jdstrand is right. The default case was supposed to make the logic more pleasant to use, and we screwed up the original implementation by not making the default do the right thing.

Suggested route: let's enforce content to be present and match, moving this PR in. Then, let's fix attribute mutation in the interface hooks series of PRs, and then come back to reintroduce the defaults here, which shall be trivial by then.

niemeyer · 2017-01-30T14:11:44Z

interfaces/builtin/basedeclaration_test.go

+slots:
+  stuff:
+    interface: content
+    content: mk1`, `name: plug-snap


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 ... `)

niemeyer · 2017-01-30T14:12:47Z

interfaces/builtin/basedeclaration_test.go

+slots:
+  stuff:
+    interface: content
+    content: mk1`, `name: plug-snap


Same here and below.

jdstrand

Thanks for the cleanups. As mentioned before, the base declaration changes LGTM. The changes requiring that the 'content' attribute look good and are consistent with how the current implementation is supposed to work.

improve the content base declarations rules using $SLOT and $MISSING

15f6574

pedronis requested a review from jdstrand January 25, 2017 13:49

pedronis added this to the 2.22 milestone Jan 25, 2017

niemeyer reviewed Jan 25, 2017

View reviewed changes

after discussion, content label attribute was always meant to be mand…

9535c01

…atory

pedronis modified the milestones: 2.23, 2.22 Jan 25, 2017

jdstrand reviewed Jan 25, 2017

View reviewed changes

pedronis changed the title ~~interfaces/builtin: refine the content interface rules using $SLOT/$MISSING~~ interfaces/builtin: refine the content interface rules using $SLOT Jan 25, 2017

jdstrand reviewed Jan 25, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into iface-content-rules

826a779

niemeyer approved these changes Jan 30, 2017

View reviewed changes

a bit more readable

7700ee8

pedronis force-pushed the iface-content-rules branch from 6f12a08 to 7700ee8 Compare January 30, 2017 14:46

jdstrand approved these changes Jan 30, 2017

View reviewed changes

pedronis merged commit ee2506b into snapcore:master Jan 30, 2017

pedronis deleted the iface-content-rules branch January 31, 2017 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pedronis commented Jan 25, 2017 •

edited

pedronis commented Jan 25, 2017

niemeyer Jan 25, 2017

jdstrand Jan 25, 2017

niemeyer Jan 25, 2017

jdstrand Jan 25, 2017 •

edited by pedronis

pedronis Jan 25, 2017 •

edited

jdstrand Jan 25, 2017

jdstrand Jan 25, 2017

pedronis Jan 25, 2017

jdstrand Jan 25, 2017

jdstrand Jan 25, 2017

pedronis Jan 25, 2017

jdstrand Jan 25, 2017

jdstrand Jan 25, 2017

pedronis Jan 25, 2017

jdstrand commented Jan 25, 2017

pedronis commented Jan 25, 2017

jdstrand Jan 25, 2017

pedronis Jan 25, 2017

niemeyer Jan 30, 2017

jdstrand left a comment

jdstrand Jan 25, 2017

jdstrand commented Jan 25, 2017

niemeyer left a comment

niemeyer Jan 30, 2017

niemeyer Jan 30, 2017

jdstrand left a comment

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

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

Conversation

pedronis commented Jan 25, 2017 • edited

pedronis commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Jan 25, 2017 • edited by pedronis

Choose a reason for hiding this comment

pedronis Jan 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Jan 25, 2017

pedronis commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Jan 25, 2017

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

pedronis commented Jan 25, 2017 •

edited

jdstrand Jan 25, 2017 •

edited by pedronis

pedronis Jan 25, 2017 •

edited