-
Notifications
You must be signed in to change notification settings - Fork 5
Add JSON Schema for bootspec #176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
grahamc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, beyond a nit.
| @@ -0,0 +1,87 @@ | |||
| { | |||
| "$id": "https://raw.githubusercontent.com/DeterminateSystems/bootspec/v1.0.0/schema.json", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we host this somewhere nicer, to give this a nicer URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions?
| "org.nixos.specialisation.v1": { | ||
| "type": "object", | ||
| "patternProperties": { | ||
| "^.*$": { | ||
| "type": "object", | ||
| "properties": { | ||
| "org.nixos.bootspec.v1": { "$ref": "#/$defs/Bootspec" } | ||
| }, | ||
| "required": ["org.nixos.bootspec.v1"], | ||
| "additionalProperties": true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this matches what is described
|
Drat. I didn't think automerge was enabled. |
| @@ -0,0 +1,87 @@ | |||
| { | |||
| "$id": "https://raw.githubusercontent.com/DeterminateSystems/bootspec/v1.0.0/schema.json", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably won't work, since this schema didn't exist when we released v1.0.0, back in 2023 :)
| "patternProperties": { | ||
| "^.*$": { | ||
| "$ref": "#/$defs/Bootspec", | ||
| "description": "Testing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we combine this with the CUE validator that has been deployed in Nixpkgs for 2+ years now: https://github.com/NixOS/nixpkgs/blob/0d296899ff1525e7af62a6b69122a70b26050b5f/nixos/modules/system/activation/bootspec.cue
CUE supports importing JSON Schema into CUE (https://cuelang.org/docs/concept/how-cue-works-with-json-schema/), so maybe we can do something to ensure the canonical CUE from Nixpkgs and the JSON Schema here are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is reflecting the documented syntax, I like the option of publishing this JSON schema document that can be used from CUE. If it is identified to be insufficient / not reflecting the spec as designed, we can update it. I'm not too fussed about it being precisely / perfectly matched immediately, since it is unlikely to supplant the cue document right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still rather ensure that they are the same (by comparing them), than hoping they are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fwiw I did compare them by reading the code and was satisfied they were the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though perhaps not totally satisfactory.
I pasted this sample: https://github.com/DeterminateSystems/bootspec/blob/9f2a933acda49fe6a146dac3050b1b9d8b20e4f9/synthesize/integration-test-cases/expected-synthesis/21.11-specialisations.json
into this online validator: https://www.jsonschemavalidator.net/
and it complains:
Required properties are missing from object: init, kernel, kernelParams, label, system, toplevel.
I think something might be missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, yeah, generally the handling of specialisations aren't right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, the goal is not to have a functioning validator. The goal is to have a JSON spec and to generate docs with it (and have IDEs and other tools consume it). Ideally you could have a .cue file with this specified (as Nixpkgs does) and generate a JSON Schema file from that, but CUE doesn't really work that way, because CUE doesn't see that bootspec.cue file as data that it can generate something out of. Instead, it sees it as a data schema that it can use to validate other data. So CUE can take that bootspec.cue file and make sure that a given bootspec.json file conforms to the structure that is prescribed by CUE. But it can't turn bootspec.cue into a compliant JSON Schema JSON object with descriptions, examples, etc.
This PR provides a JSON Schema for bootspec JSON plus some documentation that will be available at https://determinatesystems.github.io/bootspec.
TODO: someone with proper creds will need to enable GitHub Pages for this repo.