Parse, validate, and load docker-compose v2.1 files #497
Conversation
|
Please sign your commits following these rules: $ git clone -b "441-refactor-v2_1" [email protected]:cheapRoc/libcompose.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354513824
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
Signed-off-by: Justin Reagor <[email protected]>
Signed-off-by: Justin Reagor <[email protected]>
Signed-off-by: Justin Reagor <[email protected]>
Signed-off-by: Justin Reagor <[email protected]>
Signed-off-by: Justin Reagor <[email protected]>
Signed-off-by: Justin Reagor <[email protected]>
|
I see the lints and probably borked tests... I'll touch up on that next time. |
|
|
||
| if config.Version != "2" { | ||
| parts := strings.Split(config.Version, ".") | ||
| major, _ := strconv.Atoi(parts[0]) |
There was a problem hiding this comment.
We shouldn't skip the error here
There was a problem hiding this comment.
Normally I'm 100% with you, and please correct me if I'm wrong, but I'm not sure what error we'd capture here or would want to bubble up considering that we're guaranteed to have 0 output by strconv.Atoi regardless of input or error. We should always handle a 0 versioned compose file since they're the default in all cases.
There was a problem hiding this comment.
@vdemeester, interested in your thoughts here. Thanks.
There was a problem hiding this comment.
@cheapRoc version is of type string (in the schema), so I could put a.b it would pass the validation. That's why I think we should check for errors.
There was a problem hiding this comment.
I agree, we should validate the full version number since it's a string. Maybe that's something that needs to be handled before this spot (?). That's not what I was trying to achieve here.
At this line, and the other you pointed out, I'm only parsing a single integer value. The a of a.b, and I should be handling the default case as 0 appropriately.
There was a problem hiding this comment.
That’s what I’ll do, I’ll formally parse the schema version major and minor and try to reuse it in other places.
| } | ||
|
|
||
| parts := strings.Split(config.Version, ".") | ||
| major, _ := strconv.Atoi(parts[0]) |
There was a problem hiding this comment.
Same here (error skipping)
| "^[a-zA-Z0-9._-]+$": { | ||
| "$ref": "#/definitions/service" | ||
| } | ||
| "^[a-zA-Z0-9._-]+$": { |
There was a problem hiding this comment.
I think we should extract those in json files and have go generate create this file from it (like we do on docker/cli). Not sure if we should do it in the PR or not
There was a problem hiding this comment.
I agree, I was hoping someone would mention this. It's definitely annoying to have them in a huge file.
There was a problem hiding this comment.
I checked out docker/cli (this is it correct?) and noticed that it uses go-bindata. I've used that in the past for storing JSON schemas into a binary loading them at run time, so I understand the context. Is that how you foresee doing things in libcompose?
If so, I can probably swing that.
|
This has fallen off my immediate road map as the project I intended to use it for hasn't materialized. If I have the free time I'll re-open the PR. Apologies! |
3 participants
Ref: #441
The goal of this PR is to hopefully introduce v2.1 formatted compose files so I can use
libcomposefor various stuff I'm working on around Joyent's Elastic Docker Host (Triton).I've started by refactoring how compose files are loaded and validated by their version numbers. This solves the immediate issue of #441 by cleaning up how the version is referenced when parsing compose YAML. At the moment, I've left off cleaning up how JSON schema files are loaded and passed into
config/validation.go. This is working well so I'll start utilizing this for loading v2.1 specifically.I'm working on this in my spare time ATM (which isn't much) so bare with me. Also, I'm opening this up to get any early feedback. I do realize I need tests for some stuff. 😄