-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Jinja syntax pre-commit validation #10667
Jinja syntax pre-commit validation #10667
Conversation
Hi @VannTen. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hum I was under the impression than pre-commit hooks and CI jobs were automatically synced. But they are not. |
4fa9909
to
44019b5
Compare
HMmm isn't ansible-lint supposed to already check that: https://ansible.readthedocs.io/projects/lint/rules/jinja/? The precommit is probably nice though but not sure about the CI job as it exec ansible-lint in the same CI stage IIRC? If you have a case where ansible-lint was not able to catch something and your new check does, happy to go forward with this as is! 👍 |
Somehow ansible-lint does not catch that sort of thing (don't know why), see the test PR CI run. |
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.
Indeed 👌 let's go for this then :D
/lgtm
/ok-to-test
/assign @liupeng0518 |
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.
@VannTen All good thank you
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: floryut, MrFreezeex, VannTen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
44019b5
to
f2d73a5
Compare
But they're identical ! ^ |
Allow to fail early (pre-commit time) for jinja error, rather than waiting until executing the playbook and the invalid template. I could not find a simple jinja pre-commit hook in the wild.
f2d73a5
to
203e6eb
Compare
Thanks @VannTen |
Allow to fail early (pre-commit time) for jinja error, rather than waiting until executing the playbook and the invalid template. I could not find a simple jinja pre-commit hook in the wild.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Allow to fail early (pre-commit time) for jinja error, rather than
waiting until executing the playbook and the invalid template.
This catch simple syntax errors like missing endif/endfor.
-> save CI and dev time
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I could not find a simple jinja pre-commit hook in the wild.
dansabel tries to check filter names so it does not really work.
Does this PR introduce a user-facing change?: