build,tools: implement YAML linting #40007

Trott · 2021-09-05T13:39:45Z

No description provided.

Trott · 2021-09-05T13:39:57Z

@nodejs/linting @Mesteery

Trott · 2021-09-05T13:41:39Z

This doesn't yet address the dreaded "to quote or not to quote strings in YAML" question. I want to get the basic YAML linting implemented and working first before trying to address issues around which there might be conflicting opinions. The things that were flagged by the tool that are fixed in this PR--trailing spaces, inconsistent indentation, and missing newlines at the ends of files--all seem like they would be uncontroversial.

Trott · 2021-09-05T13:53:15Z

The GitHub Action seems to have worked: https://github.com/nodejs/node/pull/40007/checks?check_run_id=3517929071

nodejs-github-bot · 2021-09-05T13:54:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39795/

Mesteery · 2021-09-05T13:56:16Z

#40004 (comment)
I was thinking of making an eslint-like yaml linter from 0, in JS, with a plugin for github action validation. And use it here in the same way as eslint. But if you prefer this, that's fine with me.

Trott · 2021-09-05T14:02:03Z

#40004 (comment)
I was thinking of making an eslint-like yaml linter from 0, in JS, with a plugin for github action validation. And use it here in the same way as eslint. But if you prefer this, that's fine with me.

That sounds like it might be less code inside Node.js core for this, which I would like. How long do you imagine it would take you to create such a thing? It may make sense to land this as something we can do now and move to your tool when it's ready. For that matter, any issues/pain points we find with this solution may inform your design and/or feature set.

Mesteery · 2021-09-05T14:15:34Z

How long do you imagine it would take you to create such a thing?

I don't really know, but I think it wouldn't take very long, on the order of a few days/week (if I have the time).

It may make sense to land this as something we can do now and move to your tool when it's ready. For that matter, any issues/pain points we find with this solution may inform your design and/or feature set.

+1

targos · 2021-09-05T14:38:25Z

I don't know what it's worth but I found an ESLint plugin for YAML:
https://github.com/ota-meshi/eslint-plugin-yml

Makefile

Trott · 2021-09-05T18:43:16Z

I don't know what it's worth but I found an ESLint plugin for YAML:
https://github.com/ota-meshi/eslint-plugin-yml

I'm not opposed to using that instead, but here would be the arguments against it:

yamllint seems like the industry standard for YAML linting.
Bolting on YAML parsing to ESLint seems like Frankenstein code (someone help me with a better term?) and might be best avoided.
The quotes rule for that plugin doesn't seem to allow for the enforcement of "don't quote unless you need to". While I'm in the "always quote" camp, some (notably @Mesteery) are not. I want to have the option to say "OK, we'll do it that way" because I care much more about the tool enforcing consistency than I care about the quotes vs. no-quotes. (This can probably be fixed with a pull request to the project which seems under active development.)
Version 0.10.0 might cause some people to want to stay away from it for now. (I'm not one of those people, though.)

nodejs-github-bot · 2021-09-05T18:44:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39802/

Trott · 2021-09-05T18:47:15Z

Jenkins linter worked too! (@aduh95 Note the Python 2.7.17. That's from the PYTHON environment variable, which is being overridden by the Jenkins job config to be Python 2 rather than Python 3. 😞 But that can change after 12.x goes EOL, or maybe sooner if we want to mess with it. But I'd want that to be separate from this PR and then we can add the PYTHON env var use back in to this.)

https://ci.nodejs.org/job/node-test-linter/42105/console

07:01:44 Python 2.7.17
07:01:44 Pip installing yamllint on ...
07:01:44 python3 -m pip install --upgrade -t tools/pip/site-packages yamllint || \
07:01:44 	python3 -m pip install --upgrade --system -t tools/pip/site-packages yamllint
07:01:45 Collecting yamllint
07:01:45   Downloading yamllint-1.26.3.tar.gz (126 kB)
07:01:46 Collecting pathspec>=0.5.3
07:01:46   Downloading pathspec-0.9.0-py2.py3-none-any.whl (31 kB)
07:01:46 Collecting pyyaml
07:01:46   Downloading PyYAML-5.4.1-cp36-cp36m-manylinux1_x86_64.whl (640 kB)
07:01:46 Collecting setuptools
07:01:46   Downloading setuptools-57.5.0-py3-none-any.whl (819 kB)
07:01:46 Building wheels for collected packages: yamllint
07:01:46   Building wheel for yamllint (setup.py): started
07:01:47   Building wheel for yamllint (setup.py): finished with status 'done'
07:01:47   Created wheel for yamllint: filename=yamllint-1.26.3-py2.py3-none-any.whl size=49687 sha256=a6e6316d6556bf3e81c1b2ba2dcfdece0cff6bbfd4ec73c1e702de05408052a8
07:01:47   Stored in directory: /home/iojs/.cache/pip/wheels/d3/e9/90/f0e40b6ef41b776cd292944dca45d5a52ca37731e135fce6f2
07:01:47 Successfully built yamllint
07:01:47 Installing collected packages: setuptools, pyyaml, pathspec, yamllint
07:01:47 Successfully installed pathspec-0.9.0 pyyaml-5.4.1 setuptools-57.5.0 yamllint-1.26.3
07:01:48 Makefile:1428: YAML linting with yamllint is not available
07:01:48 Makefile:1428: Run 'make lint-yaml-build'

Trott · 2021-09-05T18:48:29Z

Oh, wait, darnit, the Jenkins job didn't work:

07:01:48 Makefile:1428: YAML linting with yamllint is not available

OK, I'll go take a look later and see if I can figure out what went sideways. (Although if anyone beats me to it, that's great!)

