schema: required keywords not enforced for spack in config.yaml

[albestro]

• schema: refactor validators and add pre-check for version #254

PR #245 added to the config schema some requirements about spack and spack:packages.

Unfortunately these new changes were just partially enforced, because the schema is not valid due to the required entry for spack being nested in properties.

@bcumming was it your intention to have both repo and commit mandatory for both spack and spack:packages?

[albestro]

From CI it seems that this makes fail schemas v1, which opens up again the topic raised up in #247 (comment)

[albestro]

This currently depends on #254. If that proposal gets accepted, this can go on.

[bcumming]

This is a nice little fix and clean up.

I have one possible nit about not setting a default commit: null value for a git repository type.

[bcumming]

Nice.

[albestro]

Unfortunately, I had to revert it. But I agree, it is quite nice. I'm going to explore how to change the validator in order to deal correctly with defaults also in presence of $ref, but in a following PR.

[bcumming]

provide a default value for commit?
Stackinator expects this field to be set, and has logic to ignore it if it is None/null.

[albestro]

@bcumming was it your intention to have both repo and commit mandatory for both spack and spack:packages?

do you mean that you don't want commit to be required? it's perfectly fine. I had two ways to fix with this PR, and looking at the misplaced required I inferred that you wanted it to be required (see in #245 https://github.com/eth-cscs/stackinator/pull/245/files#diff-2bdfe86113f7b73f4dd5fef35aaa8a54fe83e02e02bf212360979e4c23cada84).

Just to be clear, I think that:

• if commit is required, we can skip the default value
• if commit is not required, we should put a default (and check it in tests)

[bcumming]

I think commit should not be required.
We should provide a default that is null (i.e. empty).
This might seem odd, but the code in stackinator expects the field to be set, and gracefully handles the case where the value is "null".

[albestro]

It took a bit of time because I encountered a problem with defaults with $ref. Since it is unrelated and it requires some time, I opted for reverting that change and go for duplication, at least for now. It makes even more clear what is going on in this PR.

On the test side, I just added a couple of "in-place" test-cases for spack:commit and spack:packages:commit defaults. Minimal changes to the rest of test recipes, just to make them pass validation.

[albestro]

@bcumming @simonpintarelli @msimberg

Just to highlight the decision: both spack:commit and spack:packages:commit are not required.

Before this PR, just the latter was required and enforced, while the former was in principle required but it was not enforced (together with other properties), due to the misplaced required field in the schema.