Skip to content

Conversation

@draghuram
Copy link

I haven't been able to work on this of late so I am creating this PR in the hope that some one else can take it forward.

Note that the validation is done after the configuration is
converted to Python so the validation should work for any
file format.

Handles #129.

Signed-off-by: Raghuram Devarakonda <draghuram@gmail.com>
@funkyfuture
Copy link

related to #129

@funkyfuture
Copy link

thanks for sharing that approach. after a first review, here are some thoughts:

  • i would seperate file- and service-validation, the latter would be done in process_container_options
  • there are some checks in other parts of the code, that
    • can be moved to the validation
    • are ineffective after validation and therefore should be moved to the validation or be removed
  • the DOCKER_CONFIG_KEYS should be derived programatically from the validation schema
  • the validation could be far more specific regarding the values, e.g.:
    • expose-keys must contain only digits
    • paths are expected differently on different OS
      • their existence in the fs could even be checked, but that seems too far off
  • i also found these two packages which seem to be capable to do the job, are more pythonic and extendable:

@aanand @bfirsh @dnephin @draghuram i'd appreciate any feedback before i get on it.

@aanand
Copy link

aanand commented Apr 27, 2015

Thanks for both your PRs - I'm going to take a proper look at them soon.

@aanand
Copy link

aanand commented Apr 29, 2015

This looks like a good start. I don't feel especially strongly about this one vs @funkyfuture's so if you've not got the time, that one will probably win by default.

I know @dnephin has advocated for using JSON schema in the past and I can see a hypothetical argument for it (should the Compose format ever become something that's serializable to JSON) but I don't think it's a particularly strong one.

@draghuram
Copy link
Author

@aanand Thanks for the comments. I agree that we should go with @funkyfuture's PR considering that he is willing to see the feature through.

@mnowster
Copy link

Handled by #1808

@mnowster mnowster closed this Aug 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants