Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): Source @expo/env dotenv vars for worker deployments #2545

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

kitten
Copy link
Member

@kitten kitten commented Sep 10, 2024

Why

Deployments should allow environment variables to be added to them.

How

Source environment variables using @expo/env, add them to the manifest, and send them to the deployment API to upload environment variables for deployments.

Test Plan

  • Manually tested

Copy link

github-actions bot commented Sep 10, 2024

Size Change: +75.9 kB (+0.14%)

Total Size: 52.9 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 52.9 MB +75.9 kB (+0.14%)

compressed-size-action

Copy link
Member

@szdziedzic szdziedzic left a comment

Choose a reason for hiding this comment

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

Hmm, shouldn't we actually use server-side defined env vars for deployments? Why or why not? What is our approach here?

I believe that using @expo/env with EAS is one of the most commonly flagged pain points of EAS Update users. What I mean here is that you usually keep your local dev env vars in .env.local, but when you trigger a prod update they will unknowingly be used with the highest priority for you in your prod update bundle, so you need to be very careful when using EAS Update.

By using @expo/env for worker deployments we might introduce the same confusion here 🤔

Copy link
Contributor

@kadikraman kadikraman left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally, the env vars from .env are available in api routes ✔️

@szdziedzic
Copy link
Member

@kadikraman @kitten
this is what I planned to do with EAS Update, so my plan was to go in a slightly different direction 😅
https://exponent-internal.slack.com/archives/C013ZK4SA12/p1725644220583309

@kitten kitten force-pushed the @kitten/feat/add-worker-deploy-envs branch from 7d62955 to 9c82487 Compare September 11, 2024 11:45
Copy link

✅ Thank you for adding the changelog entry!

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.

Project coverage is 52.34%. Comparing base (51b5946) to head (8adf137).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/eas-cli/src/commands/worker/deploy.ts 0.00% 5 Missing ⚠️
packages/eas-cli/src/worker/assets.ts 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2545      +/-   ##
==========================================
- Coverage   52.34%   52.34%   -0.00%     
==========================================
  Files         554      554              
  Lines       20885    20889       +4     
  Branches     4274     4274              
==========================================
+ Hits        10931    10933       +2     
- Misses       9097     9099       +2     
  Partials      857      857              

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

@kitten kitten merged commit 1621fb3 into main Sep 11, 2024
8 checks passed
@kitten kitten deleted the @kitten/feat/add-worker-deploy-envs branch September 11, 2024 12:03
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.

3 participants