Remove import-time side effects from bundle/config and databrickscfg#5526
Open
simonfaltum wants to merge 2 commits into
Open
Remove import-time side effects from bundle/config and databrickscfg#5526simonfaltum wants to merge 2 commits into
simonfaltum wants to merge 2 commits into
Conversation
bundle/config's init() exported DATABRICKS_CLI_PATH into the process environment of every importer, including test binaries and generators. Move the export to main so only the CLI binary itself sets it. libs/databrickscfg's init() flipped the process-global ini.DefaultHeader option at import time. Enable it lazily on first config file write instead, via sync.OnceFunc. Behavior is unchanged for the CLI binary. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Approval status: pending
|
Collaborator
|
Commit: bdbfcbf
22 interesting tests: 15 SKIP, 7 RECOVERED
Top 27 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Found during a full-repo review of the CLI. Two packages mutate process-global state at import time:
bundle/confighas aninit()that exportsDATABRICKS_CLI_PATHinto the process environment. This runs in every importer, including test binaries and code generators, whereos.Args[0]is not the CLI and the resulting value is wrong.libs/databrickscfghas aninit()that flips the process-globalini.DefaultHeaderoption, affecting every user of the ini package in the process even when no databrickscfg file is ever written.Changes
Before, importing these packages was enough to trigger the side effects; now they only happen when the CLI binary starts or when a config file is actually written.
DATABRICKS_CLI_PATHexport moves frombundle/config/workspace.gotomain.go, with the original condition and comment preserved. It runs before command execution, so it is still set before any SDK auth resolution and before any child process is spawned.libs/databrickscfg/ops.goenablesini.DefaultHeaderlazily viasync.OnceFuncfromwriteConfigFile, the single function through which all profile writes (SaveToProfile,DeleteProfile) go.Behavior is unchanged for the CLI binary itself. Existing tests lock both behaviors:
TestSaveToProfile_NewFileWithoutDefaultasserts the[DEFAULT]header in written files, and the no-auth and multi-profile acceptance tests assertDATABRICKS_CLI_PATHshows up in SDK auth error output. A new test inmain_test.goguards against reintroducing the env export in a package init.Test plan
go test . ./bundle/config ./libs/databrickscfg ./cmd/configure ./cmd/authgo test ./acceptance -run 'TestAccept/bundle/run/scripts/no-auth|TestAccept/bundle/multi_profile|TestAccept/cmd/configure'(outputs unchanged, so the binary still exports the variable before SDK config resolution and still writes the[DEFAULT]header)go test ./acceptance -run 'TestAccept/bundle/run/inline-script/no-auth|TestAccept/auth'TestImportDoesNotSetCliPathEnvinmain_test.go./task fmt-q, lint on touched packages,./task checksThis pull request and its description were written by Isaac.