Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/hooks: PlugData and SlotData wrappers #3810
Conversation
Some checks were not successful
stolowski
referenced this pull request
Aug 25, 2017
Closed
[WIP] interfaces/hooks: expose attrs to the interface API, snapctl enhancements (step #4) #3120
codecov-io
commented
Aug 25, 2017
•
Edited 1 time
-
codecov-io
Sep 1, 2017
-
Sep 1, 2017
Codecov Report
@@ Coverage Diff @@
## master #3810 +/- ##
==========================================
- Coverage 75.73% 75.72% -0.02%
==========================================
Files 409 410 +1
Lines 35388 35464 +76
==========================================
+ Hits 26801 26854 +53
- Misses 6690 6711 +21
- Partials 1897 1899 +2
Continue to review full report at Codecov.
|
zyga
reviewed
Aug 25, 2017
Some early feedback, I probably don't know what you are going to do with this so some of my questions may make no sense.
| +} | ||
| + | ||
| +type SlotData struct { | ||
| + slot *snap.SlotInfo |
zyga
Aug 25, 2017
Contributor
Just think aloud, should this be a SlotInfo or is a SlotRef sufficient?
stolowski
Aug 25, 2017
Contributor
No, it wouldn't bu sufficient, I need access to slot's and plug's attributes.
| +} | ||
| + | ||
| +type Attributes interface { | ||
| + SetStaticAttr(key string, value interface{}) |
stolowski
Aug 25, 2017
Contributor
That's a good question. I need to think about it. Would make sense to fail if we wanted to prohibit setting these attributes after Sanitize* step (see the descripton of this PR).
| +} | ||
| + | ||
| +func (attrs *PlugData) Ref() PlugRef { | ||
| + return PlugRef{Snap: attrs.plug.Snap.Name(), Name: attrs.plug.Name} |
stolowski
Aug 25, 2017
Contributor
No.. For some reason we have Ref() on Plug and Slot, but not on PlugInfo/SlotInfo. Perhaps we could have that on both for convienience, in which case this can be simplified indeed?
| + | ||
| +func (attrs *PlugData) SetAttr(key string, value interface{}) error { | ||
| + if attrs.dynamicAttrs == nil { | ||
| + return fmt.Errorf("dynamic attributes not initialized") |
stolowski
Aug 25, 2017
•
Edited 1 time
-
stolowski
Aug 25, 2017
Contributor
-
Aug 25, 2017
We want it to fail if you attempt to set dynamic attribute before hooks are even run. This will be initialized after hooks are executed, so any AfterPreparePlug|Slot methods of interfaces will be able to call it without error.
pedronis
Aug 25, 2017
Contributor
my understanding is that this can be called also in the equivalent to Sanitize for setting defaults that is ok to override later
pedronis
Aug 28, 2017
Contributor
to be clear I think to implement that
plugs: make(map[string]map[string]*Plug),
slots: make(map[string]map[string]*Slot)
in repo needs to use *Data instead, then we would clone them before using them to make connections and then keep track of the final connection attributes with some object keeping those cloned/finalized *Data sides
| +} | ||
| + | ||
| +func (attrs *SlotData) Ref() SlotRef { | ||
| + return SlotRef{Snap: attrs.slot.Snap.Name(), Name: attrs.slot.Name} |
| + } | ||
| +} | ||
| + | ||
| +func (attrs *PlugData) Interface() string { |
mvo5
Aug 28, 2017
Collaborator
I am slightly unhappy about the boilerplate duplication between PlugData and SlotData, a lot of the helpers are identical except for attrs.{plug,slot}. I think by re-organizing the structs a bit we could avoid some of this duplication. Here is an outline of what I have in mind: http://paste.ubuntu.com/25416348/ - it avoids duplication but adds a bit of complexity (via the common base type but without losing type safety) so we need think about the trade-off here, i.e. it might not be worth it. With that refactor: Interface,Name,Snap,Apps,StaticAttr,SetStaticAttr,StaticAttrs,Attr,Attrs,SetAttr would all be common code. Only Ref and SecurityTags would be type specific.
mvo5
Aug 28, 2017
Collaborator
pushed to https://github.com/mvo5/snappy/tree/hook-attrs-refactor for easier cherry-pick (if we want to go down this path).
stolowski
and others
added some commits
Aug 29, 2017
Some checks were not successful
All checks have failed
All checks have passed
| - Interface: iface, | ||
| - Attrs: attrs, | ||
| - Label: label, | ||
| + PlugSlotData{ |
pedronis
Sep 5, 2017
Contributor
this kind of change looks a bit odd tbh, given that is all exported and not some internal detail
| +} | ||
| + | ||
| +type plugSlotData struct { | ||
| + data *snap.PlugSlotData |
pedronis
Sep 5, 2017
Contributor
sharing data with the original like this is probably not useful, misleading. As I wrote in one comment the point is more that the repo should be moved to be *Data based.
pedronis
Sep 5, 2017
•
Edited 1 time
-
pedronis
Sep 5, 2017
Contributor
-
Sep 5, 2017
we should treat the *Info stuff as immutable, and deal with repos/interfaces needing to do mutations by using copies, the new *Data stuff
|
Closing for now. |
stolowski commentedAug 25, 2017
Helper classes to access plug/slot attributes from implementations of interfaces - see https://forum.snapcraft.io/t/defining-the-behavior-of-dynamic-attributes-interface-hooks/673 for discussion.
They wrap plug/info with their "static" attributes as well as "dynamic" attributes coming from hooks - this is to simplify arguments of interface methods, avoid map[string]interface{} there and get better control over what gets modified and when (we could add checks and error out if SetStaticAttr is attempted after "Sanitize" step, but so far it was decided not to as this should be treated as programming error during the code review of an interface).
The branch that's actually using the new types is the #3120