Code review comment for lp:~mwhudson/lava-dispatcher/more-validation

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