Code review comment for lp:~bcsaller/pyjuju/config-set

Interactive interaction online, and +1!

<niemeyer> bcsaller: Oh, cool.. just a small note re. the review: [5] seems untouched
<bcsaller> checking
<niemeyer> bcsaller: Re. [6], thanks for the test. We really need tests specific to the feature, otherwise you remove the other method later and this logic goes untested
<niemeyer> bcsaller: It would be fine to move the logic from the other tests though
<niemeyer> bcsaller: and only test on the layer above what you haven't already tested for the layer below
<niemeyer> bcsaller: Having a ton of logic on a layer below which is only touched by tests for upper layers is the issue
<bcsaller> yeah, looks like maybe I did miss [5], I think I made changes related to [9] and thought I got it
<niemeyer> bcsaller: No worris
<niemeyer> es
<niemeyer> bcsaller: re [8], is there a reason why you didn't use our schema framework in the same way we do for metadata and configuration?
<bcsaller> niemeyer: yes, the formula author has to write these things, so either I did something simple or I had to write a yaml serialization of the schema denifnition
<niemeyer> bcsaller: I don't understand.. how's that different from the metadata file, or the environments configuration file?
<bcsaller> because the schema is coded in Python by us for the metadata file, for the config.yaml they are writing a validation spec of their own
<niemeyer> bcsaller: No, it's not..
<niemeyer> bcsaller: The schema is a generic piece of framework which you can use to parse plain dictionaries and values
<niemeyer> bcsaller: and is used both for the metadata and the environments file, which are also written by hand
<bcsaller> yes, but the desc of that is done in python
<niemeyer> bcsaller: Yes, and we can do the same for the config yaml, I believe
<niemeyer> ?
<bcsaller> I didn't want formula authors to have to write in python to use the schema system
<niemeyer> bcsaller: They don't have to..
<niemeyer> bcsaller: We're talking about validating a yaml file
<bcsaller> oh, you mean to validate that the config.yaml is correct
<niemeyer> bcsaller: Yes
<niemeyer> bcsaller: Point [8] in the review
<bcsaller> that's fine, yes, I implemented validation of option values
<niemeyer> bcsaller: Yes, and why isn't that written using our schema framework?
<bcsaller> I think I misunderstood in that case, oh well we get a new feature
<niemeyer> bcsaller: in declarative style.. that was the actual questoin
<bcsaller> validating the config.yaml (which isn't really done now) using the schema would be fine
<niemeyer> bcsaller: Yeah, that was point [8]
<niemeyer> bcsaller: You can do that in a follow up branch
<bcsaller> I have no objection to that, I thought you wanted to have them write schema to validate the options they allow for the service
<niemeyer> bcsaller: Nope
<niemeyer> bcsaller: We just have to validate the file format before bundling a formula
<niemeyer> bcsaller: Otherwise it will blow up when the formula is deployed only, which would be quite sad
<bcsaller> ok, I can add that tonight, it's a small change
<niemeyer> bcsaller: Should be indeed, no rush, and thanks!