@nodejs/linting @Mesteery

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.

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

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

#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.

#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.

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

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 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.)

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

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'

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!)

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'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!

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

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

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

Landed in 773a989...b016d5d