On Fri, 09 Mar 2012 10:16:20 -0000, Zygmunt Krynicki <email address hidden> wrote:
> Review: Approve
> 
> 8	+* Make the validation of the job file that happens before a job starts
> 9	+ more rigorous.
> 
> Yay for changelogs!

:)

> 108	+
> 109	+ parameters_schema = null_or_trivial_schema
> 110	+
> 111	def run(self):
> 
> Won't this break?
> 
> If I say: 
> 
> params: {"foo": 1, "bar": 2}
> 
> Then the code will call
> 
> run(foo=1, bar=2)
> 
> But run() will raise a TypeError then.

Ah, maybe null_or_trivial_schema is mis-named.  null_or_trivial_schema
matches {} or None/null -- nothing else.  I'll change it to
null_or_empty_schema.

> 82	+ 'boot': {'type': 'string'},
> 83	+ 'system': {'type': 'string'},
> 84	+ 'data': {'type': 'string'},
> 
> I smell URL validation.

Heh yes.

> 85	+ 'pkg': {'type': 'string', 'optional': True},
> 
> ?
> 
> 86	+ 'use_cache': {'type': 'bool', 'optional': True},
> 
> If you have optional then also specify default. This is super nice when you have json-document (and we will soon in the dispatcher). Then you can just say: params.use_cache and the default will be magically there. DRY (no need to keep the default on the python function).

Ah yes, that would be nice.  I'll add the defaults.
`
Also would be nice to not have parameters as arguments to the run
function, I like the sound of params.use_cache instead...

> 87	+ 'rootfstype': {'type': 'string', 'optional': True},
> 
> What was this? Is this an enum?

I ... guess it's an enum.  I don't know if we want to encoode all the
filesystems supported for mkfs in our master images in the schema
though?  I'll leave it as string for now.

> 182	+ if 'hwpack' in parameters:
> 183	+ if 'rootfs' not in parameters:
> 184	+ raise ValueError('must specify rootfs when specifying hwpack')
> 
> You can specify that via the schema:
> 
> "hwpack": {
>    "requires": "rootfs"  // 3rd draft renames this to "dependencies"
> }
> 
> "rootfs" {
>    "requires": "hwpack"  // 3rd draft renames this to "dependencies"
> }

Nice.  Although your next suggestion is actually better...

> 185	+ if 'image' in parameters:
> 186	+ raise ValueError('cannot specify image and hwpack')
> 
> This tells me that the schema is a type union. You should list all valid types as complete alternatives and don't resort to optional:
> 
> {
> "type": [schema_rootfs_plus_hwpack, schema_image]
> }

Ah, I didn't know you could do that.

> 189	+ elif 'kernel_matrix' in parameters:
> 190	+ raise ValueError('cannot specify kernel_matrix with an image')
> 
> I don't get this. What was kernel_matrix again?

I don't remember entirely either.  Let's not try to solve all the
problems of the world at once :-)

> 212	+ 'result_disk': {'type': 'string', 'optional': True},
> 
> What is this?

I don't remember.  I'm pretty sure it's useless, but didn't want to do
the legwork to check if anyone has used it...

> Hint: let's start using "title" and "description". I'll pull them in json-document pydoc support.
> 
> 
> 233	+ 'timeout': {'type': 'integer', 'optional': True},
> 
> Let's add validation against ranges (0 -> something). I'll implement them in the validator.

What is the syntax for ranges?

> 
> Otherwise +1

Thanks!

Cheers,
mwh