-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Supports a go workspace projects #12012
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
@@ -272,6 +272,11 @@ func main() { | |||
} else if util.DirExists("vendor") { | |||
modMode = ModMod | |||
} | |||
if util.FileExists("go.work") { |
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.
Hey @cardil! 👋🏼 Thank you for your contribution. Checking for the presence of a go.work
file should be sufficient in most cases, but I have a suggestion to make the check for workspace mode more resilient.
Please consider checking the output of the go env GOWORK
command instead. This documentation from the Go blog provides some context on the command.
Checking for workspace mode using go env GOWORK
is resilient to the build being run with GOWORK=off
, in which case workspace mode should not be used even if a go.work
file is present. Please let us know if you have any questions about this suggestion, and thank you again for opening this PR.
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.
If it helps, there are existing patterns within that file for executing a command and working with the output. Here's an example:
codeql/go/extractor/cli/go-autobuilder/go-autobuilder.go
Lines 317 to 321 in 862948f
res := util.RunCmd(exec.Command("go", "mod", "tidy", "-e")) | |
if !res { | |
log.Println("Failed to run `go mod tidy -e`") | |
} else { |
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.
Make sense.
Although I'm thinking about tests. I can't see any. Maybe I should include some?!
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.
Contributions that include one or more tests are always a bonus. 😄 We have a couple of existing tests in go-autobuilder_test.go
, for functions within the go-autobuilder.go
file. If you do intend to include tests, that would be the place to do 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.
We'll probably want an end-to-end integration test for these, and we're just converting those tests for Go into a new format, so it's probably best to leave writing that test to us just now
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.
Those tests have been converted now, in this PR: #12130
Superseded by #15361 which implemented support for |
TBD
Resolves #12010