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

[ENG-12793][eas-cli] make eas login and eas whoami behave more clearly when one is authenticated using EXPO_TOKEN #2461

Merged

Conversation

szdziedzic
Copy link
Member

@szdziedzic szdziedzic commented Jul 16, 2024

Why

Using EXPO_TOKEN seems confusing to people.

#2460

How

When using the eas whoami command signal that a user is authenticated by using EXPO_TOKEN. It seems like people sometimes tend to forget that they have EXPO_TOKEN set, then they log in as a different account but eas whoami still returns the same value because they are authed using EXPO_TOKEN and it seems to be confusing.

Warn people when running eas login with the EXPO_TOKEN set that this is not necessary.

Test Plan

Test manually

eas whoami after and before
Screenshot 2024-07-16 at 16 39 28

eas login
Screenshot 2024-07-16 at 16 52 52
Screenshot 2024-07-16 at 16 53 06
Screenshot 2024-07-16 at 16 53 39

Copy link

github-actions bot commented Jul 16, 2024

Size Change: -455 B (0%)

Total Size: 52 MB

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

compressed-size-action

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 53.41%. Comparing base (17ecdb2) to head (fd2b914).

Files Patch % Lines
packages/eas-cli/src/commands/account/login.ts 33.34% 8 Missing ⚠️
packages/eas-cli/src/commands/account/view.ts 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2461      +/-   ##
==========================================
- Coverage   53.43%   53.41%   -0.02%     
==========================================
  Files         530      530              
  Lines       19564    19577      +13     
  Branches     3979     3984       +5     
==========================================
+ Hits        10453    10456       +3     
- Misses       9089     9099      +10     
  Partials       22       22              

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

@szdziedzic szdziedzic requested a review from wschurman July 16, 2024 15:45
@szdziedzic szdziedzic marked this pull request as ready for review July 16, 2024 15:45
} = await this.getContextAsync(AccountLogin, { nonInteractive: false });

if (sessionManager.getAccessToken()) {
Log.warn(
Copy link
Member

@wschurman wschurman Jul 16, 2024

Choose a reason for hiding this comment

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

IMO we should just throw an error when eas login is run when the EXPO_TOKEN is set. Maybe say something like: "EXPO_TOKEN is set in your environment, and is being used for all EAS authentication. To use username/password authentication, unset EXPO_TOKEN in your environment and re-run the command.".

@szdziedzic szdziedzic requested a review from wschurman July 17, 2024 09:55
@szdziedzic szdziedzic changed the title [eas-cli] make eas login and eas whoami behave more clearly when one is authenticated using EXPO_TOKEN [ENG-12793][eas-cli] make eas login and eas whoami behave more clearly when one is authenticated using EXPO_TOKEN Jul 17, 2024
Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

One wording change, otherwise lgtm.

Copy link

✅ Thank you for adding the changelog entry!

@szdziedzic szdziedzic merged commit 5745639 into main Jul 18, 2024
6 checks passed
@szdziedzic szdziedzic deleted the @szdziedzic/make-method-used-for-authentication-more-visible branch July 18, 2024 09:51
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