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

trying out govuln #193

Merged
merged 12 commits into from
Oct 3, 2022
Merged

trying out govuln #193

merged 12 commits into from
Oct 3, 2022

Conversation

michaelneale
Copy link
Contributor

Go has build in vuln management - thought it may be worth a try

https://go.dev/blog/vuln

@michaelneale
Copy link
Contributor Author

@decentralgabe not familiar enough with permissions to know how to try tweaks from a fork to workflows (I would hope it doesn't allow them)

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

thanks @michaelneale this is great to add. I think it should be done via mage, perhaps a mage vuln check?

assuming that if it finds a vuln it'll return an error code and fail CI

@michaelneale
Copy link
Contributor Author

yes I think could be part of mage (still not sure if govuln should be run earlier or later, need to read more about how it works). If its always quick, makes sense to have it earlier before tests (or in parallel perhaps?)

@decentralgabe
Copy link
Member

yes I think could be part of mage (still not sure if govuln should be run earlier or later, need to read more about how it works). If its always quick, makes sense to have it earlier before tests (or in parallel perhaps?)

parallel would be nice, but the pipeline isn't too long now a days, so I don't mind it going first, last, or wherever!

@decentralgabe
Copy link
Member

Realized this relies on go 1.18. We're still on 1.17, which is pre-generics support. Given that 1.19 is out it seems reasonable to bump to the latest. Though I'm still a little hesitant to start introducing generics 🤔

Opened #198

@decentralgabe
Copy link
Member

ok, #198 is done - after mage this should be good to go

@michaelneale
Copy link
Contributor Author

@decentralgabe ah thanks. Hadn't had a chance to look at this again. This tool seems nice - it won't return an error if there are informational only warnings (which there are at the moment - it knows that code isn't used). I think it could run parallel to build step perhaps? or do you like it how it is.

@decentralgabe
Copy link
Member

@michaelneale let's go for parallel!

@michaelneale
Copy link
Contributor Author

@decentralgabe ok - try it again (in parallel - doesn't need to run the build to scan).

@decentralgabe
Copy link
Member

nice @michaelneale looks like it works

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

approval pending mageification of govuln

@michaelneale
Copy link
Contributor Author

@decentralgabe not sure what you mean by mageification?

@decentralgabe
Copy link
Member

decentralgabe commented Sep 20, 2022

@michaelneale call go vuln via mage like

func Vuln() error {
	fmt.Println("Vulnerability checks...")
	return sh.Run("govulncheck", "./...")
}

here: https://github.com/TBD54566975/ssi-sdk/blob/main/magefile.go

then access via

mage vuln

@michaelneale
Copy link
Contributor Author

@decentralgabe ah thanks. Trying it again with mage - letting it install govulncheck on demand is probably a good way to go to keep it fresh too (and easier for end users).

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

nice - thanks for making this change

@decentralgabe
Copy link
Member

Screen Shot 2022-09-27 at 8 30 35 AM

Hmm looks like the binary isn't found on the path 🤔 : https://github.com/TBD54566975/ssi-sdk/actions/runs/3120487785/jobs/5094376835

@michaelneale
Copy link
Contributor Author

Oh that’s annoying. is that the correct way to install it to ensure it’s available?

@decentralgabe
Copy link
Member

I thought what you had should work. let me investigate

@decentralgabe
Copy link
Member

@michaelneale I pushed a fix to your branch michaelneale#3

threw in some other magefile tidyings, couldn't help myself 😉

@decentralgabe
Copy link
Member

decentralgabe commented Sep 27, 2022

the issue is the command needs go install but the magefile was using go get

upon some further reading it seems like maybe go get should be removed entirely in favor of go install

@decentralgabe decentralgabe merged commit 87daeb8 into TBD54566975:main Oct 3, 2022
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.

2 participants