Support system-installed yamlfmt

[nikolay]

There is no need for the Go compiler to use a YAML formatting hook! Your tool could have already been installed in the system using various means. support -system like many other hooks.

[braydonk]

Is there an example of a hook, either here or in some other source that has the pattern of support you want? You can make system-installed yamlfmt work with a locally defined system hook like this:

repos:
- repo: local
  hooks:
  - id: yamlfmt
    name: yamlfmt
    language: system
    entry: yamlfmt # path to yamlfmt binary
    types: [yaml]

Maybe there is a way for a system hook to work from an actual pre commit repository, maybe by creating a mirror that tracks a build in source control, but if there is a way to make the hook in this repo work as a system hook I wasn't able to figure it out.

See #58 for the discussion from when I added pre-commit support.

[nikolay]

Here's the terraform-docs pre-commit hook, which supports 3 different methods of using/installing the binary: https://github.com/terraform-docs/terraform-docs/blob/master/.pre-commit-hooks.yaml

I understand I can create the hook myself, but it's always good to just source to your hook definition and just configure it locally.

[braydonk]

I remembered the reason I didn't do this at the time. I wanted to submit it to the pre-commit.com website's list of supported hooks, but they will not accept system hooks to that list. I don't want to need to remove this repo from that list where I already put in the effort to get it added.

Best thing to do to get this functionality would probably be to host another repo to store a yamlfmt system hook rather than it living in this repo. If all a system hook in a GitHub repo does is assume that the binary is installed on the path, then realistically all the repo has to be is probably a .pre-commit-hooks.yaml with just this:

- id: yamlfmt
  name: yamlfmt
  language: system
  entry: yamlfmt
  types: [yaml]

Most likely that should be all that's required, given how the terraform-docs-system hook appears to work.

[nikolay]

Okay. By the way, @braydonk, this is the error I get today (I'm on Go 1.21):

[INFO] Installing environment for https://github.com/google/yamlfmt.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/Users/[REDACTED]/.cache/pre-commit/repocme4j91k/golangenv-default/.go/bin/go', 'install', './...')
return code: 1
stdout: (none)
stderr:
    go: downloading github.com/mitchellh/mapstructure v1.5.0
    go: downloading github.com/braydonk/yaml v0.7.0
    go: downloading github.com/RageCage64/multilinediff v0.2.0
    go: downloading github.com/bmatcuk/doublestar/v4 v4.6.0
    go: downloading github.com/google/go-cmp v0.5.9
    error obtaining VCS status: exit status 128
    	Use -buildvcs=false to disable VCS stamping.
Check the log at /Users/[REDACTED]/.cache/pre-commit/pre-commit.log```

[nikolay]

@braydonk I submitted a PR. Normally, the ID of the Go hook must have been yamlfmt-go, but this could break things for some, so, I didn't touch it.

[braydonk]

The buildvcs failure seems to be an issue with precommit itself. I found someone was able to do this to resolve it.

I will open an issue in the pre commit dot com repo and ask if they will still accept a hook in their list if one of the hooks is system. If they are okay with that then I will accept the PR.

[braydonk]

pre-commit/pre-commit.com#888 confirms that I will not remain on the pre-commit.com supported hooks list if I add a system hook. In that issue, the pre-commit creator/maintainer suggests that you are able to use the yamlfmt hook but override your own settings locally to be system. I don't really know how that works, I'm not a pre-commit user myself. But if it works such that you can override the language to be system and provide an entrypoint, then I think it should work the way you're intending.

I will be closing this issue since I now know for sure that adding a system hook to this repo will mean I can't be on pre-commit.com. So I won't accept it upstream.

[nikolay]

You're wrong! First, you say you're not using pre-commit, but we do! Many hooks listed there also come with system language like terraform-docs, which I mention here, as an option.

[braydonk]

https://github.com/terraform-docs/terraform-docs/blob/master/.pre-commit-hooks.yaml is not in https://pre-commit.com/hooks.html, which is the list that I am referring to. The maintainer of pre-commit has told me in pre-commit/pre-commit.com#888 that system hooks aren't accepted to that list. Perhaps there are some that I'm not seeing in that list because they were added to the repo after being added to this list. But the general rule is that system hooks aren't supposed to be there. Since I want to be on that list, I will not add a system hook to this repo. A system hook for yamlfmt can be maintained somewhere other than this repo.

[nikolay]

@braydonk Okay, my bad. The PR template also bans scripts, but the mentioned terraform_docs hook uses that extensively. You're operationalizing this. "Generally yes" and "Yes" in the response differ significantly in their meaning. If not, most hooks would have been long gone from that list!

And, honestly, which is more important to you: a) DX of your users or being listed in some repo nobody really cares about, which only serves vanity and has almost zero practical or marketing value?!

[braydonk]

My understanding is that list is designed to be a list of supported hooks particularly for folks using pre-commit.ci. Maybe that's not exactly the point. But either way I would like to stay on that list. I had to do work to make sure I conformed to the rules of that list already. Just because others have snuck through with unsupported hooks doesn't mean I want to go now do the same thing when the maintainer has suggested that, generally, system hooks are not excepted (perhaps we fall under some exception, I am not sure if the "generally" there is carrying some extra context).

I tried the following to use the existing hook as a system hook:

1. Created a Debian 12 VM and installed pre-commit but not Go.
2. Create a temporary Git repository with one yaml file a.yaml that will definitely be changed by the yamlfmt hook due to being formatted different than defaults.

a:
 b: 1 # one space indent will be changed to 2 by default

4. Download the latest release of yamlfmt from Releases and put it in /usr/bin (or somewhere in the path).
5. Create a .pre-commit.config.yaml in the repo:

repos:
  - repo: https://github.com/google/yamlfmt
    rev: v0.10.0
    hooks:
      - id: yamlfmt
        language: system

6. Run pre-commit install
7. git add a.yaml
8. pre-commit run --all-files

braydonk@test-pre-commit:~/tmp$ pre-commit run -v --all-files
yamlfmt..................................................................Failed
- hook id: yamlfmt
- duration: 0.01s
- files were modified by this hook
braydonk@test-pre-commit:~/tmp$ git diff
diff --git a/a.yaml b/a.yaml
index 3092de1..28f410a 100644
--- a/a.yaml
+++ b/a.yaml
@@ -1,2 +1,2 @@
 a:
- b: 1
+  b: 1

As a result I was able to successfully use the yamlfmt hook from this repo and simply overrode it to use my system yamlfmt. This should pretty much accomplish the same thing as a separate system hook. I will document this in my pre-commit hook docs.

[nikolay]

@braydonk Thanks! I didn't know it could be overridden. I didn't spend much time and only quickly browsed the hooks I've been using personally, but if I find a decent amount of other Git hooks in that official list, using system, will you reconsider the PR?

[braydonk]

What does the additional hook get you that the override doesn't?

[nikolay]

@braydonk Let's be honest here - it's a hack.

[braydonk]

I think it's a pretty reasonable solution. The only thing that actually changes between a system yamlfmt hook and a go yamlfmt hook is the language. The other settings, such as entry and file types are all the same. So it's the difference between having an actual hook called yamlfmt_system and using the yamlfmt hook along with language: system. I'm not seeing the value of it being a separate hook when the functionality is identical unless I'm missing something.