asserts: introduce model assertion #460

Merged
merged 5 commits into from Feb 8, 2016

Conversation

Projects
None yet
4 participants
Contributor

pedronis commented Feb 6, 2016

No description provided.

pedronis added some commits Feb 6, 2016

+ listStr, ok := headers[name]
+ if !ok {
+ return nil, fmt.Errorf("%q header is mandatory", name)
+ }
@atomatt

atomatt Feb 8, 2016

Contributor

It may not be worth it, but would it make sense to split checkMandatory into two, e.g. checkMandatory & checkEmpty, so that checkMandatory only has one responsibility and can be reused here?

@pedronis

pedronis Feb 8, 2016

Contributor

@emgee well this is a checker itself, so reuse is not so interesting, also I don't think we want many fields that must be there but can be empty, lists are a bit of a special case, maybe we even want to leave them out when empty but not sure about that

Contributor

atomatt commented Feb 8, 2016

+1 with optional comment

Member

chipaca commented Feb 8, 2016

👍

+var modelMandatory = []string{"os", "architecture", "gadget", "kernel", "store", "class"}
+
+func assembleModel(assert assertionBase) (Assertion, error) {
+ if assert.headers["brand-id"] != assert.headers["authority-id"] {
@atomatt

atomatt Feb 8, 2016

Contributor

It seems there is no authority-id for model assertions - the brand-id takes its place.

@pedronis

pedronis Feb 8, 2016

Contributor

@emgee yea I discussed that with Gustavo, the library assumes there is always an authority-id and that's the signer until we get strong input to add the complexity of aliases for it etc

asserts/checkers.go
+ return nil, fmt.Errorf("%q header is mandatory", name)
+ }
+
+ // empty list are allowed
@stevenwilkin

stevenwilkin Feb 8, 2016

Member

s/list/lists/

+}
+
+// BrandID returns the brand identifier. Same as the authority id.
+func (mod *Model) BrandID() string {
@stevenwilkin

stevenwilkin Feb 8, 2016

Member

Perhaps m instead of mod for the method receiver?

+ return mod.allowedModes
+}
+
+// RequiredSnaps returns the snaps that must be installed at all times and cannot be removed for this model.
@stevenwilkin

stevenwilkin Feb 8, 2016

Member

Comment on line 86 wraps onto a new line but this doesn't

+ tsLine string
+}
+
+var (
@stevenwilkin

stevenwilkin Feb 8, 2016

Member

No need for multiline var (...) ?

@pedronis

pedronis Feb 8, 2016

Contributor

the coming branch needs it

@pedronis pedronis changed the title from assserts: introduce model assertion to asserts: introduce model assertion Feb 8, 2016

pedronis added a commit that referenced this pull request Feb 8, 2016

@pedronis pedronis merged commit d503dc0 into snapcore:master Feb 8, 2016

2 of 3 checks passed

Integration tests Started
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 69.551%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment