feat: add pre-commit hook for validator #1002
No reviewers
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Depends on
Reference
forgejo/runner!1002
Loading…
Reference in a new issue
No description provided.
Delete branch "Freso/forgejo-runner:pre-commit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This will allow users to validate their Forgejo Actions files (both actions and workflows) prior to committing them to their repositories, using a
pre-commitconfiguration similar to@ -0,0 +4,4 @@entry: forgejo-runner validateargs: ["--repository", "."]language: golangfiles: ^(\.(forgejo|github)/workflows/[^/]+|action)\.ya?ml$some users my still use
.giteafolderFixed!
This is an excellent improvement 🙏 I did not know about that pre-commit technique.
Now... the not so easy part will be to add testing for it, to verify it works as it should. I assume you tried it manually already and I'm sure it works fine. But it needs automated testing to ensure no regression sneaks in later.
Let me know if you need help with that.
My intent of making this PR was a big motivation for forgejo/forgejo-actions-feature-requests#48 – since the current footprint is way larger than it needs to be, esp. if one wants to integrate
pre-commitchecks into an action or otherwise CI it, and possibly not able to cache the built thing (e.g., Forgejo Actions on Codeberg). :)I guess one (simple) test would be to validate the file against the
pre-commit-hooks(JSON) schema.Another test could be to manually invoke
pre-commit run forgejo-runner-validateon some test files and check that we get the expected pass or fail, though I’m not entirely sure how I’d go about this.I assume
forgejo-runner validatehas its own battery of tests, so there’s no need to duplicate those here. I’m not sure how/if other projects testpre-commit-hooksfiles.I’ll try and expand the PR with at least a test validating the hooks file against the schema. Maybe it’ll be more clear to me how to add more/other tests in the process of doing this.
1f6fdbeb3c418232196b418232196bd5bb189011d5bb189011a87970e187a87970e1878cda0aeceeOkay, there’s now a “validate pre-commit-hooks file” job in the action workflows.
I guess one way to additionally test its functionality could be to make some test action/workflow files (in
.forgejo/testdata?), and then expand the new job to something like:mkdir forgejo-runner-validate-testgit initpre-commit install-hookscp -R ../.forgejo/testdata/pre-commit-hook/good/* ./git add .git commit -m 'test commit'cp -R ../.forgejo/testdata/pre-commit-hook/bad/* ./git add .git commit -m 'test commit'I’m not really sure how or if to take the automated testing further, so any suggestions for what additional testing needs to happen would be welcome!
#1002/files is a good start. 👍
That sounds perfect. You can obtain the runner compiled from the pull request like so:
8cda0aecee/.forgejo/workflows/test.yml (L84-L89)Okay, so, uh… What exactly do you want tested? Just that a valid workflow passes and a non-valid workflow fails? Do you want
action.ya?mltested too? Different file names? I’m a bit overwhelmed by the possibilities, hence why I asked for more guidance on how/what to test further. 🙇Another (additional?) option could be to add
/.pre-commit-config.yamlto run thepre-commithook on this repository itself, which would hopefully also help with catching any regressions.That won’t really help with the
pre-commithook, since that’ll want to build it itself in its own “container”-like thing. (Again, another reason for forgejo/forgejo-actions-feature-requests#48 :) )Yes, that would be perfect. After this is done, more refinements can be added but that would be proof enough that the hook actually works. It would, for instance, guard against a regression where someone editing the hook in the future changes:
to
by accident.
Note that once this is merged it should also be documented somewhere in https://forgejo.org/docs/next/user/actions/troubleshooting/ and a new section at https://forgejo.org/docs/next/user/actions/advanced-features/. I think it deserves to be the first section there, it is going to help a lot.
forgejo-runner validateto run on files in repository without cloning it #51During additional testing to get the automated testing working… I realised some unwanted behaviour (or lack of behaviour): forgejo/forgejo-actions-feature-requests#51 – so I’ll probably consider this blocked by that and leave this PR be until forgejo/forgejo-actions-feature-requests#51 is implemented. :)
feat: Add pre-commit hook for validatorto WIP: feat: Add pre-commit hook for validator [blocked]You can
validate --clonedir . --repository .to avoid cloning when at the root of the repository.forgejo-runner validateto run on files in repository without cloning it #518cda0aeceeab4f2549c5WIP: feat: Add pre-commit hook for validator [blocked]to feat: Add pre-commit hook for validatorab4f2549c5967a8005ed967a8005ed4c107493ca4c107493cab21e5e4bbeb21e5e4bbe23f4d991bd23f4d991bd90418423fa90418423faa289965b5eIs
forgejo-runnersupposed to build and install correctly usinggo install ./...at the root of the repository?@ -225,0 +250,4 @@- name: validate .pre-commit-hooks.yamlrun: pre-commit validate-manifest .pre-commit-hooks.yamlYou can obtain the runner compiled from the pull request by inserting those steps here:
8cda0aecee/.forgejo/workflows/test.yml (L84-L89)you can probably move it to
/usr/local/binso it is in the PATHYes, but that’s not how
pre-commitworks (or is supposed to work), at least for non-language: system. One of the ideas ofpre-commitis that developers don’t need to manually install all the tools for the checks themselves, but rather havepre-commithandle that part.For
language: golanghooks,pre-commitwill build and install them usinggo install ./...:I tried running
go install ./...locally and didn’t find anyforgejo-runnerexecutable afterwards, hence my question whether that is supposed to work here.Thanks for explaining so patiently, I'm learning a lot 😁
I tried and found that it is installed under the name
runner.Yahaha! Found it! 🌱
Thank you! :)
@ -0,0 +5,4 @@# Hopefully there will be a less hacky way to do this in the future :)# https://code.forgejo.org/forgejo/forgejo-actions-feature-requests/issues/51args: ['--repository', '.', '--clonedir', '.']language: golangAlthough that would work fine, it seems to require a lot of resources when validating workflows/actions in environments where Go is not in use at all.
Wouldn't it be lighter to instead use something like script that downloads the required version of the runner if not already available?
It is just a question: as long as it works it is fine by me.
I’m considering adding both a
-scriptand a-systemvariant (see e.g., howactionlintprovides a-dockerand a-systemin addition to thelanguage: golanghook), but sticking to the core concept for now/for this PR, to not have it balloon too much. :)a289965b5e00311f054500311f05452bd48807f82bd48807f83944762bd13944762bd1b2f0c2b7a3b2f0c2b7a3b4151f51b4b4151f51b41da07b5acf1da07b5acf4f400d6d8b4f400d6d8b66b2ab0f74Okay, seems like everything’s working on the
pre-commitside now!Two issues which I believe lie with
forgejo-runner validate:validatealways exits with code 0 (success) regardless of whether validation succeeded or failed, which in turn means thatpre-commitdoesn’t get a signal that it didn’t validate. As a workaround, I’ve set the hook to always beverboseso end-users can at least see the validate output in case anything doesn’t pass.validatedoesn’t only checkaction.ya?mlin the root of the repository, but rather checks anyaction.ya?mlfile in the repository. This causesforgejo-runner validateto fail (well, except for point 1. :) ) on https://code.forgejo.org/forgejo/runner since there is a deliberately badaction.ymltestdata file (act/runner/testdata/local-action-fails-schema-validation/action/action.yml).I’m not quite sure what the best way to deal with this is, but this is unlikely to be an issue for a lot of repositories besides this one, so it can be worked around on a local basis where/when/if needed.
I can open separate issues for these (if there aren’t any already), but I think these are outside the scope of this PR.
66b2ab0f742214ac4e36feat: Add pre-commit hook for validatorto feat: add pre-commit hook for validatorYes, because actions can exist in sub-directories and there really are no rules to figure out where they are. I think it is fine if the pre-commit hook does not work for the runner repository itself as a first implementation. And this is likely the only repository where a deliberately corrupted action is to be found 😁
@Freso wrote in #1002 (comment):
I will send a fix for that today.
2214ac4e36c9498b877dvalidatecomplains about YAML merge keys (<<) #1011c9498b877dba17b0dde6PR for documentation additions/updates: https://codeberg.org/forgejo/docs/pulls/1495