Skip to content
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

Add github actions for go fmt, test #21

Merged
merged 1 commit into from
Nov 8, 2021

Conversation

mnm678
Copy link
Contributor

@mnm678 mnm678 commented Nov 2, 2021

Solves #16

go.mod Outdated
@@ -4,7 +4,6 @@ go 1.17

require (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we need a go.mod here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, this was messing up go fmt, but I guess it does belong in a different pr

@mnm678 mnm678 force-pushed the github-actions branch 3 times, most recently from 954ffb6 to e29dbc0 Compare November 8, 2021 20:28
@mnm678
Copy link
Contributor Author

mnm678 commented Nov 8, 2021

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

You can see the workflow in action here: mnm678#6. It fails on newer go versions, which I'll fix in another pr.

@mnm678 mnm678 merged commit 449a8c9 into notaryproject:main Nov 8, 2021
@sudo-bmitch
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

mnm678 commented Nov 10, 2021

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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants