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

ci: add linting for yml files #22

Merged
merged 2 commits into from
Sep 11, 2024
Merged

ci: add linting for yml files #22

merged 2 commits into from
Sep 11, 2024

Conversation

dhth
Copy link
Owner

@dhth dhth commented Sep 11, 2024

No description provided.

@dhth dhth merged commit 0872535 into main Sep 11, 2024
5 checks passed
@dhth dhth deleted the add-linting-for-yml-files branch September 11, 2024 18:35
@ccoVeille
Copy link

I'm surprised. Usually yamlfmt is not used for limiting. You will find quite GitHub action for that, it may explain why you had to do the trick to install the latest version

Usually, yamlint is the one used for linting.

Any reason for that?

Comment on lines +17 to +20
- name: Get yamlfmt
run: |
LATEST_VERSION=$(curl -s https://api.github.com/repos/google/yamlfmt/releases/latest | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/' | sed 's/^v//')
./.github/scripts/get-yamlfmt.sh "Linux" "x86_64" "$LATEST_VERSION"

Choose a reason for hiding this comment

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

You could have used

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

It would have been simpler

Copy link
Owner Author

@dhth dhth Sep 12, 2024

Choose a reason for hiding this comment

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

Yeah, that's what I did in the beginning, but setting up the whole go toolchain just to able to use a binary seemed a bit wrong (after all, being able to just use them directly is a major benefit of statically linked binaries, no?).

The best solution for this would be a github action as a wrapper for the installation process. Might create that if I get the time.

Choose a reason for hiding this comment

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

I looked for yamlfmt action and it's very disappointing

https://github.com/yk-lab/yamlfmt-action

Or this disappointing one

https://github.com/credfeto/action-yaml-format

The reformat is a sed 😂

I found this one, but yes it use setup-go then install it
https://github.com/norwd/fmtya

Copy link
Owner Author

Choose a reason for hiding this comment

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

Haha, yeah, it's slim pickings for this tool.

I did find https://github.com/supplypike/setup-bin which does work for simply downloading the binary files and putting the in $PATH, but then I lose checksum and signing (if applicable) validation.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

I guess there is no silver bullet for this use case as the validation steps can be quite varied across repositories.

@dhth
Copy link
Owner Author

dhth commented Sep 12, 2024

I'm surprised. Usually yamlfmt is not used for limiting. You will find quite GitHub action for that, it may explain why you had to do the trick to install the latest version

Usually, yamlint is the one used for linting.

Any reason for that?

Well, you would use the same tool for formatting and linting, no? Between the choice between yamlfmt and yamllint, I went with the former because:

a. It's written in go, so easier to install locally, and should (theoretically) be faster
b. It's under the @google org (not that it matters too much), so hopefully will have more resources behind it (that's of course a guess on my side)

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