Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Caps type attrs #205
Conversation
zyga
added some commits
Dec 1, 2015
zyga
referenced this pull request
Dec 2, 2015
Merged
Replace caps.FileType with caps.BoolFile with required "path" #206
mvo5
reviewed
Dec 2, 2015
| - if c.Attrs != nil && len(c.Attrs) != 0 { | ||
| - return fmt.Errorf("attributes must be empty for now") | ||
| + // Check that all supported attributes are present | ||
| + for _, attr := range t.SupportedAttrs { |
mvo5
Dec 2, 2015
Collaborator
Hm, if I read this correctly it will fail if a "SupportedAttr" is missing? If thats the case, maybe it should be "t.RequiredAttrs" instead as the name? To make more clear that this is a mandatory attribute?
zyga
Dec 2, 2015
Contributor
I was actually looking for "mandatory/required + only (no other allowed).
I'll rename this to RequiredAttrs for now
zyga
added some commits
Dec 2, 2015
mvo5
reviewed
Dec 2, 2015
| @@ -31,6 +31,9 @@ type Type struct { | ||
| // Name is a key that identifies the capability type. It must be unique | ||
| // within the whole OS. The name forms a part of the stable system API. | ||
| Name string | ||
| + // RequiredAttrs contains names of attributes that are understood by |
mvo5
Dec 2, 2015
Collaborator
In nitpick mode now (sorry for that!). Maybe instead of "that are understood by" "that are required by" ?
mvo5
reviewed
Dec 2, 2015
| - // show that this code is actually being used. | ||
| - if c.Attrs != nil && len(c.Attrs) != 0 { | ||
| - return fmt.Errorf("attributes must be empty for now") | ||
| + // Check that all supported attributes are present |
mvo5
reviewed
Dec 2, 2015
| + // Look for any unexpected attributes | ||
| + if len(t.RequiredAttrs) != len(c.Attrs) { | ||
| + for attr := range c.Attrs { | ||
| + supported := false |
mvo5
Dec 2, 2015
Collaborator
(nitpick, probably over the top this time). Its a bit unfortunate that there is no "Contains()" outside of the testutil package :) I think this would be easier to read by:
for attr := range c.Attrs {
if !Contains(t.RequiredAttrs, attr) {
return fmt.Errorf("capability contains unexpected attribute %q", attr)
}
But not for this branch :)
|
Thanks, looks good to me. |
chipaca
reviewed
Dec 2, 2015
| + } | ||
| + } | ||
| + if !supported { | ||
| + return fmt.Errorf("capability contains unexpected attribute %q", attr) |
chipaca
Dec 2, 2015
Member
I think you should wait until the end of the loop and report all unexpected attribtues, not just the first one at random.
chipaca
Dec 2, 2015
Member
you've got to be <---------- this ----------> nitpicky to follow this suggestion
|
lgtm |
niemeyer
reviewed
Dec 2, 2015
| + // Check that all required attributes are present | ||
| + for _, attr := range t.RequiredAttrs { | ||
| + if _, ok := c.Attrs[attr]; !ok { | ||
| + return fmt.Errorf("capability lacks required attribute %q", attr) |
niemeyer
reviewed
Dec 2, 2015
| + return fmt.Errorf("capability lacks required attribute %q", attr) | ||
| + } | ||
| + } | ||
| + // Look for any unexpected attributes |
niemeyer
Dec 2, 2015
Contributor
This will break forwards compatibility in a really bad way. Think in terms of code: imagine you have a Python type that millions of people are using, and then when you decide to add a new method on that type, you break every single existing application out there.
zyga
Dec 2, 2015
Contributor
Can you explain this more please? I've made it precisely so that we have forward compatibility. Otherwise anyone can ship a product with a "foo" type that has a custom, unexpected "bar" attribute that we want to use but now cannot because shipping products and snaps have already used it.
Down the line I have an idea for type versions so that we can release, say, "bool-file" v2 with more attributes and applications that want to see that can use the new attributes (perhaps with new usage semantics as well, that are not strictly encoded in attributes but this is unrelated).
When we ship a type we ship it with fixed set of attributes. Each time we want to change what they are, what they mean, we bump the version and ship another capability (version) that represents the same resource. This gives us future-compatibility by letting us evolve types over time.
Obviously you had something else on your mind so please explain what you mean again and I'll adjust the code.
niemeyer
reviewed
Dec 2, 2015
| + Name: "name", | ||
| + Label: "label", | ||
| + Type: t, | ||
| + Attrs: map[string]string{}, |
niemeyer
Dec 2, 2015
Contributor
Why do we need an empty map here? The logic should handle nil maps okay, right?
There should also be another test with a non-empty map but non-matching attributes, and also another positive test which has both matching and non-matching attributes but the required attributes satisfied.
zyga
Dec 2, 2015
Contributor
No need, removed now.
The second part is fixed with the exception that the last test you've mentioned is not implemented (since I didn't change Validate() yet as I need to wait for your explanation).
zyga
added some commits
Dec 2, 2015
|
Just for record keeping, this still has some issues to address. |
|
I think this is fixed now |
|
@niemeyer AFAICT everything was addressed; landing this now (if we missed anything, there's plenty of branches in the pipeline where we can address whatever it was). |
zyga commentedDec 2, 2015
This branch adds simple attribute validation to capability types. Each type now contains the list of attributes that must (and only those) be present in a capability.