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

[eas-cli] Makes eas.json configuration to only run on update:configure #1598

Merged
merged 3 commits into from
Jan 4, 2023

Conversation

jonsamp
Copy link
Member

@jonsamp jonsamp commented Dec 29, 2022

Checklist

  • I've added an entry to CHANGELOG.md if necessary. You can comment this pull request with /changelog-entry [breaking-change|new-feature|bug-fix|chore] [message] and CHANGELOG.md will be updated automatically.

Why

This PR is a bug fix. I landed a PR where when running eas update:configure, we set the channel property in build profiles. However, I placed that logic in a place that is also called when running eas update .... (We want to set up expo-updates and configure app.json if needed when running eas update)

This is not the desired behavior for this configuration however. We only want to set up channels in build profiles when a developer is running eas update:configure. By removing this from the eas update command, it allows developers to set a channel in their native code, then use EAS Build, and not have their native channel overwritten.

How

  • Takes the ensureEASUpdateIsConfiguredInEasJsonAsync() function out of the shared configuration and call it in the update:configure command.

Test Plan

  1. Create an app with npc create-expo-app myApp
  2. Run eas build:configure
  3. Run eas update. Expect that there will be no channels configured in eas.json, while app.json will be configured.
  4. Run eas update:configure. Expect that eas.json is configured with channels.

@jonsamp
Copy link
Member Author

jonsamp commented Dec 29, 2022

/changelog-entry bug-fix Makes eas.json configuration to only run on update:configure

@jonsamp jonsamp changed the title [eas-clie] Makes eas.json configuration to only run on update:configure [eas-cli] Makes eas.json configuration to only run on update:configure Dec 29, 2022
@github-actions
Copy link

github-actions bot commented Dec 29, 2022

Size Change: -162 B (0%)

Total Size: 40.3 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 40.3 MB -162 B (0%)

compressed-size-action

@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Merging #1598 (26dcf67) into main (9245657) will increase coverage by 0.01%.
The diff coverage is 66.67%.

@@            Coverage Diff             @@
##             main    #1598      +/-   ##
==========================================
+ Coverage   51.94%   51.94%   +0.01%     
==========================================
  Files         464      464              
  Lines       16243    16243              
  Branches     3381     3381              
==========================================
+ Hits         8435     8436       +1     
+ Misses       7791     7790       -1     
  Partials       17       17              
Impacted Files Coverage Δ
packages/eas-cli/src/commands/update/configure.ts 32.26% <50.00%> (-1.07%) ⬇️
packages/eas-cli/src/update/configure.ts 12.50% <100.00%> (+0.83%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -46,6 +49,8 @@ export default class UpdateConfigure extends EasCommand {
platform,
});

await ensureEASUpdateIsConfiguredInEasJsonAsync(projectDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

update:configure is supposed to prepare the project for updates, but with this change, you are removing all config steps and only updating eas.json. e.g. You are not checking if expo-updates is installed.

Based on the PR description I would expect that changes in this PR will not affect update:configure method and the only difference for the update call would be skipping that eas.json update.

Copy link
Member Author

@jonsamp jonsamp Jan 3, 2023

Choose a reason for hiding this comment

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

I think there may be a misunderstanding here.

This line (52) is inside the runAsync() function on the update:configure command. When running update:configure, we will run two key functions:

  1. ensureEASUpdateIsConfiguredAsync(): This installs expo-updates, configures app.json, etc
  2. ensureEASUpdateIsConfiguredInEasJsonAsync(): This adds channel names to eas.json

Previously, that second function, ensureEASUpdateIsConfiguredInEasJsonAsync(), was inside of ensureEASUpdateIsConfiguredAsync(), which made it run whenever anyone ran update.

This PR removes the eas.json configuration part to only run on update:configure, and makes it so that update does not run the eas.json configuration part.

When running update, we will still install expo-updates + configure app.json etc. Here's where we call that in the update command:

await ensureEASUpdateIsConfiguredAsync(graphqlClient, {

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you are right, sorry. I didn't see that both functions were called in configure

@jonsamp
Copy link
Member Author

jonsamp commented Jan 4, 2023

Merging because I got two checkmarks and I think I answered @wkozyra95's question adequately. If you still think that this is an issue, please let me know and we can discuss/revert

@jonsamp jonsamp merged commit 5694bb9 into main Jan 4, 2023
@jonsamp jonsamp deleted the jon/eng-7125-only-add-channels-to-build-profiles-when branch January 4, 2023 04:37
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.

4 participants