-
Notifications
You must be signed in to change notification settings - Fork 795
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
Autoformat bash scripts, yaml files, and markdown files with pre-commit #1996
Conversation
Seems good overall to me @manics, thanks for your work!
Arrrgh, I'd like to avoid having both double quote and single quote enforced in different contexts. I remember this issue with chartpress, doh! Perhaps we can coerce chartpress with a new behavior instead? I believe I have tried in the past unsuccessfully though.
Sure, they are prone to formatting errors. We are using MyST markdown, I wonder if there are some linting and or autoformatting or similarly for MyST specifically btw. I'll investigate. Update:
Update: # Markdown formatting
- repo: https://github.com/executablebooks/mdformat
rev: 0.5.5
hooks:
- id: mdformat
additional_dependencies:
# Autoformat nested Python code: https://github.com/hukkinj1/mdformat-black
- mdformat-black
# Autoformat nested json/yaml/toml: https://github.com/hukkinj1/mdformat-config
- mdformat-config
# Tolerate GitHub flavored markdown: https://github.com/hukkinj1/mdformat-gfm
- mdformat-gfm Update: mdformat-config did the following, hmm...
spec:
- storageClassName: ""
+ storageClassName: ''
accessModes:
- - ReadWriteMany
+ - ReadWriteMany |
@manics chartpress will not disrupt us after jupyterhub/chartpress#115 is merged and released. It will properly preserve no / single / double quotes for Chart.yaml / values.yaml then. |
Requested action points
Bonus action points
|
ci/common contains bash-specific syntax, and triggers several warnings
``` tools/templates/lint-and-validate-values.yaml [error] tools/templates/lint-and-validate-values.yaml: SyntaxError: Map keys must be unique; "nodeSelector" is repeated (11:3) ```
@manics is it okay if I --force push to this branch? (changes here: https://github.com/consideRatio/zero-to-jupyterhub-k8s/commits/pre-commit-2)
What would remain then:
|
Go ahead and push |
f000c75
to
9451c39
Compare
1.0.4 contains a bugfix making chartpress no longer forcefully wrap image tags set in values.yaml with single quotes.
9451c39
to
6ee24df
Compare
@manics this LGTM now, what do you think? I did a visual inspection of the markdown and is generally happy about the kind of formatting changes there, as well as in the YAML now that we have double quotes which is the default. With prettier, we also automatically get indentation within the yaml syntax highlighted code blocks! We don't get python black formatting with in the python code blocks though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me! Thanks for finishing this off.
🎉 thanks for starting it up! |
jupyterhub/zero-to-jupyterhub-k8s#1996 Merge pull request #1996 from manics/pre-commit-2
@manics it has been a great success to use prettier I think! :D I catch several syntax errors because it fails to autoformat invalid python code! ❤️ |
Closes #1899
For discussion:
shellcheck (first 3 commits): I think it should be useful, and I've configured it to ignore the current warnings from
ci/common
. It's not automatically installed though by pre-commit (in contrast to the other autoformatters where running pre-commit will auto-download it for you), so I've put it in a separate configuration file and added a second step totest_chart.yaml
to run it.It picked up one minor error-
ci/common
uses bash not sh syntax. Should we keep shellcheck, make it part of the defaultpre-commit-config.yaml
, or drop it?helmlint: should be fine
prettier (last three commits): I've excluded
jupyterhub/templates/
(not proper yaml files) and*.md
(it makes the diff hard to review so probably best left for a separate commit after this is reviewed). There was minor one error, see 662f5e8 . The other thing to note is that had to override it's default of double quotes to use single quotes instead, this is because chartpress uses single quotes when resettingjupyterhub/values.yaml
. Should we go ahead and auto-format markdown files too?