-
Notifications
You must be signed in to change notification settings - Fork 85
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-12115][eas-cli] add eas onboarding flow #2338
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2338 +/- ##
==========================================
- Coverage 53.83% 53.64% -0.19%
==========================================
Files 525 530 +5
Lines 19275 19479 +204
Branches 4074 4120 +46
==========================================
+ Hits 10375 10447 +72
- Misses 8180 8302 +122
- Partials 720 730 +10 ☔ View full report in Codecov by Sentry. |
e15b190
to
41b58b0
Compare
Size Change: +7.5 kB (0%) Total Size: 51.3 MB
|
41b58b0
to
5026f1e
Compare
|
||
if (!actor.preferences.onboarding) { | ||
throw new Error( | ||
'This command is a continuation of the onboarding process started on the Expo website. Start the onboarding process on the website before running this command. Visit https:/expo.new to create an account and start the onboarding process.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: https:/expo.new -> https://expo.new
actor: Actor; | ||
}): DynamicConfigContextFn { | ||
return async (options?: ExpoConfigOptions) => { | ||
const expBefore = getPrivateExpoConfig(projectDir, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not reuse getPrivateExpoConfigWithProjectIdAsync
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sure
@@ -17,6 +17,10 @@ import { | |||
import { Client } from '../vcs'; | |||
|
|||
export default class GitClient extends Client { | |||
constructor(private maybeCwdOverride?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to instead have this.cwd = findWorkspaceRoot(cwd) ?? cwd
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are not always running it workspace root (for example when git adding something), this can be risky and can break something
appId | ||
isCLIDone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting — can a single user only have a single preferences entry for a single app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so, cc: @tchayen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! This is an interactive wizard on the website. We don't allow people to run more than one at the same time. If they have such a need they probably don't need an interactive wizard in the first place.
And everything we do here can be automated as well by the user if they ever need that.
const githubUsername = app.githubRepository | ||
? app.githubRepository.metadata.githubRepoOwnerName | ||
: 'expo'; | ||
const githubRepositoryName = app.githubRepository | ||
? app.githubRepository.metadata.githubRepoName | ||
: 'expo-default-template'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if one is defined, but not the other we'd try to clone sjchmiela/expo-default-template
? Seems like we shouldn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both githubRepoOwnerName
and githubRepoName
are reguired on metadata
object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I believe there should never be a case when one exists and other not
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |
Why
Add onboarding flow for EAS
https://linear.app/expo/issue/ENG-12115/add-eas-onboarding-command-to-automatically-execute-all-of-the
How
Add onboarding flow for EAS
Test Plan
Test manually