2i2c-org / infrastructure

Infrastructure for configuring and deploying our community JupyterHubs.
https://infrastructure.2i2c.org
BSD 3-Clause "New" or "Revised" License
105 stars 64 forks source link

yaml_lint action clutters PRs with comments: replace it with pre-commit and a `prettier` hook #805

Closed consideRatio closed 3 years ago

consideRatio commented 3 years ago

We use the https://github.com/karancode/yamllint-github-action, which can be found to comment on PRs.

image

This is a terrible practice in my mind, and seems to be the default behavior for the action, even though its documented to not be so. Perhaps there a bug that makes the comment happens no matter what?

I suggest this action is dropped in favor of using a pre-commit hook with prettier, that auto-formats the YAML instead of just lints it. I've loved it in z2jh and other projects with big YAML files.

I've just verified also that with prettier, we would catch duplicate keys with a clear error message if they are detected.

prettier.................................................................Failed
- hook id: prettier
- exit code: 2

jupyterhub/Chart.yaml
[error] jupyterhub/Chart.yaml: SyntaxError: Map keys must be unique; "appVersion" is repeated (2:1)
[error]    1 | # Chart.yaml v2 reference: https://helm.sh/docs/topics/charts/#the-chartyaml-file
[error] >  2 | apiVersion: v2
[error]      | ^^^^^^^^^^^^^^
[error] >  3 | name: jupyterhub
[error]      | ^^^^^^^^^^^^^^^^
[error] >  4 | version: 0.0.1-set.by.chartpress
[error]      | ^^^^^^^^^^^^^^^^
[error] >  5 | appVersion: 2.0.0rc2
[error]      | ^^^^^^^^^^^^^^^^
[error] >  6 | appVersion: 2.0.0rc3
[error]      | ^^^^^^^^^^^^^^^^
[error] >  7 | description: Multi-user Jupyter installation
[error]      | ^^^^^^^^^^^^^^^^
[error] >  8 | keywords: [jupyter, jupyterhub, z2jh]
[error]      | ^^^^^^^^^^^^^^^^
[error] >  9 | home: https://z2jh.jupyter.org
[error]      | ^^^^^^^^^^^^^^^^
[error] > 10 | sources: [https://github.com/jupyterhub/zero-to-jupyterhub-k8s]
[error]      | ^^^^^^^^^^^^^^^^
[error] > 11 | icon: https://jupyter.org/assets/hublogo.svg
[error]      | ^^^^^^^^^^^^^^^^
[error] > 12 | kubeVersion: ">=1.17.0-0"
[error]      | ^^^^^^^^^^^^^^^^
[error] > 13 | maintainers:
[error]      | ^^^^^^^^^^^^^^^^
[error] > 14 |   # Since it is a requirement of Artifact Hub to have specific maintainers
[error]      | ^^^^^^^^^^^^^^^^
[error] > 15 |   # listed, we have added some below, but in practice the entire JupyterHub team
[error]      | ^^^^^^^^^^^^^^^^
[error] > 16 |   # contributes to the maintenance of this Helm chart.
[error]      | ^^^^^^^^^^^^^^^^
[error] > 17 |   - name: Erik Sundell
[error]      | ^^^^^^^^^^^^^^^^
[error] > 18 |     email: erik@sundellopensource.se
[error]      | ^^^^^^^^^^^^^^^^
[error] > 19 |   - name: Simon Li
[error]      | ^^^^^^^^^^^^^^^^
[error] > 20 |     url: https://github.com/manics/
[error]      | ^^^^^^^^^^^^^^^^
[error] > 21 |
[error]      | ^

To close this

consideRatio commented 3 years ago

The PR review will show all comments, not just the code surrounding the lines of code changed, but also the code surrounding all comments. This PR was small, but was swamped with comments.

not-helpful-comments

choldgraf commented 3 years ago

this seems pretty reasonable to me - agreed that it is confusing seeing the bot post comments all throughout the file. Does anybody have a strong objection?

damianavila commented 3 years ago

Does anybody have a strong objection?

Nop, in fact, I would be OK-ish with it (btw, I have not used pre-commit.ci before although the concept seems interesting to me).

choldgraf commented 3 years ago

I'm a huge fan of pre-commit.ci