aduh95 · 2021-09-05T18:57:06Z

I'm not sure we need to wait for Node.js 12.x to go away, AFAIK this job doesn't build node, so it should be alright to use python3? I haven't tested it though, but I'd find it surprising that it wouldn't work. Note that we could also specify a rule in the linter script if (node_major > 13) PYTHON=python3 else PYTHON=python2. There's already a similar check anyway for make lint-py: https://github.com/nodejs/build/blob/master/jenkins/pipelines/node-linter.jenkinsfile#L54-L59.

I understand that prefer not to do it, and I'm not blocking this PR to be clear.

Trott · 2021-09-05T19:58:55Z

I'm not sure we need to wait for Node.js 12.x to go away, AFAIK this job doesn't build node, so it should be alright to use python3? I haven't tested it though, but I'd find it surprising that it wouldn't work. Note that we could also specify a rule in the linter script if (node_major > 13) PYTHON=python3 else PYTHON=python2. There's already a similar check anyway for make lint-py: https://github.com/nodejs/build/blob/master/jenkins/pipelines/node-linter.jenkinsfile#L54-L59.

I understand that prefer not to do it, and I'm not blocking this PR to be clear.

I got confused by this in the Jenkins job:

#!/bin/bash -ex

# Lint with python3 in new branches
# Refs: https://github.com/nodejs/build/issues/1631
if [[ "$NODEJS_MAJOR_VERSION" -ge "12" ]]; then
  make lint-py-build PYTHON=python3 || true
  make lint-py PYTHON=python3
fi

# show Node.js version used to run linter
node --version || true

make lint-py-build PYTHON=python2 || true

So it runs it with Python 3 for Node.js greater than 12.x, but it also runs it with Python 2 for everything regardless of version.

BUT....that's just Python linting, not all linting.

So I think you're right and I can just use the environment variables. Sorry for the confusion!

targos

GitHub actions output:

Pip installing yamllint on Python 3.9.6...
python3 -m pip install --upgrade -t tools/pip/site-packages yamllint || \
	python3 -m pip install --upgrade --system -t tools/pip/site-packages yamllint
Collecting yamllint
  Downloading yamllint-1.26.3.tar.gz (126 kB)
Collecting pathspec>=0.5.3
  Downloading pathspec-0.9.0-py2.py3-none-any.whl (31 kB)
Collecting pyyaml
  Downloading PyYAML-5.4.1-cp39-cp39-manylinux1_x86_64.whl (630 kB)
Collecting setuptools
  Downloading setuptools-57.5.0-py3-none-any.whl (819 kB)
Using legacy 'setup.py install' for yamllint, since package 'wheel' is not installed.
Installing collected packages: setuptools, pyyaml, pathspec, yamllint
    Running setup.py install for yamllint: started
    Running setup.py install for yamllint: finished with status 'done'
Successfully installed pathspec-0.9.0 pyyaml-5.4.1 setuptools-57.5.0 yamllint-1.26.3
Makefile:1421: YAML linting with yamllint is not available
Makefile:1421: Run 'make lint-yaml-build'

Fix indentation, traiiling spaces, and missing newline issues in preparation for linting.

Trott · 2021-09-06T10:57:25Z

GitHub actions output:

@targos Fixed. Thanks.

https://github.com/nodejs/node/pull/40007/checks?check_run_id=3523886970

Pip installing yamllint on Python 3.9.6...
python3 -m pip install --upgrade -t tools/pip/site-packages yamllint || \
	python3 -m pip install --upgrade --system -t tools/pip/site-packages yamllint
Collecting yamllint
  Downloading yamllint-1.26.3.tar.gz (126 kB)
Collecting pathspec>=0.5.3
  Downloading pathspec-0.9.0-py2.py3-none-any.whl (31 kB)
Collecting pyyaml
  Downloading PyYAML-5.4.1-cp39-cp39-manylinux1_x86_64.whl (630 kB)
Collecting setuptools
  Downloading setuptools-58.0.0-py3-none-any.whl (816 kB)
Using legacy 'setup.py install' for yamllint, since package 'wheel' is not installed.
Installing collected packages: setuptools, pyyaml, pathspec, yamllint
    Running setup.py install for yamllint: started
    Running setup.py install for yamllint: finished with status 'done'
Successfully installed pathspec-0.9.0 pyyaml-5.4.1 setuptools-58.0.0 yamllint-1.26.3

nodejs-github-bot · 2021-09-06T10:58:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39819/

nodejs-github-bot · 2021-09-06T17:11:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39833/

github-actions · 2021-09-07T13:54:12Z

Landed in 773a989...b016d5d

Fix indentation, traiiling spaces, and missing newline issues in preparation for linting. PR-URL: #40007 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: #40007 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Fix indentation, traiiling spaces, and missing newline issues in preparation for linting. PR-URL: #40007 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: #40007 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Richard Lau <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels Sep 5, 2021

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2021

jasnell approved these changes Sep 5, 2021

View reviewed changes

aduh95 reviewed Sep 5, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

targos requested changes Sep 6, 2021

View reviewed changes

Trott added 3 commits September 6, 2021 03:54

tools,build: update YAML files in preparation for linting

2dfdff0

Fix indentation, traiiling spaces, and missing newline issues in preparation for linting.

build: add YAML linting

6e6ff64

build: add YAML linting to GitHub Actions

7594048

Trott force-pushed the lint-yaml branch from 045fdad to 7594048 Compare September 6, 2021 10:54

aduh95 approved these changes Sep 6, 2021

View reviewed changes

targos approved these changes Sep 6, 2021

View reviewed changes

richardlau approved these changes Sep 6, 2021

View reviewed changes

VoltrexKeyva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Sep 7, 2021

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2021

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2021

github-actions bot closed this Sep 7, 2021

BethGriggs mentioned this pull request Sep 21, 2021

v16.10.0 proposal #40175

Merged

1 task

Trott deleted the lint-yaml branch September 25, 2022 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,tools: implement YAML linting #40007

build,tools: implement YAML linting #40007

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

nodejs-github-bot commented Sep 5, 2021

Mesteery commented Sep 5, 2021

Trott commented Sep 5, 2021

Mesteery commented Sep 5, 2021

targos commented Sep 5, 2021

Trott commented Sep 5, 2021 •

edited

Loading

nodejs-github-bot commented Sep 5, 2021

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

aduh95 commented Sep 5, 2021

Trott commented Sep 5, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

targos left a comment

Trott commented Sep 6, 2021

nodejs-github-bot commented Sep 6, 2021

nodejs-github-bot commented Sep 6, 2021

github-actions bot commented Sep 7, 2021

build,tools: implement YAML linting #40007

build,tools: implement YAML linting #40007

Conversation

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

nodejs-github-bot commented Sep 5, 2021

Mesteery commented Sep 5, 2021

Trott commented Sep 5, 2021

Mesteery commented Sep 5, 2021

targos commented Sep 5, 2021

Trott commented Sep 5, 2021 • edited Loading

nodejs-github-bot commented Sep 5, 2021

Trott commented Sep 5, 2021

Trott commented Sep 5, 2021

aduh95 commented Sep 5, 2021

Trott commented Sep 5, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

targos left a comment

Choose a reason for hiding this comment

Trott commented Sep 6, 2021

nodejs-github-bot commented Sep 6, 2021

nodejs-github-bot commented Sep 6, 2021

github-actions bot commented Sep 7, 2021

Trott commented Sep 5, 2021 •

edited

Loading

Trott commented Sep 5, 2021 •

edited

Loading