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

Switch go_modules from getUpdatedVersion to a public go command #3599

Closed
jeffwidman opened this issue Apr 29, 2021 · 3 comments
Closed

Switch go_modules from getUpdatedVersion to a public go command #3599

jeffwidman opened this issue Apr 29, 2021 · 3 comments
Labels
T: bug 🐞 Something isn't working

Comments

@jeffwidman
Copy link
Member

jeffwidman commented Apr 29, 2021

See discussion here: #3590 (comment)

In particular:

there's a lot of custom code whose sole goal is to identify what the latest version of a go module. That's the only reason we need https://github.com/dependabot/dependabot-core/blob/main/go_modules/helpers/updatechecker/main.go which is the only reason we need an entire repo: https://github.com/dependabot/gomodules-extracted/.

And @thepwagner pointed out:

I think something like go list -m -mod=mod -versions -json <dep> is a better solution than the current getUpdatedVersion helper. I've experimented with it here: https://github.com/thepwagner/action-update-go/blob/2ba2b9a3d49a3285b327622fbd5daccb4fe64e19/gomodules/check.go#L141 .

The one thing to note is that go list, go get etc download modules. I'm not sure the existing getUpdatedVersion does that. I haven't looked into how it works. But from a speed/network chattiness, avoiding downloading modules is a big win. Although on the flip side, from a correctness point of view, public APIs like go list, go get etc will probably do a better job of navigating any issues such as retracted versions, pre-release versions, etc.

Also the guts of how all that upstream code works, regardless of whether accessed via getUpdatedVersion or go list and friends, is going to change a lot in go 1.17 when lazy module loading lands. That may also greatly affect correctness/speed, unclear at this point.

Fixing this would implicitly fix #3569.

@thepwagner
Copy link
Contributor

This was investigated and decided against (or at least: deferred) in #3630

@jeffwidman
Copy link
Member Author

Completely agree that given the current behavior this isn't the right move.

However, this is probably worth holding open until golang/go#36460 lands, as it may change the behavior. See #3630 (comment).

@jeffwidman
Copy link
Member Author

This was done as part of #4434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug 🐞 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants