-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Working goproxy implementation #55
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
Conversation
| max-ttl = "8h" | ||
| } | ||
|
|
||
| gomod { |
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.
Sick!
internal/strategy/gomod_cacher.go
Outdated
| // Hash the name to create a cache key that matches cachew's format | ||
| key := cache.Key(sha256.Sum256([]byte(name))) | ||
|
|
||
| // Try to open the cached content |
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.
Can we get rid of all these completely superfluous comments? I find they make it quite a bit harder to comprehend what is actually going on.
I'm a bit surprised they're showing up TBH, the AGENTS.md file has explicit instructions about it 🤔
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.
Yeah, they are actually worse than superfluous. I found several comments working on another branch yesterday that were straight up misleading or false.
Claude has a well known history of ignoring AGENTS.md whenever it feels like it. They did recent(ish)ly add .claude/rules/ like .cursor/rules/. Perhaps we can try that and see if it's better at obeying those?
internal/strategy/gomod_cacher.go
Outdated
| // The TTL is determined by inspecting the cache name to identify whether | ||
| // it represents mutable or immutable content. |
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.
Super weird, is this actually true? What is "name" in the context of the interface? Does it actually include things like /v@/list?
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.
I checked and you are 100% right...that's super weird, as the default implementation doesn't seem to do any sort of timing out of /list entries etc...
Ah, I bet I know what's going on - I bet it uses an ETag to conditionally update it.
I think that means we can actually get rid of the two different TTL types, and just rely on the ETag to manage the cache.
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.
I'm not at all convinced I'm correct about that actually, however it does seem to overwrite the list on every go get, as well as hit upstream list on every request...so there doesn't seem to be much point in having a TTL on list at all. If anything, it might be better to completely ignore /list...
DBG Request received client=127.0.0.1:56140 request="GET /gomod/github.com/alecthomas/chroma/@v/list"
DBG Request received client=127.0.0.1:56142 request="GET /gomod/github.com/alecthomas/@v/list"
DBG Request received client=127.0.0.1:56141 request="GET /gomod/github.com/@v/list"
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/alecthomas/@v/list
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/@v/list
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/alecthomas/chroma/@v/list
DBG goproxy.Get client=127.0.0.1:56141 request="GET /gomod/github.com/@v/list" name=github.com/@v/list
DBG goproxy.Get client=127.0.0.1:56142 request="GET /gomod/github.com/alecthomas/@v/list" name=github.com/alecthomas/@v/list
DBG goproxy.Put client=127.0.0.1:56140 request="GET /gomod/github.com/alecthomas/chroma/@v/list" name=github.com/alecthomas/chroma/@v/list
DBG Request received client=127.0.0.1:56142 request="GET /gomod/github.com/dlclark/regexp2/@v/list"
DBG Go module proxy outbound request method=GET url=https://proxy.golang.org/github.com/dlclark/regexp2/@v/list
DBG goproxy.Put client=127.0.0.1:56142 request="GET /gomod/github.com/dlclark/regexp2/@v/list" name=github.com/dlclark/regexp2/@v/list
Super weird.
Also interestingly, on cold start it seems to hit the upstream no matter what. Subsequent runs are quite fast, so it appears it might have some in-memory caching too.
I'm not sure what to think about it all.
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.
I checked the source and it does indeed just call the upstream. I also checked and the upstream has cache control headers with a 60 second TTL.
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.
Yeah I think we should just ignore the endpoints that were previously listed as mutable when caching.
|
I pushed up a minor fix to configure it to use our logger, so it didn't spam to stdout, however it does spam errors for every package it pulls, for some reason: Running: Gives: I think we're going to need some kind of log interceptor to filter these out 😕 |
|
Okay fixed the logging too! |
Co-authored-by: Alec Thomas <aat@block.xyz>
Co-authored-by: Alec Thomas <aat@block.xyz>
…sfptc into johnm/investigate-goproxy
|
I've stripped out the unnecessary extra junk and things are looking a lot leaner. |
What?
This replaces the custom gomod handling and embeds https://github.com/goproxy/goproxy instead.
Why?
More robust and battle tested than our current implementation. It also should give us a path to supporting private modules as well via a custom fetcher in the future.
Tests