search

notaryproject / tuf

The Update Framework for OCI Registries
11 stars 11 forks source link

Add github actions for go fmt, test #21

Closed mnm678 closed 2 years ago

mnm678 commented 2 years ago

Solves #16

mnm678 commented 2 years ago

Can I get a review/approval @sudo-bmitch, @trishankatdatadog?

You can see the workflow in action here: https://github.com/mnm678/tuf-1/pull/6. It fails on newer go versions, which I'll fix in another pr.

sudo-bmitch commented 2 years ago

Belated review from me, but I also noticed that the actions tab didn't populate, so it got my attention.

I think the path needs to be .github/workflows/ (plural). And since the action won't modify the commit, running go fmt in the action isn't much value, we just need to require that from committers before merging PR's. Most of us do that in our IDEs automatically on save, but we could include a Makefile that does this before locally building and testing.

I can probably get a PR with these later this week since they're pretty standard in my projects.

trishankatdatadog commented 2 years ago

I think the path needs to be .github/workflows/ (plural). And since the action won't modify the commit, running go fmt in the action isn't much value, we just need to require that from committers before merging PR's. Most of us do that in our IDEs automatically on save, but we could include a Makefile that does this before locally building and testing.

go fmt here could at least help detect and prevent issues, no?

mnm678 commented 2 years ago

Thanks @sudo-bmitch, I opened #23 to add the s, but it still doesn't seem to run the action?

My thought was that the go fmt run would mostly verify that the committer ran that correctly, but a Makefile that fixes it would be even better.

sudo-bmitch commented 2 years ago

@trishankatdatadog

go fmt here could at least help detect and prevent issues, no?

That would likely go ignored, the return code is 0 even when formatting needs to be fixed. At most you see a filename that was updated in the output, which requires someone to double check that output on every run. Unless we're doing something I'm missing to ensure there's no files changed by go fmt.