- Notifications
You must be signed in to change notification settings - Fork292
Comments
Makefile: use go run instead of globally installing Go tools#1210
Makefile: use go run instead of globally installing Go tools#1210marten-seemann wants to merge 2 commits intomasterfrom
Conversation
CLAassistant commentedJun 19, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CLAassistant commentedJun 19, 2024
|
hslatman commentedJun 19, 2024
If we make changes to this, then I would prefer the Note that much of the Makefile is cloned in pretty much all of our projects, so if we make changes here, it makes sense to do something similar in the other projects too. I think that's also the reason the tools are now installed globally: they're needed in just about every project. Another thing to take into account, and that seems somewhat related, is that we have shared CI workflows that also install tools with certain versions. It would be nice if 1) CI would install the same version as what we would install locally and 2) if we can somehow update the version of tools (centrally). I think 2 would come down to updating the |
marten-seemann commentedJun 20, 2024
Just wondering why that's a requirement to get this PR merged. As before, it uses the
Happy to propagate the change to other repos, once we've agreed on how to proceed here.
I added the |
hslatman commentedJun 20, 2024
There are tools (e.g.
I'll let others chime in on this.@azazeal,@maraino,@dopey thoughts on this? |
azazeal commentedJun 20, 2024
We should be using the version of the tools defined in |
marten-seemann commentedJun 24, 2024
My point is: Whyinstall any tools on the system anyway? We can justrun the tools, using whatever version we like, without touching the system configuration at all. Imagine the following situation: A developer has golangci-lint version X installed on their system for his work on a non-Smallstep project. Now they want to build the cli. We shouldn't change their system configuration by installing golangci-lint version Y. |
azazeal left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks reasonable. Don't know ifgo run -mod=readonly ... would be a bad idea or not. Perhaps it might be required.
| govulncheck: | ||
| $Q govulncheck ./... | ||
| $Qgo run golang.org/x/vuln/cmd/govulncheck@latest ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This shouldn't be using the@latest tag, I think.
azazeal commentedJun 24, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm not advocating that we must install said tools. I'm saying thatwhen trying to install them, we should be respectful of their versions in go.mod. Obviously, if we don't need to install them, and we can simply run them via There may be patterns lying around in these repos,@marten-seemann, that were written (or carried over) before Overall, this PR looks reasonable assuming that the Edit: There might be some muscle memory that needs to be retrained for those in the team that are used to typing Edit 2: The CI build doesn't have the same behaviorref. |
Fixes#1209. See issue for motivation.
Possible future improvements:
tools.gomethod (example) to havego.modmanage the versions of the tools. Main advantage: We could use Dependabot to upgrade to new versions, and have CI test that upgrade doesn't break anything.