From e2eb304e28598eada68362e384576b5608a78deb Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Wed, 20 May 2026 12:52:08 +1000 Subject: [PATCH 1/2] Populate process ENV from EnvManager by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #578 switched `EnvManager` from `Dotenv.load` to `Dotenv.parse` to gain instance isolation and clean `reset!` semantics. The trade was net negative: fastlane actions read their `default_value:` from `ENV.fetch(...)`, so without ENV mutation the loaded values are invisible to actions like `app_store_connect_api_key`, `match`, or `upload_to_testflight` — defeating the affordance `EnvManager` exists to provide. Layer parsed values into process `ENV` by default, with no-override semantics so pre-existing `ENV` entries (e.g. set by CI) still win. Track which keys we added so `reset!` can roll back precisely, without disturbing pre-existing entries. Pass `mutate_env: false` to opt out and keep the parse-only / instance-isolation semantics for callers (mostly tests) that want them. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 +- .../env_manager/env_manager.rb | 41 ++++- spec/env_manager_spec.rb | 141 +++++++++++++++++- 3 files changed, 177 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a69e035d..706339808 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Breaking Changes -_None_ +- `EnvManager`: populate the process `ENV` from the loaded `.env` file by default (no-override semantics — pre-existing `ENV` values win), so fastlane actions that read their `default_value:` from `ENV` find loaded secrets without callers having to thread them through explicitly. Pass `mutate_env: false` to opt out and keep the parse-only / instance-isolation semantics introduced in [#578]. `EnvManager.reset!` now also rolls back the keys it added. [#XXX] ### New Features diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb b/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb index 75078af5d..6655995a9 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb @@ -15,10 +15,22 @@ class << self end # Set up by loading the .env file with the given name. + # + # When `mutate_env` is true (the default), values from the .env file are + # layered into the process `ENV` using no-override semantics: keys already + # set in `ENV` (e.g. by CI) win. This lets fastlane actions that look up + # values via `ENV.fetch(...)` for their `default_value:` find them without + # the caller having to thread them through explicitly. + # + # Pass `mutate_env: false` to keep `ENV` pristine — values are still + # accessible via `get_required_env!`, but only through this instance. + # Useful for tests that want isolation, or for callers that prefer to + # control `ENV` themselves. def initialize( env_file_name:, env_file_folder: File.join(Dir.home, '.a8c-apps'), example_env_file_path: 'fastlane/example.env', + mutate_env: true, print_error_lambda: ->(message) { FastlaneCore::UI.user_error!(message) }, print_warning_lambda: ->(message) { FastlaneCore::UI.important(message) } ) @@ -31,10 +43,29 @@ def initialize( @print_warning_lambda.call("Warning: env file not found at #{@env_path}. Environment variables may not be loaded.") end - # Parse rather than load so we don't mutate the global ENV. Each instance - # gets its own view, and values from the process environment (e.g. set by - # CI) still take precedence — see `env_value`. + # Always parse rather than load: it lets us track exactly which keys we + # add to `ENV` so `reset!` can undo only those, without disturbing keys + # that pre-existed in the process environment. @loaded_env = File.exist?(@env_path) ? Dotenv.parse(@env_path) : {} + @mutated_keys = [] + + return unless mutate_env + + @loaded_env.each do |key, value| + # No-override: anything already in `ENV` (e.g. set by CI) wins. + next if ENV.key?(key) + + ENV[key] = value + @mutated_keys << key + end + end + + # Remove from `ENV` any keys this instance added via `mutate_env: true`. + # Idempotent — calling more than once is safe. Used by `reset!` and + # available for callers that want to roll back manually. + def restore_env! + @mutated_keys.each { |key| ENV.delete(key) } + @mutated_keys = [] end # Use this instead of getting values from `ENV` directly. It will throw an error if the requested value is missing or empty. @@ -135,8 +166,10 @@ def self.require_env_vars!(*keys) default!.require_env_vars!(*keys) end - # Clears the default instance, useful for test teardown. + # Clears the default instance, useful for test teardown. Also rolls back + # any `ENV` mutations the default instance performed via `mutate_env`. def self.reset! + @default&.restore_env! @default = nil end diff --git a/spec/env_manager_spec.rb b/spec/env_manager_spec.rb index 11ca45375..4ff6df611 100644 --- a/spec/env_manager_spec.rb +++ b/spec/env_manager_spec.rb @@ -62,7 +62,7 @@ expect(manager.env_example_path).to eq('fastlane/example.env') end - it 'loads values from the .env file without mutating ENV' do + it 'loads values from the .env file into ENV by default' do ENV.delete('TEST_INIT_VAR') with_tmp_file(named: 'test.env', content: "TEST_INIT_VAR=loaded\n") do |path| @@ -72,22 +72,40 @@ print_error_lambda: print_error_lambda ) + expect(manager.get_required_env!('TEST_INIT_VAR')).to eq('loaded') + expect(ENV.fetch('TEST_INIT_VAR', nil)).to eq('loaded') + end + end + + it 'leaves ENV untouched when mutate_env is false' do + ENV.delete('TEST_INIT_VAR') + + with_tmp_file(named: 'test.env', content: "TEST_INIT_VAR=loaded\n") do |path| + manager = described_class.new( + env_file_name: File.basename(path), + env_file_folder: File.dirname(path), + mutate_env: false, + print_error_lambda: print_error_lambda + ) + expect(manager.get_required_env!('TEST_INIT_VAR')).to eq('loaded') expect(ENV.fetch('TEST_INIT_VAR', nil)).to be_nil end end - it 'keeps multiple instances isolated from each other' do + it 'keeps multiple instances isolated when mutate_env is false' do with_tmp_file(named: 'a.env', content: "SHARED_KEY=from_a\n") do |path_a| with_tmp_file(named: 'b.env', content: "SHARED_KEY=from_b\n") do |path_b| manager_a = described_class.new( env_file_name: File.basename(path_a), env_file_folder: File.dirname(path_a), + mutate_env: false, print_error_lambda: print_error_lambda ) manager_b = described_class.new( env_file_name: File.basename(path_b), env_file_folder: File.dirname(path_b), + mutate_env: false, print_error_lambda: print_error_lambda ) @@ -108,6 +126,31 @@ ) expect(manager.get_required_env!('PRECEDENCE_KEY')).to eq('from_env') + # No-override: the pre-existing ENV value is left in place. + expect(ENV.fetch('PRECEDENCE_KEY')).to eq('from_env') + end + end + + it 'lets the first instance win when multiple mutate_env instances share a key' do + ENV.delete('FIRST_WINS_KEY') + + with_tmp_file(named: 'a.env', content: "FIRST_WINS_KEY=from_a\n") do |path_a| + with_tmp_file(named: 'b.env', content: "FIRST_WINS_KEY=from_b\n") do |path_b| + described_class.new( + env_file_name: File.basename(path_a), + env_file_folder: File.dirname(path_a), + print_error_lambda: print_error_lambda + ) + described_class.new( + env_file_name: File.basename(path_b), + env_file_folder: File.dirname(path_b), + print_error_lambda: print_error_lambda + ) + + # The second instance's value never lands in ENV because the first + # already populated the key. + expect(ENV.fetch('FIRST_WINS_KEY')).to eq('from_a') + end end end @@ -329,6 +372,64 @@ end end + describe '#restore_env!' do + it 'removes only the keys this instance added to ENV' do + ENV.delete('RESTORE_NEW_KEY') + ENV['RESTORE_PREEXISTING_KEY'] = 'original' + + with_tmp_file( + named: 'restore.env', + content: "RESTORE_NEW_KEY=loaded\nRESTORE_PREEXISTING_KEY=from_file\n" + ) do |path| + manager = described_class.new( + env_file_name: File.basename(path), + env_file_folder: File.dirname(path), + print_error_lambda: print_error_lambda + ) + + expect(ENV.fetch('RESTORE_NEW_KEY')).to eq('loaded') + expect(ENV.fetch('RESTORE_PREEXISTING_KEY')).to eq('original') + + manager.restore_env! + + expect(ENV.fetch('RESTORE_NEW_KEY', nil)).to be_nil + expect(ENV.fetch('RESTORE_PREEXISTING_KEY')).to eq('original') + end + end + + it 'is idempotent' do + ENV.delete('IDEMPOTENT_KEY') + + with_tmp_file(named: 'restore.env', content: "IDEMPOTENT_KEY=loaded\n") do |path| + manager = described_class.new( + env_file_name: File.basename(path), + env_file_folder: File.dirname(path), + print_error_lambda: print_error_lambda + ) + + manager.restore_env! + expect { manager.restore_env! }.not_to raise_error + expect(ENV.fetch('IDEMPOTENT_KEY', nil)).to be_nil + end + end + + it 'is a no-op when mutate_env was false' do + ENV.delete('NOOP_KEY') + + with_tmp_file(named: 'restore.env', content: "NOOP_KEY=loaded\n") do |path| + manager = described_class.new( + env_file_name: File.basename(path), + env_file_folder: File.dirname(path), + mutate_env: false, + print_error_lambda: print_error_lambda + ) + + expect { manager.restore_env! }.not_to raise_error + expect(ENV.fetch('NOOP_KEY', nil)).to be_nil + end + end + end + describe 'CI environment helpers' do subject(:manager) do described_class.new( @@ -522,6 +623,42 @@ expect(described_class).not_to be_configured end + + it 'rolls back ENV mutations the default instance performed' do + described_class.reset! + ENV.delete('RESET_ROLLBACK_KEY') + + with_tmp_file(named: 'reset.env', content: "RESET_ROLLBACK_KEY=loaded\n") do |path| + described_class.set_up( + env_file_name: File.basename(path), + env_file_folder: File.dirname(path), + print_error_lambda: print_error_lambda + ) + + expect(ENV.fetch('RESET_ROLLBACK_KEY')).to eq('loaded') + + described_class.reset! + + expect(ENV.fetch('RESET_ROLLBACK_KEY', nil)).to be_nil + end + end + + it 'leaves pre-existing ENV entries alone on rollback' do + described_class.reset! + ENV['PREEXISTING_KEY'] = 'original' + + with_tmp_file(named: 'reset.env', content: "PREEXISTING_KEY=from_file\n") do |path| + described_class.set_up( + env_file_name: File.basename(path), + env_file_folder: File.dirname(path), + print_error_lambda: print_error_lambda + ) + + described_class.reset! + + expect(ENV.fetch('PREEXISTING_KEY')).to eq('original') + end + end end describe '.configured?' do From 97a20022e08dd030be002d9559cf4f20d6e046eb Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Tue, 26 May 2026 14:08:35 +1000 Subject: [PATCH 2/2] EnvManager: preserve later overwrites on restore `restore_env!` previously deleted every key this instance had set, even if a later caller had overwritten the value via `ENV[key] = ...`. That contradicts the intent of "remove only what this instance added" and could silently drop a caller's update. Track the value written for each mutated key and delete only when `ENV[key]` still matches. Addresses https://github.com/wordpress-mobile/release-toolkit/pull/723#discussion_r3270961934. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.7 --- .../env_manager/env_manager.rb | 15 ++++++++----- spec/env_manager_spec.rb | 21 +++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb b/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb index 6655995a9..2d820cbcd 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb @@ -47,7 +47,10 @@ def initialize( # add to `ENV` so `reset!` can undo only those, without disturbing keys # that pre-existed in the process environment. @loaded_env = File.exist?(@env_path) ? Dotenv.parse(@env_path) : {} - @mutated_keys = [] + # Records the value this instance wrote for each key it added. On + # restore, we delete only if `ENV[key]` still matches — if a later + # caller overwrote it, their value is left alone. + @mutations = {} return unless mutate_env @@ -56,16 +59,18 @@ def initialize( next if ENV.key?(key) ENV[key] = value - @mutated_keys << key + @mutations[key] = value end end - # Remove from `ENV` any keys this instance added via `mutate_env: true`. + # Remove from `ENV` any keys this instance added via `mutate_env: true`, + # but only if the value is still the one we wrote. Keys that a later + # caller has overwritten are left untouched. # Idempotent — calling more than once is safe. Used by `reset!` and # available for callers that want to roll back manually. def restore_env! - @mutated_keys.each { |key| ENV.delete(key) } - @mutated_keys = [] + @mutations.each { |key, value| ENV.delete(key) if ENV[key] == value } + @mutations = {} end # Use this instead of getting values from `ENV` directly. It will throw an error if the requested value is missing or empty. diff --git a/spec/env_manager_spec.rb b/spec/env_manager_spec.rb index 4ff6df611..413e2b3d8 100644 --- a/spec/env_manager_spec.rb +++ b/spec/env_manager_spec.rb @@ -413,6 +413,27 @@ end end + it 'leaves a key alone when a later caller has overwritten it' do + ENV.delete('RESTORE_OVERWRITTEN_KEY') + + with_tmp_file(named: 'restore.env', content: "RESTORE_OVERWRITTEN_KEY=loaded\n") do |path| + manager = described_class.new( + env_file_name: File.basename(path), + env_file_folder: File.dirname(path), + print_error_lambda: print_error_lambda + ) + + expect(ENV.fetch('RESTORE_OVERWRITTEN_KEY')).to eq('loaded') + + # Simulate a later caller overwriting the value the manager set. + ENV['RESTORE_OVERWRITTEN_KEY'] = 'overwritten' + + manager.restore_env! + + expect(ENV.fetch('RESTORE_OVERWRITTEN_KEY')).to eq('overwritten') + end + end + it 'is a no-op when mutate_env was false' do ENV.delete('NOOP_KEY')