Consolidate environment variable interaction #747
Merged
Conversation
This file contains 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
There are a couple places throughout the code base where interaction with environment variables takes place. Moreover, more than one of these would try to read a value from more than one environment variable as fallback (for backwards compatibility). This change consolidates those accesses. The majority of diffs in this change are mechanical (i.e. add an argument or replace a call). This change: * Moves common environment variable lookups for bundles to `bundles/env`. * Adds a `libs/env` package that wraps `os.LookupEnv` and `os.Getenv` and allows for overrides to take place in a `context.Context`. By scoping overrides to a `context.Context` we can avoid `t.Setenv` in testing and unlock parallel test execution. * Updates call sites to pass through a `context.Context` where needed. * For bundles, introduces `DATABRICKS_BUNDLE_ROOT` as new primary variable instead of `BUNDLE_ROOT`. This was the last environment variable that did not use the `DATABRICKS_` prefix.
andrewnester
approved these changes
Sep 8, 2023
andrewnester
approved these changes
Sep 8, 2023
|
|
||
| // format is <DATABRICKS_BUNDLE_TMP>/<target> | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, filepath.Join(bundleTmpDir, "default"), cacheDir) | ||
| } | ||
|
|
||
| func TestBundleMustLoadSuccess(t *testing.T) { | ||
| t.Setenv(envBundleRoot, "./tests/basic") | ||
| t.Setenv(env.RootVariable, "./tests/basic") |
There was a problem hiding this comment.
shall we use envlib.Set(ctx, env.RootVariable, "./tests/basic") here instead already? Or in general what is our plan to get rid of t.Setenv?
There was a problem hiding this comment.
We have (and will have) a couple of long running tests that should be parallelized. We don't have to parallelise all tests IMO, for example tests that don't make any/very few network calls.
For example, the sync integration tests can take minutes if run serially and thus are important to parallelise.
There was a problem hiding this comment.
Indeed, we can do this incrementally. I'd like this change to be as small and mechanical as possible.
shreyas-goenka
approved these changes
Sep 8, 2023
pietern
added a commit
that referenced
this pull request
Sep 11, 2023
Bundles: * Fix conversion of job parameters ([#744](#744)). * Add schema and config validation to jsonschema package ([#740](#740)). * Support Model Serving Endpoints in bundles ([#682](#682)). * Do not include empty output in job run output ([#749](#749)). * Fixed marking libraries from DBFS as remote ([#750](#750)). * Process only Python wheel tasks which have local libraries used ([#751](#751)). * Add enum support for bundle templates ([#668](#668)). * Apply Python wheel trampoline if workspace library is used ([#755](#755)). * List available targets when incorrect target passed ([#756](#756)). * Make bundle and sync fields optional ([#757](#757)). * Consolidate environment variable interaction ([#747](#747)). Internal: * Update Go SDK to v0.19.1 ([#759](#759)).
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 11, 2023
Bundles: * Fix conversion of job parameters ([#744](#744)). * Add schema and config validation to jsonschema package ([#740](#740)). * Support Model Serving Endpoints in bundles ([#682](#682)). * Do not include empty output in job run output ([#749](#749)). * Fixed marking libraries from DBFS as remote ([#750](#750)). * Process only Python wheel tasks which have local libraries used ([#751](#751)). * Add enum support for bundle templates ([#668](#668)). * Apply Python wheel trampoline if workspace library is used ([#755](#755)). * List available targets when incorrect target passed ([#756](#756)). * Make bundle and sync fields optional ([#757](#757)). * Consolidate environment variable interaction ([#747](#747)). Internal: * Update Go SDK to v0.19.1 ([#759](#759)).
hectorcast-db
pushed a commit
that referenced
this pull request
Oct 13, 2023
## Changes There are a couple places throughout the code base where interaction with environment variables takes place. Moreover, more than one of these would try to read a value from more than one environment variable as fallback (for backwards compatibility). This change consolidates those accesses. The majority of diffs in this change are mechanical (i.e. add an argument or replace a call). This change: * Moves common environment variable lookups for bundles to `bundles/env`. * Adds a `libs/env` package that wraps `os.LookupEnv` and `os.Getenv` and allows for overrides to take place in a `context.Context`. By scoping overrides to a `context.Context` we can avoid `t.Setenv` in testing and unlock parallel test execution for integration tests. * Updates call sites to pass through a `context.Context` where needed. * For bundles, introduces `DATABRICKS_BUNDLE_ROOT` as new primary variable instead of `BUNDLE_ROOT`. This was the last environment variable that did not use the `DATABRICKS_` prefix. ## Tests Unit tests pass.
hectorcast-db
pushed a commit
that referenced
this pull request
Oct 13, 2023
Bundles: * Fix conversion of job parameters ([#744](#744)). * Add schema and config validation to jsonschema package ([#740](#740)). * Support Model Serving Endpoints in bundles ([#682](#682)). * Do not include empty output in job run output ([#749](#749)). * Fixed marking libraries from DBFS as remote ([#750](#750)). * Process only Python wheel tasks which have local libraries used ([#751](#751)). * Add enum support for bundle templates ([#668](#668)). * Apply Python wheel trampoline if workspace library is used ([#755](#755)). * List available targets when incorrect target passed ([#756](#756)). * Make bundle and sync fields optional ([#757](#757)). * Consolidate environment variable interaction ([#747](#747)). Internal: * Update Go SDK to v0.19.1 ([#759](#759)).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
There are a couple places throughout the code base where interaction with environment variables takes place. Moreover, more than one of these would try to read a value from more than one environment variable as fallback (for backwards compatibility). This change consolidates those accesses.
The majority of diffs in this change are mechanical (i.e. add an argument or replace a call).
This change:
bundles/env.libs/envpackage that wrapsos.LookupEnvandos.Getenvand allows for overrides to take place in acontext.Context. By scoping overrides to acontext.Contextwe can avoidt.Setenvin testing and unlock parallel test execution for integration tests.context.Contextwhere needed.DATABRICKS_BUNDLE_ROOTas new primary variable instead ofBUNDLE_ROOT. This was the last environment variable that did not use theDATABRICKS_prefix.Tests
Unit tests pass.