feat: extend GoModResolver to support any valid Go *.mod file#268
feat: extend GoModResolver to support any valid Go *.mod file#268kezhenxu94 merged 3 commits intoapache:mainfrom
Conversation
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
Hi @kezhenxu94 When you have time, could you have a look pls? |
There was a problem hiding this comment.
Pull request overview
Extends the Go dependency resolver to recognize and handle non-default Go module files (e.g., go.tool.mod), including passing -modfile to go mod download, and adds tests plus a Makefile arch-detection tweak.
Changes:
- Update
GoModResolver.CanResolveto match*.modfiles and add module-file validation +-modfilesupport during resolution. - Add Go resolver unit tests for
CanResolve, invalid module-file validation behavior, and package license resolution. - Makefile now derives
ARCHfromgo env GOARCH(overridable), and renames theunamevariable used for OS detection.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/deps/golang.go | Broaden resolver matching to *.mod, validate modfile contents, and pass -modfile for non-go.mod files |
| pkg/deps/golang_test.go | New tests covering Go mod resolver matching/validation and license resolution behavior |
| Makefile | Detect host GOARCH by default rather than hardcoding amd64 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func validateGoModFile(file string) error { | ||
| content, err := os.ReadFile(file) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !goModuleDirective.Match(content) || !goVersionDirective.Match(content) { | ||
| return fmt.Errorf("%v is not a valid Go module file", file) | ||
| } |
There was a problem hiding this comment.
validateGoModFile requires both module and go directives, but the go directive is optional in valid go.mod/*.mod files. This will reject legitimate modules and cause dependency resolution to fail. Consider parsing with golang.org/x/mod/modfile (and only requiring a valid module directive), or relax the validation to not require a go directive.
pkg/deps/golang.go
Outdated
| var ( | ||
| goModuleDirective = regexp.MustCompile(`(?m)^\s*module\s+\S`) | ||
| goVersionDirective = regexp.MustCompile(`(?m)^\s*go\s+\d`) | ||
| possibleLicenseFileName = regexp.MustCompile(`(?i)^LICENSE|LICENCE(\.txt)?|COPYING(\.txt)?$`) |
There was a problem hiding this comment.
possibleLicenseFileName is missing grouping/anchoring, so it can match unintended names (e.g., any filename containing "LICENCE" anywhere) and ^LICENSE will also match directories like LICENSES. Since ResolvePackageLicense doesn't check info.IsDir(), this can lead to attempting to ReadFile on a directory and failing license resolution. Suggest anchoring the full pattern (e.g., ^(LICENSE|LICENCE)(\.txt)?$|^COPYING(\.txt)?$) and skipping directories before reading.
| possibleLicenseFileName = regexp.MustCompile(`(?i)^LICENSE|LICENCE(\.txt)?|COPYING(\.txt)?$`) | |
| possibleLicenseFileName = regexp.MustCompile(`(?i)^(LICENSE|LICENCE|COPYING)(\.txt)?$`) |
There was a problem hiding this comment.
this regex pre-existed the PR (it was just moved); the PR made it visible. I could fix it, as it's valid and a real bug.
| return err | ||
| } | ||
|
|
||
| logger.Log.Debugf("\t- Found license: %v", identifier) |
There was a problem hiding this comment.
Before os.ReadFile(licenseFilePath) earlier in this function, ensure the directory entry isn’t a directory (info.IsDir()), otherwise directories like LICENSES/ can be treated as license files and cause a hard error. (The current name regex also matches prefixes like LICENSE*.)
| t.Fatalf("expected 1 resolved, got %d", len(report.Resolved)) | ||
| } | ||
| if report.Resolved[0].LicenseSpdxID != npmLicenseApache20 { | ||
| t.Errorf("expected %v, got %v", npmLicenseApache20, report.Resolved[0].LicenseSpdxID) | ||
| } |
There was a problem hiding this comment.
This test relies on npmLicenseApache20, which is declared in an unrelated NPM resolver test file. That coupling is fragile (e.g., if that file is renamed, moved, or build-tagged) and makes this test harder to understand in isolation. Prefer defining a local constant (or using a literal) in this file.
|
@ivankatliarchuk can you please fix the CI |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
Done. I've added test-docker to Makefile directive. As running tests locally require maven installed. Tested with Green as well |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Why: Projects using Go 1.24's tool dependency pattern (e.g. go.tool.mod) were failing with unable to find a resolver because the resolver only matched the hardcoded filename go.mod.
What:
Current
With the Change
Log example