Conversation
fernando79513
left a comment
There was a problem hiding this comment.
This is very very cool.
I've run it in all the yaml files in the base provider, and mostly the linting issues were related to the estimated duration issues:
https://docs.google.com/spreadsheets/d/1G-8OoozIgTTThQCZA-tSOnv5Nyj1aDVDkQH5_vigtQY/edit?gid=0#gid=0
| "estimated_duration": { | ||
| "description": "Estimated job duration as a float in seconds or a duration string (e.g. 1h 2m 30s).", | ||
| "oneOf": [ | ||
| { "type": "number", "minimum": 0 }, |
There was a problem hiding this comment.
This field does not match the translated units.
The field is always translated as a string, but the linter looks for either a number or a specific pattern, so it fails validation:
id: audio/list_devices
estimated_duration: 1.0
id: audio/list_devices
estimated_duration: '1.0'Raises this error.
Incorrect type. Expected "number".yaml-schema: Job Unit | Template Unit
We should either modify the translator so we do the parsing there so the new field in the yaml is estimated_duration: 1.0, or accept every string that matches a float in the linter estimated_duration: '1.0' probably going for the first one
We could even force the estimation parsing directly on the translation. Having both the float and the "h m s" approaches feels a bit weird to me. TBH, I don't think we are really using float estimations, but I think may be out of the scope for this PR.
| "type": "array", | ||
| "items": { | ||
| "type": "string", | ||
| "enum": [ |
There was a problem hiding this comment.
There are several jobs that still use the "preserve-locale" flag, and it's marked as not existing.
We should remove this flag from all the jobs that use it in our repo, but especially from the docstrings in startprovider.py
BTW, its the first time I see the first try for Zygga to create a syntax highlighting:
https://github.com/canonical/checkbox/blob/main/checkbox-ng/contrib/pxu.vim
| "title": "Category Unit", | ||
| "description": "Schema that describes a Checkbox Category unit", | ||
| "type": "object", | ||
| "properties": { |
There was a problem hiding this comment.
[GENERAL] We could also add "unevaluatedProperties": false,
That way, all the fields that are not evaluated will be marked by the linter:
unit: category
id: my_category
non-existing-field: field <- This will be linted
| "plugin": { | ||
| "description": "Type of job. Determines how the job is run and how the outcome is determined.", | ||
| "type": "string", | ||
| "enum": ["manual", "shell", "user-interact", "user-interact-verify", "attachment", "resource"], |
There was a problem hiding this comment.
We are missing user-verify plugin
| "items": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "with-io-log", |
There was a problem hiding this comment.
Do we know if we are really using these anywere?
I looked for many of them and couldn't find any usage in our codebase, I only see 5 of them in our exporter:
with-sys-info, with-summary, with-job-description, with-text-attachments,tp-export
| "unit": { | ||
| "description": "The type of unit.", | ||
| "const": "category" | ||
| }, | ||
| "id": { | ||
| "description": "Unique identifier of the category.", | ||
| "type": "string" | ||
| }, | ||
| "name": { | ||
| "description": "Human readable name of the category", | ||
| "type": "string" |
There was a problem hiding this comment.
[GENERAL] I think it's clearer if you add the name of the field to the title and have the unit in parenthesis like a subtitle, or if you preffer in the desciption:
```suggestion
"unit": {
"title": "Unit (Category Unit)",
"description": "The type of unit.",
"const": "category"
},
"id": {
"title": "ID (Category Unit)",
"description": "Unique identifier of the category.",
"type": "string"
},
"name": {
"title": "Name (Category Unit)",
"description": "Human readable name of the category",
"type": "string"
}
There was a problem hiding this comment.
This incidentally solves a weird issue, that after some investigation, I don't know why it's occurring. If you don't set the title per field, the unit title is inherited from the template schema
"id": {
"description": "Unique identifier of the job",
"type": "string"
},
But if you set the title section for each one of the unit fields, then the correct title appears:
"id": {
"title": "ID (Job Unit)",
"description": "Unique identifier of the job",
"type": "string"
},
There was a problem hiding this comment.
Pull request overview
This PR introduces a JSON Schema set intended to validate Checkbox “unit” definitions (job/test plan/category/template/exporter/manifest entry/packaging meta-data), plus editor configuration to wire the schema into VS Code, and a small documentation correction for exporter units.
Changes:
- Added a top-level unit schema (
unit.schema.json) and per-unit schemas underunit_json_schema/. - Updated exporter documentation to include
tarandtp-exportas supported values/options. - Added a VS Code YAML schema mapping to apply the unit schema to selected YAML file globs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| unit_json_schema/unit.schema.json | Entry-point schema that references the per-unit schemas via $ref. |
| unit_json_schema/job.schema.json | Schema for job units (shared fields + siblings). |
| unit_json_schema/test-plan.schema.json | Schema for test plan units including include/mandatory/bootstrapping fields. |
| unit_json_schema/category.schema.json | Schema for category units. |
| unit_json_schema/template.schema.json | Schema for template units (extends job fields). |
| unit_json_schema/exporter.schema.json | Schema for exporter units. |
| unit_json_schema/manifest-entry.schema.json | Schema for manifest entry units. |
| unit_json_schema/packaging-meta-data.schema.json | Schema for packaging meta-data units. |
| docs/reference/units/exporter.rst | Documentation update to reflect tar and tp-export. |
| .vscode/settings.json | VS Code YAML schema association configuration. |
Key issues to address before this is practically usable in-repo (all verified against existing provider .pxu unit files and the docs in docs/reference/units/):
- All schemas use
$idvalues pointing at a non-existentschema_checkbox/path and also use GitHubblob/URLs (HTML), which will break remote resolution and can break relative$refresolution in some tooling. - Several schemas require non-underscored fields (e.g.
name,summary,template-summary) but existing units and docs consistently use translatable underscore forms (e.g._name,_summary,_template-summary)—causing widespread validation failures. - Multiple fields are typed as arrays in the schemas (e.g.
depends,requires,imports,flags,environ), but are commonly expressed as scalar strings in existing.pxuunits (and documented as space/comma-separated lists), which will also fail validation. template.schema.jsonrestrictstemplate-unittoconst: "job", but the template unit documentation explicitly states other values may be used..vscode/settings.jsonmaps the schema tounits/*.yamlglobs, but this repository’s actual unit files are primarilyproviders/**/units/**/*.pxu(so the mapping won’t apply unless.pxuis associated and included).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
This adds a json schema for yaml Checkbox units. This also adds pre-configuration for VS Code to simplify the life of those using it, which I think is why we committed it to the repo.
Minor: this fixes a missing exporter in the docs I found.
Resolved issues
Fixes: CHECKBOX-1396
Documentation
N/A
Tests
Tested locally with the vs code plugin + the neovim lsp plugin