Skip to content

[eas-cli] Prompt before configuring EAS project#1356

Merged
wschurman merged 1 commit into
mainfrom
@wschurman/interactive-project-setup
Sep 16, 2022
Merged

[eas-cli] Prompt before configuring EAS project#1356
wschurman merged 1 commit into
mainfrom
@wschurman/interactive-project-setup

Conversation

@wschurman

@wschurman wschurman commented Sep 13, 2022

Copy link
Copy Markdown
Member

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.

Background

Why

This PR overhauls how we do EAS project configuration to be the following:

  • Any command that needs the EAS project ID now must configure it if it is not already present in the app config. This means that if they choose not to configure it or we can't do it for them (the dynamic config case), the command will fail until they add it to their app config. This is technically a breaking change for apps that use dynamic configs and have not configured their apps before doing something in a non-interactive mode, but this is a necessary breaking change since project ID is required going forward.
  • Configuring it means:
    1. Looking up a project with accountName/slug on the server. If it exists, ask the user whether this is the project they intended to configure. This asking prevents unintentional configuration with the incorrect project.
    2. If no project is found, ask the user whether they want to create a new project. This asking prevents unintentional project creation due to misconfiguration (wrong owner field for example).

If at the end of this process we don't have an EAS project ID, the command will fail.

How

Update the method to match behavior described above, and update tests accordingly.

Note that this exposed some inconsistencies between how we do non-interactive across commands, so I may unify those before or after landing this, I haven't decided.

Test Plan

Run commands to ensure they ask if not configured, but not afterwards:

$ ~/expo/eas-cli/packages/eas-cli/bin/run branch:list --json
EAS project not configured.
    Error: Must configure EAS project by running `eas init` before this command can be run in
    non-interactive mode

$ ~/expo/eas-cli/packages/eas-cli/bin/run branch:list
EAS project not configured.
✔ Would you like to automatically create an EAS project for @wschurman141/turtlewat? … yes
✔ Created @wschurman141/turtlewat on Expo
✔ Linked local project to EAS project 1a7e84d3-978d-476d-97cb-08a992acd17e

Branches:
...

$ ~/expo/eas-cli/packages/eas-cli/bin/run branch:list

Branches:
...

@wschurman

Copy link
Copy Markdown
Member Author

/changelog-entry breaking-change Prompt before configuring EAS project

@github-actions

github-actions Bot commented Sep 13, 2022

Copy link
Copy Markdown

Size Change: +153 B (0%)

Total Size: 41.1 MB

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

compressed-size-action

@wschurman wschurman marked this pull request as ready for review September 13, 2022 21:34
@wschurman wschurman force-pushed the @wschurman/interactive-project-setup branch from 4a5321d to 39490aa Compare September 13, 2022 21:36
@wschurman wschurman force-pushed the @wschurman/interactive-project-setup branch from 39490aa to 5a2b08d Compare September 13, 2022 22:10
@codecov

codecov Bot commented Sep 13, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1356 (b180759) into main (b288c31) will decrease coverage by 0.04%.
The diff coverage is 21.62%.

@@            Coverage Diff             @@
##             main    #1356      +/-   ##
==========================================
- Coverage   51.24%   51.21%   -0.03%     
==========================================
  Files         410      410              
  Lines       14531    14569      +38     
  Branches     3001     3014      +13     
==========================================
+ Hits         7445     7460      +15     
- Misses       6548     6553       +5     
- Partials      538      556      +18     
Impacted Files Coverage Δ
packages/eas-cli/src/build/android/build.ts 31.82% <ø> (ø)
packages/eas-cli/src/build/android/prepareJob.ts 34.79% <0.00%> (ø)
...-cli/src/build/android/syncProjectConfiguration.ts 30.00% <0.00%> (ø)
packages/eas-cli/src/build/createContext.ts 26.79% <0.00%> (ø)
packages/eas-cli/src/build/ios/build.ts 36.37% <ø> (ø)
packages/eas-cli/src/build/ios/prepareJob.ts 36.37% <0.00%> (ø)
.../eas-cli/src/build/ios/syncProjectConfiguration.ts 25.00% <0.00%> (ø)
packages/eas-cli/src/build/metadata.ts 22.10% <ø> (ø)
packages/eas-cli/src/commands/branch/delete.ts 29.10% <0.00%> (ø)
packages/eas-cli/src/commands/branch/list.ts 52.64% <0.00%> (ø)
... and 53 more

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

const fullName = await getProjectFullNameAsync(exp);
const projectId = await getProjectIdAsync(exp);

// TODO(wschurman): this non-interactive flag should be standardized

@wkozyra95 wkozyra95 Sep 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that this exposed some inconsistencies between how we do non-interactive across commands, so I may unify those before or after landing this, I haven't decided.

