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

Reformat yaml #2124

Closed
wants to merge 2 commits into from
Closed

Reformat yaml #2124

wants to merge 2 commits into from

Conversation

anna-parker
Copy link
Contributor

@anna-parker anna-parker commented Jun 10, 2024

Summary

This uses https://marketplace.visualstudio.com/items?itemName=redhat.vscode-yaml to format all yaml files consistently.

@corneliusroemer
Copy link
Contributor

This uses https://marketplace.visualstudio.com/items?itemName=redhat.vscode-yaml to format all yaml files consistently.

Using an extension isn't great for portability, but afaict the extension uses the same formatter as prettier under the hood - so we could use that:

@corneliusroemer
Copy link
Contributor

An alternative is to use #2130 (google's yamlfmt)

@@ -1,4 +1,4 @@
{{- $keycloakHost := printf "authentication-%s" .Values.host }}
{ { - $keycloakHost := printf "authentication-%s" .Values.host } }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks the config by adding a space between the paranthesis - I can't figure out how to stop it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that probably means we shouldn't use it

There might not be a way to fix/configure it - other than adding the folder to ignore files (which would be ok, this is not proper yaml anyways, it's a superset with templating specific grammar)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it doesn't break it because it aborts due to syntax error

$ yamlfmt kubernetes/loculus/templates/keycloak-config-map.yaml 
encountered the following formatting errors:
kubernetes/loculus/templates/keycloak-config-map.yaml: yaml: did not find expected node content

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In VSCode, it also doesn't format it, because I have the kubernetes extension installed which recognizes this file as a helm template (as opposed to yaml) and hence doesn't format with the yaml language formatter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah awesome! sorry I missed this and basically added this same comment thread to #2130

@anna-parker
Copy link
Contributor Author

Closing this as #2130 does a better job at handling helm templates - thanks @corneliusroemer for looking into this!

corneliusroemer added a commit that referenced this pull request Jun 17, 2024
### Summary
- Add yamlfmt config to root
- Recommend yamlfmt vscode extension

This formatter with current config doesn't wrap long lines, which saves us a lot of lines.

Install formatter with homebrew or:

```
go install github.com/google/yamlfmt/cmd/yamlfmt@latest
```

Blog writeup on the formatter: https://til.simonwillison.net/yaml/yamlfmt

Alternative to #2124
@anna-parker anna-parker deleted the reformat_yaml branch August 14, 2024 12: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.

2 participants