Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
repo: ConnectedPlug and ConnectedSlot types #4108
Conversation
stolowski
added some commits
Oct 11, 2017
Some checks were not successful
Some checks were not successful
| + | ||
| +type ConnectedPlug struct { | ||
| + dynamicAttrs map[string]interface{} | ||
| + plugInfo *snap.PlugInfo |
| +type ConnectedSlot struct { | ||
| + dynamicAttrs map[string]interface{} | ||
| + slotInfo *snap.SlotInfo | ||
| +} |
| +} | ||
| + | ||
| +func (plug *ConnectedPlug) StaticAttrs() map[string]interface{} { | ||
| + return plug.plugInfo.Attrs |
| + if plug.dynamicAttrs == nil { | ||
| + return nil, fmt.Errorf("dynamic attributes not initialized") | ||
| + } | ||
| + return plug.dynamicAttrs, nil |
pedronis
Nov 2, 2017
Contributor
it's a bit strange that Attr fallsback to static attrs but this doesn't
| + return slot.slotInfo.Snap | ||
| +} | ||
| + | ||
| +func (slot *ConnectedSlot) Apps() map[string]*snap.AppInfo { |
pedronis
Nov 2, 2017
Contributor
missing Hooks ? or we don't use them on slots? do we use Apps on slots either?
niemeyer
Nov 10, 2017
Contributor
missing Hooks ? or we don't use them on slots? do we use Apps on slots either?
Yeah, we should have it, both for consistency and because it makes sense to allow access to the resources that the slot grants.
| +} | ||
| + | ||
| +func (slot *ConnectedSlot) StaticAttrs() map[string]interface{} { | ||
| + return slot.slotInfo.Attrs |
Some checks were not successful
| + } | ||
| + return mp, nil | ||
| + default: | ||
| + return nil, fmt.Errorf("unsupported attribute type '%T', value '%v'", value, value) |
| + return cpy.(map[string]interface{}), err | ||
| +} | ||
| + | ||
| +func copyRecursive(value interface{}) (interface{}, error) { |
zyga
Nov 2, 2017
Contributor
Can we have a test with snap.yaml that has ~10KB of {{{{...}}}} dict to see what happens here? Are we going to exhaust golang stack?
pedronis
Nov 2, 2017
•
Edited 1 time
-
pedronis
Nov 2, 2017
Contributor
-
Nov 2, 2017
we already have recursive code in snap/info_snap_yaml.go so that's probrably a concern there but not here.
About all the copying, sorry, my comment was a genuine question, in this full form it feels a bit overkill, is just a bit strange to hide the full map though in a private field but then have an accessor that gives back all of it without even a shallow copy.
I think an important thing I don't understand here is what will typically use Attrs() ? and what are we going to need to do for dotted/nested attributes with Set* etc?
pedronis
Nov 2, 2017
Contributor
I remembered though we do have a precedent for this kind of copying in accessors, in assertionBase.Header and assertionBase.Headers
stolowski
Nov 3, 2017
Contributor
This API will be used by all interfaces. The point of returning a deep copy of static attributes is valid I think, it's desirable that interfaces code doesn't alter static attributes when it's not supposed to (in *ConnectedPlug/Slot methods).
Regarding dotted/nested attributes, these are handled in snapctl set and at that level we are just internally operating on two dictonaries and not using this API. Not sure if that answers your question?
| + return plug.dynamicAttrs, nil | ||
| + } | ||
| + return plug.StaticAttrs() | ||
| +} |
pedronis
Nov 3, 2017
Contributor
the definition of this is still strange, do we know how is going to be used? can be leave it out until we have a use case otherwise?
stolowski
Nov 9, 2017
•
Edited 1 time
-
stolowski
Nov 9, 2017
Contributor
-
Nov 9, 2017
Ok, changed for now to only return dynamic attrs until I see clear use case. I expect this to be used only by interfaces code if for some reasn it's more convienient to get all attributes at once.
stolowski
added some commits
Nov 9, 2017
stolowski
requested a review
from
niemeyer
Nov 9, 2017
| + "github.com/snapcore/snapd/snap" | ||
| +) | ||
| + | ||
| +type Connection struct { |
| +} | ||
| + | ||
| +func NewConnectedSlot(slot *snap.SlotInfo, dynamicAttrs map[string]interface{}) (*ConnectedSlot, error) { | ||
| + return &ConnectedSlot{ |
| +} | ||
| + | ||
| +func (plug *ConnectedPlug) StaticAttr(key string) (interface{}, error) { | ||
| + if val, ok := plug.plugInfo.Attrs[key]; ok { |
| +} | ||
| + | ||
| +func (plug *ConnectedPlug) Attr(key string) (interface{}, error) { | ||
| + if plug.dynamicAttrs != nil { |
zyga
Nov 10, 2017
Contributor
So dynamic vars always overrides the static attr? I'm mostly asking, not sure how you planned this to work (I came here naively assuming we'll get dynamic and fall back to static).
stolowski
Nov 13, 2017
Contributor
No, we don't allow dynamic arrs to override static ones. This method is just a getter with a fallback to lookup static attribute if there is no such static attr.
| + return nil, fmt.Errorf("dynamic attributes not initialized") | ||
| +} | ||
| + | ||
| +func (plug *ConnectedPlug) SetAttr(key string, value interface{}) error { |
stolowski
Nov 13, 2017
Contributor
I don't think we need locks. This API will be used by interfaces, as such the operations will be serialized, and we should prevent conflicting tasks from running in parallel.
| + | ||
| +func (plug *ConnectedPlug) SetAttr(key string, value interface{}) error { | ||
| + if plug.dynamicAttrs == nil { | ||
| + plug.dynamicAttrs = make(map[string]interface{}) |
stolowski
Nov 13, 2017
Contributor
I've modified the code now to make a copy of static attributes upfront when new connected plug/slot is created.
niemeyer
requested changes
Nov 10, 2017
API looks quite nice, thanks. A few notes, but they are all about two many issues I think: typing and errors.
| + if plug.dynamicAttrs != nil { | ||
| + return plug.dynamicAttrs, nil | ||
| + } | ||
| + return nil, fmt.Errorf("dynamic attributes not initialized") |
niemeyer
Nov 10, 2017
Contributor
It's not clear how this error can reasonably show up. If we have a ConnectedPlug at hand, we should have dynamic attributes because by definition we have a plug connected.
The message is not very user friendly, but it sounds like this should be made into an impossible situation instead.
| + plug.dynamicAttrs = make(map[string]interface{}) | ||
| + } | ||
| + if _, ok := plug.plugInfo.Attrs[key]; ok { | ||
| + return fmt.Errorf("attribute %q cannot be overwritten", key) |
niemeyer
Nov 10, 2017
Contributor
This error makes it sound like this attribute may never change, but that's not true. The real issue is that it was previously defined in a static way. Perhaps something like:
"cannot change attribute %q as it was statically specified in the snap details"
| + return slot.slotInfo.Snap | ||
| +} | ||
| + | ||
| +func (slot *ConnectedSlot) Apps() map[string]*snap.AppInfo { |
pedronis
Nov 2, 2017
Contributor
missing Hooks ? or we don't use them on slots? do we use Apps on slots either?
niemeyer
Nov 10, 2017
Contributor
missing Hooks ? or we don't use them on slots? do we use Apps on slots either?
Yeah, we should have it, both for consistency and because it makes sense to allow access to the resources that the slot grants.
| + if slot.dynamicAttrs != nil { | ||
| + return slot.dynamicAttrs, nil | ||
| + } | ||
| + return nil, fmt.Errorf("dynamic attributes not initialized") |
| + slot.dynamicAttrs = make(map[string]interface{}) | ||
| + } | ||
| + if _, ok := slot.slotInfo.Attrs[key]; ok { | ||
| + return fmt.Errorf("attribute %q cannot be overwritten", key) |
| + return v, nil | ||
| + case int: | ||
| + return int64(v), nil | ||
| + case int64: |
niemeyer
Nov 10, 2017
Contributor
The typed customization here feels a bit strange. This must have been cleaned up on the way in, by whatever changed these values. We shouldn't have to reparse it here. The goal of this is only to copy it over to prevent mutation, and as such we don't need to know the individual types unless the type is mutable, which is only the case for the two entries below. All the above can be in default.
We also need a comment in the code that does prevent other types from going in that this code must be changed if new mutable types are introduced. The lack of synchronicity will create very unfortunate runtime issues, that we'll only know about from users when they decide to use it in a particular interface.
pedronis
Nov 10, 2017
Contributor
yes, this seems to come from the code we have in snap/info_snap_yaml.go to indeed normalize values, but at this point they should be normalized
| + "b": true, | ||
| + "c": int(100), | ||
| + "d": []interface{}{"x", "y", true}, | ||
| + "e": map[string]interface{}{ |
niemeyer
Nov 10, 2017
Contributor
This can be in a single line as well (same as the one right above it).
| + cpy, err := CopyAttributes(orig) | ||
| + c.Assert(err, IsNil) | ||
| + // verify that int is converted into int64 | ||
| + c.Check(reflect.TypeOf(cpy["c"]).Kind(), Equals, reflect.Int64) |
niemeyer
Nov 10, 2017
Contributor
Similarly, that feels strange. It feels like a very awkward thing to be doing on the copy operation. The value shouldn't reach that internal dictionary with a wrong type, and it should break at set time rather than penalizing those reading the broken value.
niemeyer
Nov 12, 2017
Contributor
Break or be converted. In this particular case (int => int64) automatically converting on set sounds better.
stolowski
added some commits
Nov 13, 2017
codecov-io
commented
Nov 13, 2017
•
Edited 10 times
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 23, 2017
-
codecov-io
Nov 22, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
Nov 23, 2017 -
Nov 23, 2017 -
Nov 23, 2017 -
Nov 23, 2017 -
Nov 23, 2017 -
Nov 23, 2017 -
Nov 23, 2017 -
Nov 22, 2017 -
Nov 16, 2017 -
Nov 16, 2017
Codecov Report
@@ Coverage Diff @@
## master #4108 +/- ##
==========================================
+ Coverage 75.87% 75.94% +0.07%
==========================================
Files 440 443 +3
Lines 38295 38577 +282
==========================================
+ Hits 29055 29296 +241
- Misses 7224 7264 +40
- Partials 2016 2017 +1
Continue to review full report at Codecov.
|
| +) | ||
| + | ||
| +// Connection represents a connection between ConnectedPlug and ConnectedSlot | ||
| +type Connection struct { |
stolowski
Nov 13, 2017
Contributor
Not yet (it's only used internally in repo in the followup PR), but it will most likely be returned when repo grows getter method(s) to get connections.
zyga
Nov 23, 2017
Contributor
I'd say Connection represents connection between a particular plug and slot.
| +} | ||
| + | ||
| +// Attrs returns all dynamic attributes of this plug. | ||
| +func (plug *ConnectedPlug) Attrs() map[string]interface{} { |
pedronis
Nov 13, 2017
Contributor
should this be called DynamicAttrs given that it returns only the dynamic attrs?
niemeyer
Nov 23, 2017
Contributor
I can't see a good use case for a dynamic map attribute, and I agree it's confusing that this is called Attrs but returning partial data and unlike Attr itself. When do we ever care about looking just at dynamic attributes? It feels like if we have such a method, we'll use this and make an error since we'll assume the map is complete.
I suggest dropping the method for now and waiting for the use case.
| +} | ||
| + | ||
| +// StaticAttrs returns all static attributes. | ||
| +func (slot *ConnectedSlot) StaticAttrs() map[string]interface{} { |
stolowski
added some commits
Nov 15, 2017
| +) | ||
| + | ||
| +// Connection represents a connection between ConnectedPlug and ConnectedSlot | ||
| +type Connection struct { |
stolowski
Nov 13, 2017
Contributor
Not yet (it's only used internally in repo in the followup PR), but it will most likely be returned when repo grows getter method(s) to get connections.
zyga
Nov 23, 2017
Contributor
I'd say Connection represents connection between a particular plug and slot.
| + | ||
| +// Apps returns all the apps associated with this plug. | ||
| +func (plug *ConnectedPlug) Apps() map[string]*snap.AppInfo { | ||
| + return plug.plugInfo.Apps |
zyga
Nov 23, 2017
Contributor
Perhaps ok but I worry about apps being modified because the caller got them through this method.
| + | ||
| +// Hooks returns all the hooks associated with this plug. | ||
| +func (plug *ConnectedPlug) Hooks() map[string]*snap.HookInfo { | ||
| + return plug.plugInfo.Hooks |
| + | ||
| +// StaticAttrs returns all static attributes. | ||
| +func (plug *ConnectedPlug) StaticAttrs() map[string]interface{} { | ||
| + return plug.staticAttrs |
stolowski
Nov 23, 2017
Contributor
The constructor of ConnectedPlug/Slot creates a deep copy of static attributes, so modifications done directly to the map returned by this method will only affect this instance. Which would be nice to avoid, yes. But I'm not sure if we want a deep copy every time we return the map here?
| +} | ||
| + | ||
| +// Attrs returns all dynamic attributes of this plug. | ||
| +func (plug *ConnectedPlug) Attrs() map[string]interface{} { |
pedronis
Nov 13, 2017
Contributor
should this be called DynamicAttrs given that it returns only the dynamic attrs?
niemeyer
Nov 23, 2017
Contributor
I can't see a good use case for a dynamic map attribute, and I agree it's confusing that this is called Attrs but returning partial data and unlike Attr itself. When do we ever care about looking just at dynamic attributes? It feels like if we have such a method, we'll use this and make an error since we'll assume the map is complete.
I suggest dropping the method for now and waiting for the use case.
| + // note: ensure all the mutable types (or types that need a conversion) | ||
| + // are handled here. | ||
| + switch v := value.(type) { | ||
| + case int: |
zyga
Nov 23, 2017
Contributor
The int and float cases probably deserve a comment about why this is done. The function seems to conflate JSON support and recursive copying.
| +} | ||
| + | ||
| +// Attrs returns all dynamic attributes of this plug. | ||
| +func (plug *ConnectedPlug) Attrs() map[string]interface{} { |
pedronis
Nov 13, 2017
Contributor
should this be called DynamicAttrs given that it returns only the dynamic attrs?
niemeyer
Nov 23, 2017
Contributor
I can't see a good use case for a dynamic map attribute, and I agree it's confusing that this is called Attrs but returning partial data and unlike Attr itself. When do we ever care about looking just at dynamic attributes? It feels like if we have such a method, we'll use this and make an error since we'll assume the map is complete.
I suggest dropping the method for now and waiting for the use case.
| +} | ||
| + | ||
| +// Attrs returns all dynamic attributes of this slot. | ||
| +func (slot *ConnectedSlot) Attrs() map[string]interface{} { |
| + case int: | ||
| + return int64(v) | ||
| + case float32: | ||
| + return float64(v) |
pedronis
Nov 23, 2017
Contributor
afaiu, now copyRecursive is used in SetAttr as well, so it helps maintaining the invariant setup in snap/info_snap_yaml.go that attributes don't contain anything but int64 and float64 for new values being set. Without this kind of code in some form, interfaces themselves cannot continue to assume that to be the case.
stolowski
Nov 24, 2017
Contributor
This code has been changed to make just a recursive copy of the static attirbutes, and normalize the dynamic attributes in the constructor and in SetAttr methods.
stolowski
added some commits
Nov 23, 2017
| + | ||
| + val, err = slot.Attr("number") | ||
| + c.Assert(err, IsNil) | ||
| + c.Assert(reflect.TypeOf(val).Kind(), Equals, reflect.Int64) |
niemeyer
Nov 24, 2017
Contributor
For the record, this check is unnecessary given the following line. Equals takes the type into account as well. Same below.
stolowski commentedOct 31, 2017
•
Edited 1 time
-
stolowski
Oct 31, 2017
Add new data types for connected plugs/slots. Note: depends on #4013.
The is the implementation of item 5 of the plan https://forum.snapcraft.io/t/preparing-the-interfaces-logic-for-connection-hooks/2184.
The repository will use ConnectedSlot/ConnectedPlug for plugs/slots that are connected (PlugInfo/SlotInfo are used by the repository for all plugs/slots on the system). The new types will also be passed to Add*ConnectedPlug|Slot(...) methods of interfaces (this will come in the followup branch).