[CICD] Move gofumpt check to CICD, add cache

[mikeland73]

Summary

This mimics how we handle failures in our opensource repo. The CICD logic exists in the github action workflow.

• We install released devbox action, with cache
• We cache go files
• Run lint and gofumpt via devbox

Extra notes:

• In theory golangci-lint suppots gofumpt but I could not get it to work properly.
• I want to test out an experimental runx flow that could eliminate the need to install gofumpt.

How was it tested?

devbox run lint

plus

CICD

[gcurtis]

Why move it into the workflow where it can't be run locally?

[mikeland73]

Why move it into the workflow where it can't be run locally?

gofumpt can still be run locally. I just moved the if CICD logic to CICD.

[gcurtis]

@mikeland73 I was thinking that respecting CI in devbox.json would be a convenient way to debug exactly what GitHub runs in situations where the behavior differs. In this case we pass the -extra flag when not in CI because it's supposed to "be vetted by a human".

If you want to use git diff --exit-code could that just replace the body of the if in devbox.json?

[savil]

go.sum? We don't have a go-workspace in devbox.

[mikeland73]

good catch. My copy paste bad.

[savil]

suggestion:

key: ${{ runner.os }}-devbox-go-${{ hashFiles('go.work.sum') }}
restore-keys: ${{ runner.os }}-devbox-go-

https://github.com/actions/cache/blob/main/caching-strategies.md#using-restore-keys-to-download-the-closest-matching-cache

This may make it faster in the event of a cache miss, since it'll use the pre-existing "closest" cache as a starting point and then download the handful of changed dependencies to update the cache.

[mikeland73]

Agree with using restore keys, but not with adding devbox-go-. (I would just use a generic go or cache-go like the example you shared). This would allow sharing between go projects if they happen to have identical dependencies. (though that is rare)

[savil]

oops, I missed the discussion with Greg. Approved modulo what y'all decide about moving the code or not.

[mikeland73]

@gcurtis I was hoping to keep devbox.json simpler and easy to parse by a human. Putting a large bash script in makes it harder to read, and if we start doing that for all scripts it's gonna get really long.

One alternative I'm ok with is to put it in bash script on it's own, but that obscures what the devbox.json is doing.

Agree that being able to test CICD locally is good, which I think is the point of using devbox run. I'm not convinced that adding it to devbox.json is worth the complexity, since testing it locally only requires you to confirm that fmt caused not changes by doing git diff which seems simple enough.

Regarding --extra I'm not sure how I feel about CICD and local versions doing different things/checks. I think we either decide a rule is important and we enforce it or we don't, otherwise we end up with exceptions. FWIW, I assume these formatters are safe enough to never actually introduce something that would change logic or cause something not to compile.

[mikeland73]

@savil @gcurtis I need to revert the CICD changes in this repo because of a bug in the nix version golangci-lint which is preventing it from working with go1.21

Since we don't see to have consensus on the devbox.json I'll just move it to a script if that's ok with @gcurtis

[gcurtis]

I think the diff from this PR disappeared?

I like git diff --exit-code. If you use that in devbox.json it'll simplify it quite a bit. The complexity for CI needs to go somewhere, and I'd argue it's better to have it all in devbox.json instead of the YAML. I also think putting it in a separate script would obscure what it's doing, but putting it in the YAML that can only be run in GitHub obscures it more. I'd be fine with compromising on putting it in a separate script.

Regarding --extra I'm not sure how I feel about CICD and local versions doing different things/checks. I think we either decide a rule is important and we enforce it or we don't, otherwise we end up with exceptions.

Having exceptions isn't necessarily bad. I run golangci-lint locally with rules that generate lots of false-positives but sometimes catch bugs. They're too unreliable to run in CI, but still useful for a human. If you really don't like the -extra then I'm okay with removing it. My point was more that the logic between CI and local differ (CI does a diff and local edits).

[mikeland73]

Having exceptions isn't necessarily bad. I run golangci-lint locally with rules that generate lots of false-positives but sometimes catch bugs. They're too unreliable to run in CI, but still useful for a human. If you really don't like the -extra then I'm okay with removing it. My point was more that the logic between CI and local differ (CI does a diff and local edits).

I think having stricter checks that we run locally is fine, but if we explicitly run a check on CICD, having the same command do something differently on your machine can be confusing. My main holdup is doing 2 different things depending on the environment.

e.g. I would suggest having lint-strict or something like that.