Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint YAML files (OSOE-188) #11

Closed
0liver opened this issue Aug 2, 2022 · 25 comments · Fixed by Lombiq/GitHub-Actions#302
Closed

Lint YAML files (OSOE-188) #11

0liver opened this issue Aug 2, 2022 · 25 comments · Fixed by Lombiq/GitHub-Actions#302
Assignees
Labels
enhancement New feature or request

Comments

@0liver
Copy link
Contributor

0liver commented Aug 2, 2022

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
Copy link
Member

Piedone commented Aug 14, 2022

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
Copy link
Contributor Author

0liver commented Feb 14, 2023

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

@BenedekFarkas
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Piedone commented Feb 14, 2023

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
Copy link
Contributor Author

0liver commented Feb 15, 2023

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
Copy link
Contributor Author

0liver commented Feb 15, 2023

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
Copy link
Contributor Author

0liver commented Feb 16, 2023

@Piedone Piedone added the enhancement New feature or request label Mar 6, 2023
@tteguayco tteguayco self-assigned this Oct 31, 2023
@Piedone
Copy link
Member

Piedone commented Nov 5, 2023

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: #70

@Piedone
Copy link
Member

Piedone commented Nov 6, 2023

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
Copy link
Member

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
Copy link

tteguayco commented Nov 23, 2023

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

Piedone commented Nov 23, 2023

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
Copy link
Member

BenedekFarkas commented Nov 23, 2023

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
Copy link

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
Copy link
Member

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
Copy link
Member

Piedone commented Nov 27, 2023

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
Copy link

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
Copy link

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

image

@Piedone
Copy link
Member

Piedone commented Dec 1, 2023

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
Copy link
Member

Piedone commented Dec 13, 2023

Any news here, Tegua?

@tteguayco
Copy link

Yeap, opening a PR very soon. @Piedone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants