[eas-cli] Combine `.env` and EAS environment variables for `deploy` command #2783

kitten · 2024-12-20T14:22:27Z

Why

This allows --environment to be combined with local .env files, outputting a warning when any variables conflict since EAS environment variables will take precedence.

How

Update createManifestAsync to return manifest and conflicting variable names
Always call getEnv and merge variables into the env object
Output names when conflicts occurred

Test Plan

Tested locally

github-actions · 2024-12-20T14:22:53Z

Subscribed to pull request

File Patterns	Mentions
*/	@szdziedzic, @khamilowicz, @sjchmiela, @radoslawkrzemien

Generated by CodeMention

github-actions · 2024-12-20T14:26:36Z

Size Change: +3.04 kB (+0.01%)

Total Size: 53.4 MB

Filename	Size	Change
`./packages/eas-cli/dist/eas-linux-x64.tar.gz`	53.4 MB	+3.04 kB (+0.01%)

_{compressed-size-action}

codecov · 2024-12-20T14:39:20Z

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 52.49%. Comparing base (a1d086f) to head (0be4a4f).

Files with missing lines	Patch %	Lines
packages/eas-cli/src/worker/assets.ts	0.00%	11 Missing ⚠️
packages/eas-cli/src/commands/worker/deploy.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2783      +/-   ##
==========================================
- Coverage   52.51%   52.49%   -0.01%     
==========================================
  Files         583      583              
  Lines       22582    22589       +7     
  Branches     4452     4454       +2     
==========================================
  Hits        11856    11856              
- Misses      10691    10698       +7     
  Partials       35       35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-12-20T19:13:58Z

✅ Thank you for adding the changelog entry!

wschurman

Question for discussion: should this be the behavior for all commands that accept an --environment flag? (update, etc)

szdziedzic · 2025-01-07T09:29:01Z

packages/eas-cli/src/worker/assets.ts

+  // NOTE: This is required for the .env resolution
+  process.env.NODE_ENV = 'production';
+  // Resolve .env file variables
+  const env: Record<string, string | undefined> = getEnv(params.projectDir).env;


This is probably the question for @byCedric but doesn't the getEnv have a side effect of overriding process.env under the hood? I'm just curious?

Oh okay I see it will be fixed

szdziedzic · 2025-01-07T09:35:44Z

packages/eas-cli/src/worker/assets.ts

+    // Load EAS Env vars into `env` object, keeping track of conflicts
+    conflictingVariableNames = [];
+    for (const variable of loadedVariables) {
+      if (variable.value != null) {
+        if (env[variable.name] != null) {
+          conflictingVariableNames.push(variable.name);
+        }
+        env[variable.name] = variable.value;
+      }
+    }
  }


I believe it doesn't really matter if we override .env with EAS env vars here because npx expo export that generates worker code always uses .env with the highest priority either way. So this won't be the final env var resolution order in the final itself AFAIK because Expo CLI will load env vars from .env for the second time on its own afterwards.

I would say that maybe we should:

just use .env env vars when --environment flag is not specified

just use EAS env vars when --environment flag is specified and skip using .env env vars in Expo CLI by using EXPO_NO_DOTENV=1

what do you think?

We reasoned that during local development, it's very common to start adding new .env vars while still not being ready to update the remote env variables. Hence, we wanted for them to be combined.

Without the combination it's easy to get into a situation where you'd have to always go into the EAS environment variables and modify them (or add temporary env vars) while deploying, for the worker deployments to have the desired set during test deployments.

We can add the EXPO_NO_DOTENV flag for consistency though.

szdziedzic · 2025-01-07T09:36:12Z

packages/eas-cli/src/worker/assets.ts

-  let env: Record<string, string | undefined>;
+): Promise<CreateManifestResult> {
+  // NOTE: This is required for the .env resolution
+  process.env.NODE_ENV = 'production';


Won't it affect all of the actions happening afterward? I discussed it with @brentvatne some time ago and we generally disliked that NODE_ENV which can have many side effects is responsible for choosing which .env file to use. I believe we should unset it somewhere.

This doesn't set the environment outside the process itself, no. So it won't have any effects outside of this command

As per re. which environment is chosen, we don't really want any non-production related .env file from being chosen for the worker deployment, since logically the worker deployments are in a production environment once deployed

szdziedzic · 2025-01-07T09:46:56Z

@kitten Oh okay I think this merging can actually work. You just need to always use EXPO_NO_DOTENV=1 when making calls to Expo CLI and make EAS CLI entirely responsible for lading env vars 👍

One more thing that remains is that this logic would be different from how eas update operates. eas update does:

use .env env vars when the --environment flag is not specified
use EAS env vars when the --environment flag is specified and skip using .env env vars in Expo CLI by using EXPO_NO_DOTENV=1

I'm not saying this is necessarily bad I just want to make you aware of it. Why do I think this can possibly be confusing? We have 2 commands that work really similarly (deploy and update) and they would do this very differently. It can be yet another way of handling env vars in EAS.

szdziedzic · 2025-01-07T09:55:45Z

should this be the behavior for all commands that accept an --environment flag?

EAS Build is tricky here because in 50% of the cases people have .gitignored dotenv files that are not available later on EAS servers, so I'm not sure if we really want to start supporting .env files in eas build command.

kitten requested review from byCedric, kadikraman and szdziedzic December 20, 2024 14:22

kitten force-pushed the @kitten/feat/deploy-env-and-eas-env-simultaneously branch from c6eb6e7 to 04a5671 Compare December 20, 2024 14:22

kitten added 3 commits December 20, 2024 19:13

Allow env vars to be loaded from .env and EAS env simultaneously

a8cc776

Add CHANGELOG entry

fb8b05f

Apply lints

0be4a4f

kitten force-pushed the @kitten/feat/deploy-env-and-eas-env-simultaneously branch from 18ea25b to 0be4a4f Compare December 20, 2024 19:13

wschurman reviewed Jan 6, 2025

View reviewed changes

szdziedzic requested changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eas-cli] Combine `.env` and EAS environment variables for `deploy` command #2783

[eas-cli] Combine `.env` and EAS environment variables for `deploy` command #2783

kitten commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024

wschurman left a comment

szdziedzic Jan 7, 2025

szdziedzic Jan 7, 2025

szdziedzic Jan 7, 2025

kitten Jan 7, 2025

szdziedzic Jan 7, 2025

kitten Jan 7, 2025

kitten Jan 7, 2025

szdziedzic commented Jan 7, 2025

szdziedzic commented Jan 7, 2025

[eas-cli] Combine .env and EAS environment variables for deploy command #2783

Are you sure you want to change the base?

[eas-cli] Combine .env and EAS environment variables for deploy command #2783

Conversation

kitten commented Dec 20, 2024

Why

How

Test Plan

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024 • edited Loading

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 20, 2024

wschurman left a comment

Choose a reason for hiding this comment

szdziedzic Jan 7, 2025

Choose a reason for hiding this comment

szdziedzic Jan 7, 2025

Choose a reason for hiding this comment

szdziedzic Jan 7, 2025

Choose a reason for hiding this comment

kitten Jan 7, 2025

Choose a reason for hiding this comment

szdziedzic Jan 7, 2025

Choose a reason for hiding this comment

kitten Jan 7, 2025

Choose a reason for hiding this comment

kitten Jan 7, 2025

Choose a reason for hiding this comment

szdziedzic commented Jan 7, 2025

szdziedzic commented Jan 7, 2025

[eas-cli] Combine `.env` and EAS environment variables for `deploy` command #2783

[eas-cli] Combine `.env` and EAS environment variables for `deploy` command #2783

github-actions bot commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading