Refactor environment variable implementation#3381
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the environment variable implementation to support deprecation, better distinguish between unset and empty values, and provide a more ergonomic API. The changes move from a global function-based approach (GetEnvironmentVariable(env)) to method-based access (env.Value()). The new EnvironmentVariable type includes support for deprecated variable names via Replaces and ReplacedBy fields, with automatic fallback and warning mechanisms.
Changes:
- Introduces new methods on
EnvironmentVariable:Lookup(),IsSet(),Value(),Clear(), andLookupName()for accessing and managing environment variables - Adds deprecation infrastructure with automatic warning generation when deprecated variables are used
- Updates all environment variable access throughout the codebase to use the new API pattern
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| common/environment.go | Core refactoring: adds new methods, deprecation support, and changes from function-based to method-based API |
| common/oauthTokenManager.go | Updates OAuth token manager to use new Lookup() pattern for checking if environment variables are set |
| common/version.go | Converts UserAgentPrefix access to use new Value() method |
| common/logger.go | Updates DisableSyslog check to use Lookup() with proper boolean handling |
| common/init.go | Refactors folder initialization to use Lookup() for distinguishing unset vs empty paths |
| common/init_windows.go | Updates UserDir access to use Value() method |
| common/init_unix.go | Updates UserDir access to use Value() method |
| common/credentialFactory.go | Updates credential creation to use Lookup() for checking variable presence |
| common/credCache_windows.go | Updates token cache file path logic to use Lookup() |
| common/clientFactory.go | Updates shared key credential creation to use Lookup() |
| common/ProxyLookupCache.go | Updates proxy cache lookup to use Value() method |
| ste/xferVersionPolicy.go | Updates default service API version initialization to use Value() |
| ste/xfer.go | Updates request timeout initialization to use Lookup() |
| ste/xfer-remoteToLocal-file.go | Updates download temp path check to use Value() |
| ste/sourceInfoProvider-GCP.go | Updates GCP credentials path access to use Lookup() with proper error messages |
| ste/sender-blockBlob.go | Updates blob transfer resume check to use Value() with strings.EqualFold |
| ste/pageRangeOptimizer.go | Updates sparse page blob optimization check to use Value() |
| ste/pacer-autoPacer.go | Updates pace page blobs check to use Value() |
| ste/concurrency.go | Updates concurrency and bool configuration helpers to use Lookup() |
| jobsAdmin/init.go | Updates MIME mapping path check to use Lookup() |
| jobsAdmin/JobsAdmin.go | Updates buffer GB override check to use Lookup() |
| traverser/zc_traverser_gcp_service.go | Changes projectID initialization to package-level with Value() |
| traverser/zc_traverser_blob.go | Updates hierarchical scanning check to use Value() |
| testSuite/cmd/platdiff_windows.go | Updates UserDir access to use Value() |
| testSuite/cmd/common.go | Updates AWS credentials access to use Lookup() with proper error messages |
| cmd/uihooks.go | Updates CPU and memory profiling path checks to use Lookup() |
| cmd/root.go | Updates concurrency value check to use Lookup() for auto-tuning logic |
| cmd/login.go | Updates certificate password and client secret checks to use Lookup() |
| cmd/env.go | Adds deprecation warning display when listing environment variables |
| cmd/copy.go | Updates concurrency check to use IsSet() |
| azcopy/output.go | Updates performance state display check to use IsSet() |
| azcopy/credentialUtil.go | Updates credential type and cloud provider credential checks to use IsSet() and Lookup() |
| azcopy/copyRedirection.go | Updates concurrency environment variable access with Lookup() for better error messages |
| azcopy/copy.go | Updates concurrency check to use IsSet() |
| azcopy/client.go | Updates login cache name check to use Lookup() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wonwuakpa-msft
left a comment
There was a problem hiding this comment.
It should be good after addressing the copilot comments
Description
In the course of work on the
cap-mbpswork item, I wanted to deprecate the oldAZCOPY_CONCURRENCY_VALUEenvironment variable in favor of a more descriptiveAZCOPY_CONCURRENT_ROUTINESvariable. However, we don't have any real support for doing so. The intent of this PR is a few things.EnvironmentVariable.EnvironmentVariabledirectly, as opposed to calling a function with the environment variable as a parameter. This makes it less frustrating to work with an environment variable.""vs unsetType of Change
How Has This Been Tested?
Refactor that shouldn't add any functionality-- existing testing should catch issues.