Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add linting and validation of all YAML files#27698
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
So just a few things blocking this one:
|
In the interest of getting this in without having to deal with many conflicts, I've commented out the Azure Pipelines check, which we can enable once it's fixed. |
Ah, it looks like pre-commit.ci doesn't have network access, so we can't reference schemas from SchemaStore with URLs. Would we be fine with dropping those schemas in the repo somewhere? |
personally, unless the schemas are too large (I'm really thinking like megabytes, which would be relatively impressive for plain text files) I'd be okay with adding them, probably under the |
They are, at this time, 160K, so not much of a burden to ship. |
| @@ -146,39 +147,39 @@ jobs: | |||
| path: dist/ | |||
| - name: Build wheels for CPython 3.12 | |||
| uses: pypa/cibuildwheel@ce3fb7832089eb3e723a0a99cab7f3eaccf074fd # v2.16.5 | |||
| uses: pypa/cibuildwheel@ce3fb7832089eb3e723a0a99cab7f3eaccf074fd# v2.16.5 | |||
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.
Just want to double check that this change does not interfere with dependabot...
my guess is that it is not a problem for dependabot directly... but that it is possible/likely that dependabot will not place two spaces when it updates, and thus cause linting failures as a result.
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.
Looking atthe implementation commit, thecomment regex is(?<comment>\s+#.*), so it should be fine with any number of leading spaces/tabs. Thenupdating the version is only replacing old with new.
There's also atest case with multiple spaces before and after the#.
.pre-commit-config.yaml Outdated
| - id: check-jsonschema | ||
| files: ^\.appveyor\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/appveyor.json"] | ||
| - id: check-jsonschema | ||
| files: ^\.circleci/config\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/circleciconfig.json"] | ||
| - id: check-jsonschema | ||
| files: ^\.github/FUNDING\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/github-funding.json"] | ||
| - id: check-jsonschema | ||
| files: ^\.github/ISSUE_TEMPLATE/config\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/github-issue-config.json"] | ||
| - id: check-jsonschema | ||
| files: ^\.github/ISSUE_TEMPLATE/.*\.yml$ | ||
| exclude: ^\.github/ISSUE_TEMPLATE/config\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/github-issue-forms.json"] | ||
| - id: check-jsonschema | ||
| files: ^\.github/codecov\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/codecov.json"] | ||
| - id: check-jsonschema | ||
| files: ^\.github/labeler\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/pull-request-labeler-5.json"] | ||
| - id: check-jsonschema | ||
| files: ^environment\.yml$ | ||
| args: ["--verbose", "--schemafile", "ci/schemas/conda-environment.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.
I would prefer if each of these could be given a uniquename that would make it easier to tell what to look for when it fails (I don't actually know what all is printed on failure, but the wall of 8 genericValidate files with jsonschema doesn't actually tell me what it has validated (granted, mostly will be skipped when running locally since they all have file filters, but still)
(Also that it saysjsonschema is mildly confusing since these are yaml files... I understand that the schema is specified in json, but not sure I would remember(/expect if I didn't know this PR) that when the check fails)
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 would put a README inschemas stating
Vendored YAML Schemas for linting and validation.
Since pre-commit CI doesn't have Internet access, we need to bundle these files
in the repo. The schemas can be updated usingvendor_schema.py.
Also, consider puttingvendor_schema.py in theschema folder. It's slightly better to have everything in one place rather than the generating script in the parent folder.
Otherwise LGTM.
The `if:` expressions don't need to preserve newlines, for example.
This is necessary because pre-commit CI doesn't have Internet access andthus cannot download schemas itself.
Not sure whether CI failures are unrelated. You may self-merge if you are sure, they are unrelated. |
QuLogic commentedFeb 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think (other than Cygwin) they're all unfortunately flaky errors, but I've re-run them for now. |
Everything looks good now. |
PR summary
This adds stylistic linting and schema validation to all YAML files that we use. This will prevent errors like#27642 and#27694 before merging.
Because these files tend to have fairly nested entries, I chose a rather long line length of 111. This is long enough to fitthe longest action reference without wrapping. Note also that
azure-pipelines.ymlwas almost entirely incorrectly indented, so appears as if it's changed entirely, but it's mostly just indent.I also changed some newline-preserving blocks (
|) to newline-folding (>-) as they don't need to keep newlines.At the moment, some of the schemas do not pass even though the CI works, so I'm just checking whether those are outdated or just a bit stricter about things.
PR checklist