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

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.

82 + 'boot': {'type': 'string'},
83 + 'system': {'type': 'string'},
84 + 'data': {'type': 'string'},

I smell URL validation.

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).

87 + 'rootfstype': {'type': 'string', 'optional': True},

What was this? Is this an enum?

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"
}

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]
}

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?

212 + 'result_disk': {'type': 'string', 'optional': True},

What is this?
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.

Otherwise +1