Our non-interactivity work is pretty consistent across all the commands, the way it was intended to work is:

  • if there is no json/non-interactive flag, we do not want to support enforcing interactivity e.g. eas credentials is always interactive so flag there would be redundant, but on the other hand, there are some commands that never needed to prompt for anything so --non-interactive flag would be pointless there too.
  • if there is only json flag command it should mean that command is always non-interactive
  • it there is only non-interactive flag, it's self-explanatory
  • if there are both json and nonInteractive flags, then json flag should require non-interactive to also be set.

In this PR you are making all the commands interactive, and breaking that convention in the process. I could see different approaches how we could resolve it:

  • add nonInteractive flag to all commands that need projectId
  • for all commands that do not have nonInteractive flag assume, nonInteractive: true just for the getProjectIdAsync call. With this solution, you need to be careful so the error message does not mention non-interactive flag.
  • change convention, I'm open to suggestions
  • do not enforce projecteId on all commands, you could just do that for build/submit/update/update:configure/build:configure.

I'm personally in favor of the second approach

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here is one potential sledgehammer approach: #1360

I'm also open to the second approach, though I do like the concept of standardization across all our commands, and being able to run all in a non-interactive manner and also optionally output json seems like a good thing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a unique case of making all commands interactive in that it theoretically only needs to be run once.

I proposed making the flow such that all commands require having run eas init at least once in the past and having that command set this up, but @EvanBacon mentioned that this wasn't a great pattern and that we should instead just do it for them and ask when we are about to make a mutation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make sense. Do you intend to land #1360 before this PR?

If not, I would still prefer if you consider one of the approaches I suggested as a temporary solution.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Going to go with approach 2 for now and then figure out a more general solution (don't want to block this).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Alright, looking into this more, I think the better interim approach is to add the non-interactive flag to commands that call getProjectIdAsync, ensureLoggedInAsync, or other utility methods like them that take an optional nonInteractive argument.

@wschurman wschurman Sep 16, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ended up with a hybrid where for each command I assessed whether it was meant to be non-interactive, interactive, or configurable interactive based on the flags (based on the set of logic you provided above). I also made some flag setups consistent (where we have json, also have non-interactive). I also fixed some non-interactive stuff by explicitly passing the flag to functions that use it.

Comment thread packages/eas-cli/src/project/projectUtils.ts
* use the owner/slug to identify an EAS project on the server, and attempt to save the
* EAS project ID to the appropriate field in the app config.
*/
export async function getProjectIdAsync(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's one problem with this function that I've been frustrated with in the last few weeks. Namely, lines 153-156. Even though this makes sense for production users, I use different environments (local/staging/production) with the same projects on a daily basis. That means if I already have the production projectId set in my app.json, I cannot use EXPO_STAGING=1 on the same project without manually editing app.json.

I think we could check here for the environment and apply the if from lines 153-156 only in production. What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this would be a good use case for app.config.js that changes the project ID based on the environment variable?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is one way to solve the issue. However, the problem is I create new projects really often to test some user reports / issues in isolation. After that, I usually remove the project directory. The solution you suggested means I need to manually create app.config.js every time I create a new project.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is definitely an interesting use case. I'm open to applying L153-156 only in production, but I do worry that even that may be fragile and more something we should just script independently of the code in the CLI. Other commands already rely upon it being in the actual resolved config (expo start for example), so for the purpose of repro'ing an issue it seems like the starting point should be something as close to what the end user is using as possible (projectId in resolved config).

Comment thread packages/eas-cli/src/commands/branch/list.ts Outdated
Comment thread packages/eas-cli/src/commands/branch/view.ts Outdated
@wschurman wschurman force-pushed the @wschurman/interactive-project-setup branch from 5a2b08d to 5e69b8b Compare September 16, 2022 04:58

@wkozyra95 wkozyra95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Assuming nonInteractive is false in case flag is not supported should be always safe, but adding a new nonInteractive flag can potentially be risky for some more complicated commands because the prompt might be nested deep inside the logic, I recommend looking carefully on cases like that.

@wschurman

Copy link
Copy Markdown
Member Author

Assuming nonInteractive is false in case flag is not supported should be always safe, but adding a new nonInteractive flag can potentially be risky for some more complicated commands because the prompt might be nested deep inside the logic, I recommend looking carefully on cases like that.

Cool, I'll audit the places I added it. Thanks for the pointer.

@wschurman wschurman force-pushed the @wschurman/interactive-project-setup branch from 5e69b8b to b180759 Compare September 16, 2022 19:41
@wschurman wschurman merged commit 5585874 into main Sep 16, 2022
@wschurman wschurman deleted the @wschurman/interactive-project-setup branch September 16, 2022 20:00
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