Repository navigation
fix cmd/job test state leaks and pre-commit git index corruption - #6576
siddhartha-quad wants to merge 13 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several stability and environment-related issues affecting the development workflow. It primarily focuses on resolving test state leakage in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents local Git environment variables from leaking into child git processes spawned by go-getter, and introduces flag-resetting mechanisms in tests to avoid state bleeding. The review feedback highlights critical thread-safety concerns in 'pkg/sourcereader/goget.go' when modifying process-wide environment variables concurrently, recommending a package-level mutex to serialize these operations. Additionally, the feedback suggests fully resetting Cobra flag values to their defaults in 'cmd/job/submit_test.go' to ensure complete isolation between test cases.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces changes across several files, including pre-commit configuration updates, flag resetting in job submit tests, and refactoring string builders to use fmt.Fprintf. Most notably, it adds logic in pkg/sourcereader/goget.go to temporarily unset Git local environment variables during module retrieval to prevent leaks into child processes. Feedback on these changes highlights that holding a global mutex during network-bound downloads will serialize remote Git fetches and degrade performance. It is recommended to permanently unset these environment variables in an init() function at package startup, which avoids data races and eliminates the need for locking.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several updates, including unsetting repository-local Git environment variables during package initialization in pkg/sourcereader/goget.go to prevent leaks, adding a new go-unit-tests pre-commit hook, and refactoring string building in deprecation_validate.go and types.go to use fmt.Fprintf. It also implements a mechanism to reset Cobra command flags between test runs in cmd/job/submit_test.go. Regarding the feedback, the reviewer identified an issue in resetCommandFlags where calling cmd.Flags().VisitAll followed by cmd.PersistentFlags().VisitAll causes redundant traversals, and suggested using cmd.LocalFlags() instead.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several updates across the codebase, including unsetting repository-local Git environment variables during package initialization in pkg/sourcereader to prevent leaks, adding a new pre-commit hook for unit tests, resetting command flags in job submission tests to prevent state bleeding, and optimizing string building operations using fmt.Fprintf. Feedback on these changes suggests avoiding side-effects in the library's init() function by exposing a public helper to unset Git environment variables, updating the corresponding tests accordingly, and correcting the reset value of the priority flag in tests to match the CLI's default behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces environment variable cleanup for local Git variables to prevent leakage into child processes, refactors string building to use fmt.Fprintf across several files, updates pre-commit configurations, and improves test isolation by resetting command flags in submit_test.go. The feedback suggests automatically calling UnsetGitLocalEnvVars() within an init() function in the sourcereader package to make the cleanup self-contained and robust, which would also allow removing the manual call in gcluster.go.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces mechanisms to prevent Git local environment variables from leaking into child processes by adding UnsetGitLocalEnvVars and updating pre-commit hooks and main entry points. It also refactors test flag resetting in cmd/job/submit_test.go to prevent state bleeding, optimizes string building in several files, and updates test assertions. The feedback suggests simplifying the flag resetting logic in cmd/job/submit_test.go by removing the redundant call to PersistentFlags().VisitAll, as LocalFlags().VisitAll already covers both local and persistent flags.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements, including unsetting repository-local Git environment variables to prevent leaks into child processes, resetting Cobra command flags between tests to avoid state bleeding, and optimizing string building with fmt.Fprintf. Feedback suggests refactoring the Git environment variable unsetting to temporarily restore them after execution to prevent global side effects, and optimizing the Cobra flag resetting logic by checking if flags were actually changed before resetting them.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces changes to prevent Git environment variables from leaking into child processes, improves test isolation by resetting Cobra command flags and global variables between test runs, and optimizes string building across several packages. The review feedback highlights a critical issue in resetCommandFlags where slice flags will fail to reset correctly because f.DefValue contains brackets, and provides a robust solution to strip them before calling Set.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces changes to isolate Git environment variables during module retrieval, refactors string formatting to use fmt.Fprintf, and adds a mechanism to reset Cobra command flags between unit tests to prevent state bleeding. The review feedback suggests a more robust way to reset slice flags in tests using pflag.SliceValue.Replace rather than string manipulation. Additionally, instead of permanently unsetting Git environment variables globally at startup, it is recommended to temporarily unset and restore them during module retrieval to avoid breaking workflows that rely on these variables.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
github-actions
Bot
assigned
AdarshK15,
agrawalkhushi18 and
Neelabh94 and unassigned
agrawalkhushi18 and
AdarshK15
|
Hi @Neelabh94 , made the respective changes as suggested |
Neelabh94
left a comment
Neelabh94
left a comment
There was a problem hiding this comment.
The code changes look good and all three comments from the previous review round have been cleanly addressed. Please rebase onto develop to resolve the merge conflict, and this can be approved right after.
Neelabh94
left a comment
Neelabh94
left a comment
There was a problem hiding this comment.
LGTM! All previous review comments and the develop rebase look clean. Left one optional, non-blocking simplification on resetJobFlags() in cmd/job/submit_test.go.
| @@ -376,20 +378,20 @@ func setupSubmitTestEnv(t *testing.T) { | |||
| restarts = 1 | |||
| ttlAfterFinished = "1h" | |||
| gracePeriodStr = "30s" | |||
| timeout = "-1s" | |||
| verbose = false | |||
| placementPolicy = "" | |||
| nodeConstraint = map[string]string{} | |||
| nodeConstraint = make(map[string]string) | |||
| cpuAffinityStr = "" | |||
| restartOnExitCodes = nil | |||
| volumeStr = nil | |||
| imagePullSecrets = "" | |||
| serviceAccountName = "" | |||
| topology = "" | |||
| gkeScheduler = "" | |||
| platform = "linux/amd64" | |||
| awaitJobCompletion = false | |||
| priority = "medium" | |||
| timeout = "-1s" | |||
| priority = "" | |||
| verbose = false | |||
| volumeStr = nil | |||
| isPathwaysJob = false | |||
| skipPrereqs = false | |||
| pathways = orchestrator.PathwaysJobDefinition{} | |||
| @@ -400,9 +402,24 @@ func setupSubmitTestEnv(t *testing.T) { | |||
| pathwaysServerEnv = nil | |||
| pathwaysWorkerEnv = nil | |||
|
|
|||
| // Reset list, logs, and inspect subcommand flags | |||
| filterStatus = "" | |||
| filterName = "" | |||
| follow = false | |||
| mainOnly = false | |||
| inspectWorkloadName = "" | |||
| inspectOutputPath = "" | |||
| inspectShow = false | |||
| } | |||
There was a problem hiding this comment.
Non-blocking nit: because resetCommandFlags(JobCmd) visits LocalFlags() on JobCmd (which includes JobCmd.PersistentFlags()) and all subcommands, calling sv.Replace(nil) on slice/array flags and f.Value.Set(f.DefValue) on scalar flags already resets every bound package-level variable back to its init() default. Only pathways (for non-flag struct fields) and nodeConstraint (pflag's stringToStringValue.Set("[]") merges into the existing map once s.changed is true) need explicit assignment:
| func resetJobFlags() { | |
| pathways = orchestrator.PathwaysJobDefinition{} | |
| resetCommandFlags(JobCmd) | |
| nodeConstraint = make(map[string]string) | |
| } |
| _ = os.Setenv("GIT_INDEX_FILE", "should-be-unset") | ||
| saved := unsetGitLocalEnvVars() | ||
| c.Assert(saved["GIT_INDEX_FILE"], Equals, "should-be-unset") | ||
| for _, key := range gitLocalEnvVars { | ||
| _, ok := os.LookupEnv(key) | ||
| c.Assert(ok, Equals, false) | ||
| } | ||
| restoreGitLocalEnvVars(saved) | ||
| c.Assert(os.Getenv("GIT_INDEX_FILE"), Equals, "should-be-unset") | ||
|
|
||
| destDir := c.MkDir() | ||
|
|
||
| // Success via HTTPS | ||
| destDirForHTTPS := filepath.Join(destDir, "https") | ||
| err := s.r.GetModule("github.com/terraform-google-modules/terraform-google-project-factory//helpers", destDirForHTTPS) | ||
| c.Assert(err, IsNil) | ||
| c.Assert(os.Getenv("GIT_INDEX_FILE"), Equals, "should-be-unset") |
There was a problem hiding this comment.
[Minor] git clone still succeeds when GIT_INDEX_FILE is a plain filename like "should-be-unset", so this test passes even if GetModule stops clearing GIT_INDEX_FILE. Point it to a missing folder so GetModule fails if the variable leaks:
| _ = os.Setenv("GIT_INDEX_FILE", "should-be-unset") | |
| saved := unsetGitLocalEnvVars() | |
| c.Assert(saved["GIT_INDEX_FILE"], Equals, "should-be-unset") | |
| for _, key := range gitLocalEnvVars { | |
| _, ok := os.LookupEnv(key) | |
| c.Assert(ok, Equals, false) | |
| } | |
| restoreGitLocalEnvVars(saved) | |
| c.Assert(os.Getenv("GIT_INDEX_FILE"), Equals, "should-be-unset") | |
| destDir := c.MkDir() | |
| // Success via HTTPS | |
| destDirForHTTPS := filepath.Join(destDir, "https") | |
| err := s.r.GetModule("github.com/terraform-google-modules/terraform-google-project-factory//helpers", destDirForHTTPS) | |
| c.Assert(err, IsNil) | |
| c.Assert(os.Getenv("GIT_INDEX_FILE"), Equals, "should-be-unset") | |
| badIndex := filepath.Join(c.MkDir(), "missing", "index") | |
| _ = os.Setenv("GIT_INDEX_FILE", badIndex) | |
| saved := unsetGitLocalEnvVars() | |
| c.Assert(saved["GIT_INDEX_FILE"], Equals, badIndex) | |
| for _, key := range gitLocalEnvVars { | |
| _, ok := os.LookupEnv(key) | |
| c.Assert(ok, Equals, false) | |
| } | |
| restoreGitLocalEnvVars(saved) | |
| c.Assert(os.Getenv("GIT_INDEX_FILE"), Equals, badIndex) | |
| destDir := c.MkDir() | |
| // Success via HTTPS | |
| destDirForHTTPS := filepath.Join(destDir, "https") | |
| err := s.r.GetModule("github.com/terraform-google-modules/terraform-google-project-factory//helpers", destDirForHTTPS) | |
| c.Assert(err, IsNil) | |
| c.Assert(os.Getenv("GIT_INDEX_FILE"), Equals, badIndex) |
| func resetJobFlags() { | ||
| resetCommandFlags(JobCmd) | ||
|
|
There was a problem hiding this comment.
[Nit] resetCommandFlags(JobCmd) already resets all flag variables to their defaults (and the manual list on lines 356–412 already misses newer flags like inspectFilter). Only pathways and nodeConstraint need a manual reset, so lines 353–413 can be simplified to:
func resetJobFlags() {
pathways = orchestrator.PathwaysJobDefinition{}
resetCommandFlags(JobCmd)
nodeConstraint = make(map[string]string)
}| func setupSubmitTestEnv(t *testing.T) { | ||
| t.Setenv("GCLUSTER_TELEMETRY_IMAGE", "") | ||
| resetJobFlags() |
There was a problem hiding this comment.
[Nit] Add t.Helper() so any test failure reports the caller's line number instead of this helper:
| func setupSubmitTestEnv(t *testing.T) { | |
| t.Setenv("GCLUSTER_TELEMETRY_IMAGE", "") | |
| resetJobFlags() | |
| func setupSubmitTestEnv(t *testing.T) { | |
| t.Helper() | |
| t.Setenv("GCLUSTER_TELEMETRY_IMAGE", "") | |
| resetJobFlags() |
Summary
Fixes four test and pre-commit hook issues :
cmd/jobtest state leak (go test -count=3 ./cmd/job/):TestListWorkloadsCmd_InvalidStatussets the package-levelfilterStatusflag variable to"InvalidStatus", which was not reset bysetupSubmitTestEnv(t), causingTestListWorkloadsCmd_Successto fail on subsequent iterations.setupSubmitTestEnv(t)incmd/job/submit_test.goto reset all package-level flag variables acrosscmd/job(list,logs,inspect, andsubmit) as well aspflag.Flag.ChangedacrossJobCmdand all its subcommands.cmd/jobtest order dependence (go test -race -shuffle=1792213762803715470 ./cmd/job/):cmd/job/submit_test.goset the package-leveldryRunManifestvariable via--dry-run-out, which was only reset at the start ofsetupSubmitTestEnv(t)and not cleaned up after test completion. When-shuffleran a dry-run test beforeTestEnsureBasicPrerequisites_SaveStateorTestEnsureBasicPrerequisites_GCloudAuthTimeout(which do not callsetupSubmitTestEnv),ensureBasicPrerequisitesshort-circuited ondryRunManifest != "".resetJobFlags()int.Cleanup(...)insidesetupSubmitTestEnv(t)so flag mutations never leak into subsequent tests.pre-commitgit index corruption andgolangci-lintrewrites:git commit,gitsetsGIT_INDEX_FILE=.git/index. When thego-unit-testshook runsgo test ./..., childgitprocesses spawned bygo-getterinGoGetterSourceReader.GetModule(pkg/sourcereader/goget.go) inheritGIT_INDEX_FILEand write ~90 foreign index entries into the repository's.git/index. This also caused the subsequentgolangci-lint run --new-from-rev HEAD --fixhook to see the entire repository as modified and rewrite unrelated files withstaticcheckQF1012findings.GIT_INDEX_FILE,GIT_DIR,GIT_WORK_TREE, etc.) inGoGetterSourceReader.GetModulebefore callingclient.Get(), and added a regression check inpkg/sourcereader/goget_test.go..pre-commit-config.yamlto unset$(git rev-parse --local-env-vars)in thego-unit-testshooks and removed--fixfrom thegolangci-linthook entry.WriteString(fmt.Sprintf(...))withfmt.Fprintf(...)inpkg/config/deprecation_validate.goandpkg/orchestrator/gke/types.go(QF1012).Vacuous assertion in
TestGeneratePathwaysManifest_Headless:pkg/orchestrator/gke/manifest_generator_test.goto check for"kill -TERM"(matchingworkloadWrapperScriptinpkg/orchestrator/gke/manifest_generator.go) instead of"kill -SIGTERM".