Skip to content

Populate process ENV from EnvManager by default#723

Open
mokagio wants to merge 1 commit into
trunkfrom
mokagio/envmanager-mutate-env-default-true
Open

Populate process ENV from EnvManager by default#723
mokagio wants to merge 1 commit into
trunkfrom
mokagio/envmanager-mutate-env-default-true

Conversation

@mokagio
Copy link
Copy Markdown
Contributor

@mokagio mokagio commented May 20, 2026

What does it do?

Flips EnvManager's default back to populating the process ENV from the loaded .env file (no-override semantics — pre-existing ENV wins).
Adds a mutate_env: false opt-out for callers who want to preserve the parse-only / instance-isolation semantics introduced in #578.

Why

#578 switched EnvManager from Dotenv.load to Dotenv.parse to make instances independent of process ENV and to give reset! something to undo.
Both real wins, but the trade was net negative.

Fastlane actions look up their default_value: via ENV.fetch(...).
With ENV pristine, calls like app_store_connect_api_key, match, and upload_to_testflight can't see anything loaded from the .env file — defeating the affordance EnvManager exists to provide.
matticspace-mobile and the Gravatar SDK had been quietly depending on Dotenv.load's side effect.
The everyfirst-app TestFlight wiring rediscovered the regression first-hand.

This PR keeps the structural wins of #578 (parse + per-instance dict) but layers values into ENV by default.
reset! tracks the keys it added and removes only those, so pre-existing ENV entries are never disturbed.

Migration

This is technically a breaking behavior change for anyone who upgraded to a release that included #578 and depended on ENV staying pristine.
Mitigation: pass mutate_env: false to set_up (or new) to restore the previous behavior.
Worth a major version bump — happy to defer that decision to reviewers.

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations.
  • Add Unit Tests (aka specs/*_spec.rb) if applicable.
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass.
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • If applicable, add an entry in the MIGRATION.md file — pending a decision on whether this ships in a major or minor bump.

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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 20, 2026 02:52
@mokagio mokagio self-assigned this May 20, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR changes EnvManager’s default behavior to once again populate the process ENV from the loaded .env file (while preserving “no-override” semantics where pre-existing ENV values win). It adds an opt-out (mutate_env: false) for callers that want the parse-only / per-instance isolation behavior introduced previously, and updates reset behavior to roll back any default-instance ENV mutations.

Changes:

  • Default EnvManager initialization now layers parsed .env values into process ENV (no-override), with mutate_env: false to opt out.
  • Track and roll back ENV mutations via restore_env!, called from EnvManager.reset!.
  • Update specs to cover default mutation behavior, opt-out isolation, and rollback semantics; add a breaking-change changelog entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb Adds mutate_env defaulting to true, implements restore_env!, and makes reset! roll back default-instance mutations.
spec/env_manager_spec.rb Updates and extends tests to validate default ENV mutation, opt-out behavior, and rollback/idempotency.
CHANGELOG.md Documents the breaking behavior change and the mutate_env: false opt-out.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +66 to +68
def restore_env!
@mutated_keys.each { |key| ENV.delete(key) }
@mutated_keys = []
Comment thread CHANGELOG.md
### 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants