Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions internal/testutil/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,11 @@ import (
// The original environment is restored upon test completion.
// Note: use of this function is incompatible with parallel execution.
func CleanupEnvironment(t TestingT) {
// Restore environment when test finishes.
environ := os.Environ()
t.Cleanup(func() {
// Restore original environment.
for _, kv := range environ {
kvs := strings.SplitN(kv, "=", 2)
os.Setenv(kvs[0], kvs[1])
}
})

path := os.Getenv("PATH")
pwd := os.Getenv("PWD")
os.Clearenv()

// Clear all environment variables.
NullEnvironment(t)

// We use t.Setenv instead of os.Setenv because the former actively
// prevents a test being run with t.Parallel. Modifying the environment
Expand All @@ -38,6 +30,23 @@ func CleanupEnvironment(t TestingT) {
}
}

// NullEnvironment sets up an empty environment with absolutely no environment variables set.
// The original environment is restored upon test completion.
// Note: use of this function is incompatible with parallel execution
func NullEnvironment(t TestingT) {
Comment thread
shreyas-goenka marked this conversation as resolved.
// Restore environment when test finishes.
environ := os.Environ()
t.Cleanup(func() {
// Restore original environment.
for _, kv := range environ {
kvs := strings.SplitN(kv, "=", 2)
os.Setenv(kvs[0], kvs[1])
}
})

os.Clearenv()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the nuance between the two different cleanup functions?

The distinction is not clear from the name (cleanup vs clear).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this to NullEnvironment to make the nuance more clear.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think NullEnvironment should follow naming of os.Clearenv() since that's essentially a wrapper: Clearenv(t* testing.T).

CleanupEnvironment can be renamed to SetMinimalEnvVars? However, I'd rather we did not do this in tests, this feels like it could break things.

// Changes into specified directory for the duration of the test.
// Returns the current working directory.
func Chdir(t TestingT, dir string) string {
Expand Down
60 changes: 58 additions & 2 deletions libs/auth/env.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
package auth

import "github.com/databricks/databricks-sdk-go/config"
import (
"fmt"
"os"
"slices"
"strings"

"github.com/databricks/databricks-sdk-go/config"
)

// Env generates the authentication environment variables we need to set for
// downstream applications from the CLI to work correctly.
Expand Down Expand Up @@ -44,7 +51,7 @@ func GetEnvFor(name string) (string, bool) {
// This is useful for spawning subprocesses since you can unset all auth environment
// variables to clean up the environment before configuring authentication for the
// child process.
func EnvVars() []string {
func envVars() []string {
out := []string{}

for _, attr := range config.ConfigAttributes {
Expand All @@ -57,3 +64,52 @@ func EnvVars() []string {

return out
}

// ProcessEnv generates the environment variables that should be set to authenticate
// downstream processes to use the same auth credentials as in cfg.
func ProcessEnv(cfg *config.Config) []string {
// We want child processes to inherit environment variables like $HOME or $HTTPS_PROXY
// because they influence auth resolution.
base := os.Environ()

out := []string{}
authEnvVars := envVars()

// Remove any existing auth environment variables. This is done because
// the CLI offers multiple modalities of configuring authentication like
// `--profile` or `DATABRICKS_CONFIG_PROFILE` or `profile: <profile>` in the
// bundle config file.
//
// Each of these modalities have different priorities and thus we don't want
// any auth configuration to piggyback into the child process environment.
//
// This is a precaution to avoid conflicting auth configurations being passed
// to the child telemetry process.
//
// Normally this should be unnecessary because the SDK should error if multiple
// authentication methods have been configured. But there is no harm in doing this
// as a precaution.
for _, v := range base {
k, _, found := strings.Cut(v, "=")
if !found {
continue
}
if slices.Contains(authEnvVars, k) {
continue
}
out = append(out, v)
}

// Now add the necessary authentication environment variables.
newEnv := Env(cfg)
Comment thread
shreyas-goenka marked this conversation as resolved.
for k, v := range newEnv {
out = append(out, fmt.Sprintf("%s=%s", k, v))
}

// Sort the environment variables so that the output is deterministic.
// Keeping the output deterministic helps with reproducibility and keeping the
// behavior consistent incase there are any issues.
slices.Sort(out)
Comment thread
shreyas-goenka marked this conversation as resolved.

return out
}
2 changes: 1 addition & 1 deletion libs/auth/env_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func TestAuthEnvVars(t *testing.T) {
"ACTIONS_ID_TOKEN_REQUEST_TOKEN",
}

out := EnvVars()
out := envVars()
for _, v := range contains {
assert.Contains(t, out, v)
}
Expand Down