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

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

Closed
3 tasks
consideRatio opened this issue Nov 5, 2021 · 4 comments · Fixed by #807
Closed
3 tasks
Labels
Task Actions that don't involve changing our code or docs.

Comments

@consideRatio
Copy link
Contributor

consideRatio commented Nov 5, 2021

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: [email protected]
[error]      | ^^^^^^^^^^^^^^^^
[error] > 19 |   - name: Simon Li
[error]      | ^^^^^^^^^^^^^^^^
[error] > 20 |     url: https://github.com/manics/
[error]      | ^^^^^^^^^^^^^^^^
[error] > 21 |
[error]      | ^

To close this

@consideRatio consideRatio added the Task Actions that don't involve changing our code or docs. label Nov 5, 2021
@consideRatio
Copy link
Contributor Author

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

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task Actions that don't involve changing our code or docs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants