Strict yaml validation

[joshwget]

Fixes #34. This makes use of xeipuuv/gojsonschema and gives error messages that match those in Compose for the most part.

This also reuses the same schemas as in Compose, with one small caveat. I removed the following line from service_schema.json:

{"$ref": "fields_schema.json#/definitions/service"},

This code always checks against fields_schema anyways, so the reference here would just add redundant validation. There's also no way in gojsonschema to handle references when schemas are provided as strings.

There are a few instances of awkward workarounds and manual parsing of the schema due to limitations in gojsonschema. These can be cleaned up once those issues are fixed.

Signed-off-by: Josh Curl hello@joshcurl.com

[ibuildthecloud]

@joshwget for the purpose of review can you separate into two commits, one for Godeps and the other for your code changes. Thanks.

[joshwget]

@ibuildthecloud Good idea. Updated!

[vdemeester]

@joshwget needs a rebase and a change (no more godep, now vendoring information should go in ./script/vendor.sh). Sorry 😝

[joshwget]

@vdemeester No problem! It should be up to date now.

[dnephin]

There's also no way in gojsonschema to handle references when schemas are provided as strings.

I don't understand this. Are you using that it doesn't support $ref ? How else would you specify a reference?

[dnephin]

I don't think these need to be package level vars, they're only used in one function (plus a setup function, which could be made to return these instead).

[ibuildthecloud]

@joshwget I think you can go one of two ways with this. Either call setupSchemaLoaders() from init() such that it's done once, or call setupSchemaLoaders() every time and not cache at the package level. Currently this code will suffer from (albeit unlikely) race conditions. I would prefer you call it from init(). From a server context I'd rather initialize this structure once.

Can you confirm that gojsonschema.JSONLoader can be used concurrently across goroutines?

[joshwget]

I hadn't actually considered this. I'll call it in init().

There's an issue about thread safety, but it hasn't received any response yet. The issue is about something different, but it's actually better to cache that than a JSONLoader.

[joshwget]

I don't understand this. Are you using that it doesn't support $ref ? How else would you specify a reference?

This is how you would specify a reference, but I don't think these still work when you inline the schemas in .go file. It look like other validation libraries allow you to override the source of a $ref to get around this, but I didn't see this kind of functionality in gojsonschema.

[dnephin]

Ah, I see what you mean, thanks.

[joshwget]

@dnephin @ibuildthecloud @vdemeester Updated to use go generate to inline schemas.

I updated the schemas to the latest from Compose at the same time, which required a few changes to how validation is done. Rather than a single validateSchema function, I split it into validateAgainstFieldsSchema and validateAgainstServiceSchema, which seems to be more in line with how Compose uses the schemas.

I'm still unsure whether gojsonschema.JSONLoader can be used across goroutines. It looks like the creator of gojsonschema is unsure too, so it might be safer to not cache schema loaders.

[joshwget]

@dnephin @ibuildthecloud @vdemeester Rebased and updated to the latest schema, as well as a few other minor changes

[vdemeester]

Looks good. I have to review it more in depth though 😛.

I wonder if the validate mechanism (project/schema_helper.go and project/schema_validate) would be better in a separate package (like a config package). This way we are sure only exported functions are called and thus it allows to separate a bit concerns.

I'm working on v2 support and I'm gonna work on a config package – to isolate parsing/validating configuration from the rest. So I could it then ;)

Needs a rebase again 😅.

[joshwget]

Rebased! I really like the idea of a config package. Interpolation should probably go in there as well. Maybe we can deal with #74 and this PR and then move to config all at once afterwards.

[vdemeester]

@joshwget Hum, you're right about the interpolation, could go to config too. Putting it in a package up-front makes it easier to refactor afterwards ; but let's have this in 0.2.0 and increment/refactor from this.

I'm not found of the go generate part as it might break a little bit the go get … workflow… I'm not sure what would be the alternative..

LGTM 🐮

/cc @dnephin @ibuildthecloud

[joshwget]

As an alternative to go generate, we could add schema.go to source control and manually run inline_schema.go whenever merging changes to the schema.

[vdemeester]

@joshwget I would prefer that 😄

[joshwget]

I definitely prefer that approach as well. Updated to remove generate.go and add schema.go.

[vdemeester]

@joshwget let's merge this and iterate over it 😝