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

linters+testdata: Reformat all yaml testcases for linting. #6511

Merged

Conversation

philipaconrad
Copy link
Contributor

This PR adds a config for yamllint, as well as mass-reformats all of the existing Yaml testcases to pass linting. A few careful exceptions and ignores were added to the config to allow keeping our existing Yaml files with minimal manual reformatting.

The PR also will include an update to the GH Actions checks done during pull requests, ensuring we'll continue to get well-formatted Yaml files for our testcases in the future. 🙂

Note: All formatting seen in this maintenance PR was done by adding --- document starts where needed, and utilizing the Prettier auto-formatter for Yaml on all of the files. Manual fixups were then done for whatever issues remained, like "trailing spaces" linter errors.

@philipaconrad philipaconrad added the github_actions Pull requests that update GitHub Actions code label Jan 3, 2024
@philipaconrad philipaconrad self-assigned this Jan 3, 2024
@philipaconrad
Copy link
Contributor Author

With an intentional commit left in that had a trailing spaces error, it looks like the new "YAML Lint" target correctly finds the issue.

@philipaconrad philipaconrad marked this pull request as ready for review January 3, 2024 20:23
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Looks good to me, but if there are changes not related to quoting of whitespace I may very well have missed them. Since contributors often add tests to these files, it would be good to add this requirement to the development docs, and explain how to run the linter locally.

@philipaconrad
Copy link
Contributor Author

Since contributors often add tests to these files, it would be good to add this requirement to the development docs, and explain how to run the linter locally.

Thanks for taking a look @anderseknert! I agree that the linting process would be great to add to the docs-- fighting a linter in CI can be very frustrating. I'll go add the "run it yourself" process to the contributor docs tomorrow. 😄

Copy link

netlify bot commented Jan 4, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit bcf707b
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/659ed95d41ae57000770fe8a
😎 Deploy Preview https://deploy-preview-6511--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@philipaconrad
Copy link
Contributor Author

philipaconrad commented Jan 4, 2024

I've added an extra step to make check, which uses this popular yamllint docker image to allow local YAML linting, without needing to install yamllint manually. Docs have been updated appropriately.

That should reduce the need for contributors to have a Python3 toolchain set up locally, but maybe it makes more sense to just add a separate check-yaml target? With this iteration of the PR, YAML failures will appear under the Go Lint Github PR check, which doesn't feel quite right. 🤔

@philipaconrad
Copy link
Contributor Author

I've separated out YAML linting into its own Makefile target: check-yaml-tests. Hopefully, that'll keep Go and YAML linting errors separate.

srenatus
srenatus previously approved these changes Jan 9, 2024
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks!

This commit adds a config for yamllint, as well as mass-reformats all of
the existing Yaml testcases to pass linting. A few careful exceptions
and ignores were added to the config to allow keeping existing Yaml
files with minimal reformatting.

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
This commit moves the YAML linting logic in the Makefile to be under the
new `check-yaml-tests` target, so that the `make check` target does not
mix Go and YAML errors together.

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
@philipaconrad
Copy link
Contributor Author

Will merge when CI goes green. 👍

@philipaconrad philipaconrad merged commit 63e1877 into open-policy-agent:main Jan 10, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants