Lombiq / NodeJs-Extensions

Utilities and extensions for Node.js, used in ASP.NET (Core) MVC and Orchard (Core) CMS development.
BSD 3-Clause "New" or "Revised" License
7 stars 2 forks source link

Lint YAML files (OSOE-188) #11

Closed 0liver closed 8 months ago

0liver commented 2 years ago

The number of YAML files we are writing constantly increases, mostly in our GitHub-Actions repository.

Let's lint them and fix common issues before code review happens and to be consistent.

This ESLint plugin looks like a decent choice, but there may be others.

Jira issue

Piedone commented 2 years ago

This should perhaps be addressed not from NE but a new project, to allow standalone YAML-using projects like our GitHub Actions to utilize it too (similar to what we've done with PowerShell Analyzers and Utility Scripts. However, if an ESLint plugin is the way to go then there's no point in reinventing the wheel and indeed it should be part of NE.

0liver commented 1 year ago

Do you think this is still viable, @BenedekFarkas, @Piedone, @DAud-IcI ?

BenedekFarkas commented 1 year ago

Yes, I'd love to have it, but I'd prefer a standalone feature, not tied to OSOCE or this repo, like Zoltán said.

sarahelsaig commented 1 year ago

I've looked up possible alternatives. There is yamllint, which is a Python-based application. There are already existing workflows that wrap this tool on the marketplace, but I'm not sure how configurable that is. There is also yaml-lint, which is Typescript-based. Both are CLI apps that can be easily installed from their respective package managers which are preinstalled in the GHA worker images. So they could be used from LGHA instead of NE. I've tried out both on the workflows in OSOCE/.github/workflows. The Typescript tool gave me no warnings, so I guess that's just a basic syntax validator and can be dismissed. The Python app gave several warnings. It can also be customized via configuration file. image

@0liver could you run the ESLint plugin you suggested on the same files, to compare what warnings are produced by it? Whichever gives the richest critique should be used.

Piedone commented 1 year ago

A standalone CLI tool would be great, even better if you don't have to install Python or any other dependency that we don't already use. While the final check here would be in GHA, and that's how we'd ultimately enforce rules, you should also be able to run it locally without much hassle.

0liver commented 1 year ago

Interesting:

image

https://www.npmjs.com/package/eslint-plugin-yml trips over strings without delimiters. So I've wrapped the names of the scripts in apostrophes, and now I get:

image

Seems pretty ... annoying to me.

0liver commented 1 year ago

Here's another one that's also available as a GitHub action: https://github.com/mikefarah/yq.

I'll give it a shot.

UPDATE: yq is not a exactly a linter - it's a yaml processor. Running yq -I 2 -i .\.github\workflows\build-and-test.yml, which is the command to enfore a 2-space indentation on the given file in-place, it produces the following diff:

image

Let's call that not useful for our use case 😏

0liver commented 1 year ago

Here a some more tools: https://faun.pub/cli-tools-for-validating-and-linting-yaml-files-5627b66849b1.

Piedone commented 11 months ago

https://prettier.io/ can be used to check YAML formatting, but I don't know if it lacks what a full linter may provide. Related: https://github.com/Lombiq/NodeJs-Extensions/issues/70

Piedone commented 11 months ago

As the first use case of this, lint Lombiq GitHub Actions and the other YML files in OSOCE. We'll need to settle on a quote style, for example.

BenedekFarkas commented 11 months ago

The unofficial VS Code plugin for GitHub Actions uses https://github.com/redhat-developer/yaml-language-server, the official one uses https://github.com/actions/languageservices, so we should try to use the latter to get consistent results.

tteguayco commented 10 months ago

In order to better estimate this task, I need to narrow down the scope a little bit. The discussion here got extended, and the following are subject to be tackled:

  1. Implement a YAML linter tool that we would ideally develop as a standalone new project/repo. Also ideally, as a CLI tool that we can run locally.
  2. Some previous research and explorations of tools need to be made first. Some options recently added under the public issue are:
  3. Use this new linter tool to Lombiq GitHub Actions and OSOCE to linter all YAML files found in them.
  4. The linter should run from GHA whenever new code is pushed to any of the above-mentioned solutions (OSOCE or Github-Actions). Prevent new code from being pushed if the linter outputs errors according to some pre-defined rules.

Do we want for now to implement this standalone tool and be able to run it locally on OSOCE or Lombiq GHA and then integrate it into our GHA pipelines in a future issue? Or tackle these two together as part of this issue?

tteguayco commented 10 months ago

On that note, I've been trying out yamllint as @sarahelsaig suggested above. And it looks quite promising. Not only provides a good output, but also gives us the chance to define a configuration file with the rules that better suit our needs.

If Docker is an option, we could use this image or build our own to bypass the hassle of setting it up with Python. Is Docker something we've run before in our GHA pipelines?

sarahelsaig commented 10 months ago

Docker is definitely an option. We use it in our setup-sql-server action to install SQL Server on Linux runners (here). Though I'm concerned that the image you linked just belongs to some random Docker community member and not official.

tteguayco commented 10 months ago

Docker is definitely an option. We use it in our setup-sql-server action to install SQL Server on Linux runners (here). Though I'm concerned that the image you linked just belongs to some random Docker community member and not official.

Yeah, we could build the image on our own.

Piedone commented 10 months ago

Thanks for summarizing, Tegua!

Two adjustments to your points:

  1. To clarify, we don't want to implement a tool, but rather, use an existing tool in a way that's mostly suitable for our local development workflow (like Node.js Extensions integrating into MSBuild and being able to run its PNPM tasks directly too) as well as for our CI workflows (NE again does this with its MSBuild integration). If both can't be addressed equally well, then we should focus on the CI workflows, like with PowerShell, see below. Ideally, this tool wouldn't need us to install any new environment, like Python.
  2. You can't prevent new code from being pushed (with a GHA workflow) but that's not the goal either, we just want a CI build. For LGHA, the *this* workflows are like this, and for OSOCE, this can be part of build-and-test.

Docker is definitely an option, though building our own image seems an overkill. For CI though we most possibly wouldn't need it, since GHA images already contain everything we may need for the environment, including Python. The linter tool itself will need to be installed via whatever package manager it uses on the fly, but that's no problem if it's just a few seconds.

Also: an inspiration can be how we do PowerShell static code analysis with https://github.com/Lombiq/PowerShell-Analyzers. That's also available as a local tool as well as a CI workflow (and used in OSOCE here). But since the local tool needs manual execution via the CLI (instead of automatically integrating into an IDE or MSBuild, the latter which of course wouldn't be applicable to projects just containing scripts) people rarely use that and only rely on the CI's results. This is fine, since the volume of PS we write compared to C# is small (and YML is smaller still).

BenedekFarkas commented 10 months ago

To expand on that, tightening the feedback loop (integrating the linting into the editor) is important for the developer experience, so having it available as an external tool is great, but less convenient to use. In the case of PS analysis, you can run the analysis on any file/folder (as an external tool), but you can also apply the same analyzer rules to VS Code (I just updated our internal instructions with that), so you see the warnings immediately.

As for YAML linting, I would assume that the linting tool/logic itself makes less of a difference (although I'd rather use the same as what the IDE uses to get the same results for sure) and the schemas you validate against matter more. VS Code reads schemas from https://www.schemastore.org/api/json/catalog.json, including those related to GitHub Actions (search for github- in that file). This has the added benefit of providing autocomplete suggestions. So as long as we're using these schemas for GHA-related YAML files, we're probably good and will have consistent results.

tteguayco commented 10 months ago

Thanks a lot for the answers above, I can say that everything is much clearer.

The only dots I'm not able to connect yet is what @BenedekFarkas mentioned at the end of the response above. So, we not only want a tool that helps standardize coding styles when writing YAML files (inspecting things like key duplicates, indentation levels, etc), but also a tool that provides intellisense capabilities when coding such files from an editor like VSCode (and YAML files that are specific to GHA in this case)?

yamllint recommends a bunch of extensions for editors in its official website here, including one for VSCode. Although the latter doesn't seem to offer anything to include schemas and/or provide the mentioned intellisense capabilities.

BenedekFarkas commented 10 months ago

Not exactly, what I meant is that choosing an option that is also compatible with IDE-integration makes it more convenient to use locally as a developer.

That is important, because having to run an external tool separately from your normal development workflow is a loss of productivity. I just mentioned IntelliSense as a nice extra for the developer experience, but that's directly relevant for automated linting.

What we use for automated linting doesn't have to be same tool as what's integrated into the IDE (whatever library the extension is using), but they need to work based on the same rules (e.g., JSON-manifest).

Piedone commented 10 months ago

For JS linting, for example, such integration is configurable like this. Something like this would be ideal, i.e. have a tool to do the lining, the same locally and in CI, and use its configuration in IDEs too.

tteguayco commented 10 months ago

After some research, I stumbled upon Trunk Check, a VSCode extension that integrates the latest version of yamllint (1.33.0).

The good thing about this extension is that it can pull the defined yamllint rules (see the whole list here) from the same configuration file we can later use when running yamllint locally or from GHA. See the below gif where linting errors are shown both in VSCode and in the command line for the file build-and-test-windows.yml (the same .yamllint.yaml file is used by both).

lint

However, it looks like this extension doesn't allow to define schemas. Regarding this, it would be great if yamllint was integrated into YAML by Red Hat, so that we could use the latter to enable IntelliSense capabilities as discussed above. Hopefully this question here may get answered.

If usage of Trunk Check is legit for our purposes, I will further investigate if it can be used along with YAML by Red Hat, so the developer experience can incorporate both linting and schema validation/IntelliSense capabilities for our YAML files.

As a sidenote, it looks like yamllint 1.33.0 is already included in the GHA image here so we can skip the installation step in the GHA runs.

tteguayco commented 10 months ago

Expanding on the above, Trunk Check allows setting a specific version of yamllint via config file:

image

Piedone commented 10 months ago

This looks great! And yes, a VS Code plugin is suitable for the local development story. BTW another plugin here is fnando.linter, which is among the ones yamllint recommends.

Piedone commented 9 months ago

Any news here, Tegua?

tteguayco commented 9 months ago

Yeap, opening a PR very soon. @Piedone