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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document that GitHub Actions already installs yamllint #588

Conversation

jeffwidman
Copy link

I was looking up instructions for how to install yamllint so that I could use it in our GitHub actions workflow when I happened to see mention that it's already installed in the base image.

So adding a quick note here.

I phrased the note generically because additional CI systems may (should!! 馃槃 ) start doing this, so we can easily add them to the list.

@jeffwidman
Copy link
Author

jeffwidman commented Aug 16, 2023

Oh crazy, I didn't realize this is literally all I need (+ a .yamllint config file for customization) to get this running in Github Actions:

---
name: yamllint
on:  # yamllint disable-line rule:truthy
  pull_request:
    branches: ["main"]

jobs:
  yamllint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3

      # yamllint is installed in GitHub Actions base runner image: https://github.com/adrienverge/yamllint/pull/588
      - run: yamllint .

We should probably document this better so that folks who want vanilla yamllint aren't first having their github action workflow download the docker image/apt-get install etc which wastes energy/bandwidth.

@adrienverge
Copy link
Owner

Hello and thanks for the proposal! You're right: yamllint is now installed by default on some platforms. However, I don't see the purpose of such a new note, especially placed before the how-to-install on popular OSes. Having this note looks more confusing to me than not having it, and we're trying to keep instructions short for clarity.

@coveralls
Copy link

coveralls commented Aug 16, 2023

Coverage Status

coverage: 99.825%. remained the same
when pulling 8744b7a on jeffwidman:document-some-CI-systems-already-install-yamllint
into 81e9f98 on adrienverge:master.

@jeffwidman
Copy link
Author

Makes sense to me about keeping the user experience short/uncluttered, so any suggestions for how to make users aware that they don't even need to install yamllint in order to run it in GitHub actions?

One of the most common places to install yamllint is in CI systems... I suspect if you could poll users for "why are you looking at the install instructions?" many would say "I'm trying to figure out how to wire it into our CI system"...

Currently I'm setting up yamllint for :dependabot:, and before I worked at GitHub, I setup yamllint multiple times at previous jobs and each time I had to hunt through the instructions trying to figure out how to install yamllint in order to get it to run. I'm sure this happens routinely for other folks as well. Plus again, since this install is happening in CI, that results in needlessly downloading / installing it hundreds of thousands of times a year vs if they knew it was already baked into the golden image it'd save human time, machine time, network bandwidth, and electricity.

If you don't want it as a "note" at the top, would you be open to a PR listing it as a another type of install? I could list it right below OpenBSD.

@andrewimeson
Copy link
Contributor

If you rely on yamllint being implicitly available in your CI environment, contributors will not get the benefits of yamllint locally unless they install it. Even if they do install it they may have a different version that produces different results than what is in CI. It's best to pin dependencies like yamllint either in a requirements.txt or requirements-dev.txt file.

@adrienverge
Copy link
Owner

If you don't want it as a "note" at the top, would you be open to a PR listing it as a another type of install? I could list it right below OpenBSD.

If this is to be merged, a small sentence at the top looks good to me 馃憤 (even if it's just a simple sentence, not in a note)

However, I'm not sure such a new paragraph adds clarity:

  • This section of the documentation is about "how to install it when you don't have it installed". I'm not sure specific instructions about specific environments are relevant here.
  • I agree with @andrewimeson's argument.
  • "yamllint is installed on GitHub Actions" seems a wrong shortcut to me, I believe it depends on the image you use, in your example it's ubuntu-latest, which is a GitHub-modified Ubuntu.
  • I don't want to maintain a list of "CI systems" in this documentation.

I'm not opposed to a very simple introduction phrase like this, if you really think it adds value:

yamllint is already installed on some systems (like GitHub Actions base image), but in other cases, here is how to install it on some popular systems.

@jeffwidman
Copy link
Author

Even if they do install it they may have a different version that produces different results than what is in CI. It's best to pin dependencies like yamllint either in a requirements.txt or requirements-dev.txt file.

There's a variety of opinions on this. I work on a product that automates bumping pinned dependencies every day, so I'm well aware of the benefits of pinning. But for dev tooling such as linters, it's quite common for maintainers to opt for the minimal maintenance overhead of "floating" latest, and only if they hit problems will they temp pin.

That's probably because the consequences of dev tooling blowing up is that typically it only kills your CI pipeline. Totally different story from needing to pin your app deps to prevent them blowing up unexpectedly in production.

Not uncommon to see smaller projects do this for pytest, flake8, ubuntu build OS versions, etc.

@jeffwidman jeffwidman force-pushed the document-some-CI-systems-already-install-yamllint branch from 9c78d3f to 745ec38 Compare May 1, 2024 22:25
I was looking up instructions for how to install `yamllint` so that I could use it in our GitHub actions workflow when I happened to see mention that it's already installed in the base image.

So adding a quick note here.

I phrased the note generically because additional CI systems may (should!! 馃槃 ) start doing this, so we can easily add them to the list.
@jeffwidman jeffwidman force-pushed the document-some-CI-systems-already-install-yamllint branch from 745ec38 to 8744b7a Compare May 1, 2024 22:26
@jeffwidman
Copy link
Author

I updated to a simple sentence.

I did link to the example, as I suspect many folks will be looking for a copy/paste example of how to run it on GitHub Actions.

It's been a bit since I worked with RST, so my apologies if I got the formatting wrong.

Also, given the lukewarm reception to this PR, no problem if you don't want this in your docs. It's really up to you.

@adrienverge
Copy link
Owner

Hello Jeff, thanks for following up and reviving this! The formatting looks good (doc8 and rstcheck pass 馃憤) Sorry about the lukewarm reception. I prefer being honest: I have the feeling that proposed new sentence in the documentation page will help only a few users, while making the documentation longer to read for many.

@jeffwidman
Copy link
Author

jeffwidman commented May 2, 2024

No problem, I'll close it. I disagree given what I've seen at multiple employers... the conversation typically goes like this:

"oh crap, another yaml bug we missed bit us"
"yeah, we should enable yamllint in CI"
"yeah, I've been meaning to do that after we got bit by a similar bug last year, we filed a ticket to do it, but no one has ever quite gotten to it..."
"did you realize it's only a 1-liner since it's already installed in github actions?"
"no, I had no idea!"

Clearly you've had a different experience though.

@jeffwidman jeffwidman closed this May 2, 2024
@jeffwidman jeffwidman deleted the document-some-CI-systems-already-install-yamllint branch May 2, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants