Conversation
alecthomas
left a comment
alecthomas
left a comment
There was a problem hiding this comment.
Overall LGTM if it works! :)
| var ( | ||
| sharedManager *Manager | ||
| sharedManagerMu sync.RWMutex | ||
| ) | ||
|
|
||
| func SetShared(m *Manager) { | ||
| sharedManagerMu.Lock() | ||
| defer sharedManagerMu.Unlock() | ||
| sharedManager = m | ||
| } | ||
|
|
||
| func GetShared() *Manager { | ||
| sharedManagerMu.RLock() | ||
| defer sharedManagerMu.RUnlock() | ||
| return sharedManager | ||
| } |
There was a problem hiding this comment.
I can't tell why this exists. We should be constructing a single Manager in main, then passing it everywhere, so AFAICT we shouldn't need this?
| "github.com/goproxy/goproxy" | ||
| ) | ||
|
|
||
| type compositeFetcher struct { |
There was a problem hiding this comment.
I think a comment here describing the purpose would be good - I think it's for routing between public/private repo's though?
| fetcher = newCompositeFetcher(publicFetcher, privateFetcher, config.PrivatePaths) | ||
|
|
||
| s.logger.InfoContext(ctx, "Configured private module support", | ||
| slog.Any("private_paths", config.PrivatePaths)) |
There was a problem hiding this comment.
| slog.Any("private_paths", config.PrivatePaths)) | |
| slog.Any("private-paths", config.PrivatePaths)) |
| type ModulePathMatcher struct { | ||
| patterns []string | ||
| } | ||
|
|
||
| func NewModulePathMatcher(patterns []string) *ModulePathMatcher { | ||
| return &ModulePathMatcher{patterns: patterns} | ||
| } | ||
|
|
||
| func (m *ModulePathMatcher) IsPrivate(modulePath string) bool { | ||
| for _, pattern := range m.patterns { | ||
| matched, err := path.Match(pattern, modulePath) | ||
| if err == nil && matched { | ||
| return true | ||
| } | ||
|
|
||
| if strings.HasPrefix(modulePath, pattern+"/") || modulePath == pattern { |
There was a problem hiding this comment.
This type doesn't seem like it needs to exist - it could just be a method on compositeFetcher
| } | ||
|
|
||
| func (p *privateFetcher) Query(ctx context.Context, path, query string) (version string, t time.Time, err error) { | ||
| logger := p.gomod.logger.With(slog.String("module", path), slog.String("query", query)) |
There was a problem hiding this comment.
We shouldn't be reaching into other objects private variables. Create a new logger for this object if necessary.
| var output []byte | ||
| var err error | ||
|
|
||
| repo.WithReadLock(func() { |
There was a problem hiding this comment.
Could also use generics here, so you can return values:
func WithRepoReadLock[R any](repo *Repository, apply func() (R, error)) (value R, err error) {
repo.WithReadLock(func() { value, err = apply() })
return
}Usage:
output, err := gitclone.WithRepoReadLock(repo, func() ([]byte, error) {
cmd := exec.CommandContext(ctx, "git", "-C", repo.Path(), "log", "-1", "--format=%cI", ref)
return cmd.CombinedOutput()
})There was a problem hiding this comment.
Slightly clearer, but eh, your call
| return nil, err | ||
| } | ||
|
|
||
| info := fmt.Sprintf(`{"Version":"%s","Time":"%s"}`, version, commitTime.Format(time.RFC3339)) |
There was a problem hiding this comment.
What is this?
Generally I wouldn't construct JSON in a string, but maybe we know that version can't include any characters that will cause us difficulties?
2 participants
What?
Re-implements #77, which adds support for private gomod repositories.
Why?
So that we are able to cache private go modules.
Tests?
Includes new unit tests. Additionally some manual testing has been performed...