Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
[WIP] interfaces/hooks: expose attrs to the interface API, snapctl enhancements (step #4) #3120
Conversation
stolowski
added some commits
Mar 28, 2017
Some checks were not successful
All checks have failed
All checks have failed
stolowski
changed the title from
[WIP] interfaces: expose attrs to the interface API, snapctl enhancements (step #4)
to
interfaces: expose attrs to the interface API, snapctl enhancements (step #4)
Mar 31, 2017
stolowski
changed the title from
interfaces: expose attrs to the interface API, snapctl enhancements (step #4)
to
interfaces/hooks: expose attrs to the interface API, snapctl enhancements (step #4)
Mar 31, 2017
stolowski
added some commits
Mar 31, 2017
Some checks were not successful
All checks have failed
All checks have failed
Some checks were not successful
All checks have failed
stolowski
referenced this pull request
Mar 31, 2017
Merged
interfaces: API additions for interface hooks #3119
stolowski
and others
added some commits
Apr 3, 2017
Some checks were not successful
All checks have failed
All checks have passed
All checks have failed
stolowski
requested a review
from
niemeyer
Apr 12, 2017
stolowski
added some commits
Apr 12, 2017
All checks have failed
All checks have failed
All checks have failed
Some checks were not successful
All checks have failed
All checks have passed
All checks have passed
All checks have failed
All checks have failed
All checks have failed
Some checks were not successful
All checks have failed
Some checks were not successful
All checks have failed
Some checks were not successful
All checks have failed
| BaseDeclaration *asserts.BaseDeclaration | ||
| } | ||
| func (connc *ConnectCandidate) plugAttrs() map[string]interface{} { | ||
| - return connc.Plug.Attrs | ||
| + return connc.PlugAttrs |
pedronis
Apr 26, 2017
•
Edited 1 time
-
pedronis
Apr 26, 2017
Contributor
-
Apr 26, 2017
ConnectCandidate is also meant to write easily tests about the correctness of decls, so I think I would prefer this to have a fallback:
if connc.PlugAttrs != nil {
return connc.PlugAttrs
}
return connc.Plug.Attrs
and then have in policy just a couple of tests that test the new paths
pedronis
Apr 26, 2017
Contributor
there's just a couple of places where is used for real in ifacestate so the risk of getting it wrong there assuming good tests there should not be big, either way we would need test to check that those fields are set because if they are left empty we get would get wrong behavior either way
pedronis
Apr 27, 2017
Contributor
as I said in the standup with the this change, while we'll need to test the new paths, we can undo the changes to many tests, at least most in policy itself and basedeclaration
| } | ||
| func (connc *ConnectCandidate) slotAttrs() map[string]interface{} { | ||
| - return connc.Slot.Attrs | ||
| + return connc.SlotAttrs |
stolowski
added some commits
Apr 27, 2017
All checks have failed
| @@ -62,6 +62,8 @@ plugs: | ||
| return &policy.ConnectCandidate{ | ||
| Plug: plugSnap.Plugs[iface], | ||
| Slot: slotSnap.Slots[iface], | ||
| + PlugAttrs: plugSnap.Plugs[iface].Attrs, | ||
| + SlotAttrs: slotSnap.Slots[iface].Attrs, |
stolowski
added some commits
Apr 28, 2017
All checks have failed
All checks have passed
All checks have failed
added a commit
that referenced
this pull request
May 10, 2017
All checks have failed
|
@stolowski the tests are failing on related code, please have a look |
|
Yes, I'm aware of the test failure; this spread test is abusing content interface and needs some reworking now. |
| @@ -29,6 +29,12 @@ import ( | ||
| "github.com/snapcore/snapd/snap" | ||
| ) | ||
| +// InterfaceAttrs is a container for plug and slot attributes of given connection | ||
| +type InterfaceAttrs struct { |
| @@ -42,17 +48,21 @@ type Repository struct { | ||
| // given a plug and a slot, are they connected? | ||
| plugSlots map[*Plug]map[*Slot]bool | ||
| backends map[SecuritySystem]SecurityBackend | ||
| + // attributes of plugs and slots; the attributes include attribute values from | ||
| + // the yaml and provided at runtime via interface hooks. | ||
| + attributes map[*Plug]map[*Slot]*InterfaceAttrs |
zyga
May 16, 2017
Contributor
Just looking at the layout of this. I wonder if we should be following the slotPlugs and plugSlots so that we can look up information about a connection from either (plug, slot) and (slot, plug) equally. We could actually store a connectionInfo or whatever there so that we could host attributes and know this connection exists in the first place. We already have code managing the two attributes there and we might just re-purpose them for storing connection state.
| @@ -438,7 +448,7 @@ func (r *Repository) ResolveDisconnect(plugSnapName, plugName, slotSnapName, slo | ||
| // Connect establishes a connection between a plug and a slot. | ||
| // The plug and the slot must have the same interface. | ||
| -func (r *Repository) Connect(ref ConnRef) error { | ||
| +func (r *Repository) Connect(ref ConnRef, plugAttrs map[string]interface{}, slotAttrs map[string]interface{}) error { |
pedronis
May 17, 2017
Contributor
shouldn't this or somewhere call Sanitize* on the dynamic attrs, that's the place that can set default value afaiu
also we should remember the issue of not passing nil to methods that can write on attrs
| + type validateSlot interface { | ||
| + ValidateSlot(slot *interfaces.Slot, slotAttrs map[string]interface{}) error | ||
| + } | ||
| + if validate, ok := iface.(validatePlug); ok { |
pedronis
May 17, 2017
Contributor
it's strange to do this here, it seems repo should grow some ValidateConnection or something like that
| +func getTaskHookAttributes(task *state.Task) (attrs interfaces.InterfaceAttrs, err error) { | ||
| + attrs.PlugAttrs = make(map[string]interface{}) | ||
| + attrs.SlotAttrs = make(map[string]interface{}) | ||
| + if err = task.Get("plug-attrs", &attrs.PlugAttrs); err == nil { |
pedronis
May 18, 2017
Contributor
does plug-attrs contains the original attrs from the yaml + what the hook changed? or just new things? the extra in the arg name to Validate* would make think the latter, but if that's the case the connection check is wrong, that one needs the updated full set or we need to tweak the code in policy a bit differently
stolowski
added some commits
May 24, 2017
All checks have failed
| @@ -81,15 +81,26 @@ func checkOnClassic(c *asserts.OnClassicConstraint) error { | ||
| } | ||
| func checkPlugConnectionConstraints1(connc *ConnectCandidate, cstrs *asserts.PlugConnectionConstraints) error { |
pedronis
May 25, 2017
•
Edited 1 time
-
pedronis
May 25, 2017
Contributor
-
May 25, 2017
I don't think we really want to change this code like this, way really need to check the final set of attributes, merged as it makes sense, the rules are about attributes as whole, so checking subsets separately doesn't have the same meaning
stolowski
added some commits
Jun 7, 2017
|
What's the state of this PR? |
chipaca
added
the
Decaying
label
Jul 10, 2017
|
@chipaca it's WIP after the recent discussions. |
stolowski
changed the title from
interfaces/hooks: expose attrs to the interface API, snapctl enhancements (step #4)
to
[WIP] interfaces/hooks: expose attrs to the interface API, snapctl enhancements (step #4)
Jul 10, 2017
stolowski
added some commits
Jul 11, 2017
All checks have failed
All checks have failed
All checks have failed
chipaca
removed
the
Decaying
label
Aug 25, 2017
|
removed the 'Decaying' label as you're obviously keeping it up to date. Nevertheless if you're not going to be able to fix those tests soon I'd suggest you close this so we don't get used to seeing it on the queue before it's reviewable again. |
stolowski
added some commits
Aug 25, 2017
All checks have passed
All checks have passed
stolowski
referenced this pull request
Aug 25, 2017
Closed
interfaces/hooks: PlugData and SlotData wrappers #3810
codecov-io
commented
Aug 25, 2017
•
Edited 1 time
-
codecov-io
Aug 25, 2017
-
Aug 25, 2017
Codecov Report
@@ Coverage Diff @@
## master #3120 +/- ##
==========================================
- Coverage 75.82% 75.76% -0.06%
==========================================
Files 402 403 +1
Lines 34793 35170 +377
==========================================
+ Hits 26381 26648 +267
- Misses 6539 6631 +92
- Partials 1873 1891 +18
Continue to review full report at Codecov.
|
|
@stolowski I'm closing this as it has plenty of reviews (unclear which ones are done), it says [WIP] in the summary still, it needs conflict solving, and it has a very dirty history:
Please open a new one once it's ready for more attention. |
stolowski commentedMar 31, 2017
•
Edited 1 time
-
stolowski
May 25, 2017
Note: #3810 has the new API in separate PR and should be reviewed first (it will reduce the size of this diff a bit once landed).