This repository has been archived by the owner on Jan 18, 2024. It is now read-only.
[expo-cli] command for syncing credentials.json and credentials on www #2460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dsokal
suggested changes
Aug 14, 2020
| { | ||
| type: 'select', | ||
| name: 'platform', | ||
| message: 'Do you want to update credentials for both platforms?', |
There was a problem hiding this comment.
Suggested change
| message: 'Do you want to update credentials for both platforms?', | |
| message: 'Which platform would you like to update?', |
Comment on lines
38
to
40
| { title: 'Android & iOS', value: BuildCommandPlatform.ALL }, | ||
| { title: 'only Android', value: BuildCommandPlatform.ANDROID }, | ||
| { title: 'only iOS', value: BuildCommandPlatform.IOS }, |
There was a problem hiding this comment.
Suggested change
| { title: 'Android & iOS', value: BuildCommandPlatform.ALL }, | |
| { title: 'only Android', value: BuildCommandPlatform.ANDROID }, | |
| { title: 'only iOS', value: BuildCommandPlatform.IOS }, | |
| { title: 'Android', value: BuildCommandPlatform.ANDROID }, | |
| { title: 'iOS', value: BuildCommandPlatform.IOS }, | |
| { title: 'both', value: BuildCommandPlatform.ALL }, |
| message: 'What do you want to do?', | ||
| choices: [ | ||
| { | ||
| title: 'Update credentials on Expo servers with local credentials.json content', |
There was a problem hiding this comment.
Suggested change
| title: 'Update credentials on Expo servers with local credentials.json content', | |
| title: 'Update credentials on the Expo servers with local credentials.json contents', |
| title: 'Update credentials on Expo servers with local credentials.json content', | ||
| value: 'remote', | ||
| }, | ||
| { title: 'Update local credentials.json with values from Expo servers', value: 'local' }, |
There was a problem hiding this comment.
Suggested change
| { title: 'Update local credentials.json with values from Expo servers', value: 'local' }, | |
| { title: 'Update or create local credentials.json with credentials from Expo servers', value: 'local' }, |
Comment on lines
1
to
2
| export { default as credentialsJson } from './credentialsJson'; | ||
| export { updateLocalCredentialsJsonAsync } from './update'; |
There was a problem hiding this comment.
What's the benefit of having a file like this? I think we've discussed this many time :D
Comment on lines
49
to
54
| const { confirm } = await prompts({ | ||
| type: 'confirm', | ||
| name: 'confirm', | ||
| message: | ||
| 'Credentials on Expo servers might be invalid or incomplete. Are you sure you want to continue?', | ||
| }); |
There was a problem hiding this comment.
Maybe we should ask whether the user wants to remove them from our servers?
There was a problem hiding this comment.
We are not proposing that if sth is missing when starting build so I don't think we need to that here.
To be honest, I'm not sure if there will be a case where this condition will be triggered, but I added it here because there is similiar check for ios and we could add validation for keystore in the future.
Comment on lines
61
to
63
| const keystorePath = | ||
| rawCredentialsJsonObject?.android?.keystorePath ?? './android/keystores/keystore.jks'; | ||
| await _updateFileAsync(ctx.projectDir, keystorePath, keystore.keystore); |
There was a problem hiding this comment.
I think we should inform the user that we created this file so he won't be surprised that it's been created.
| import log from '../../log'; | ||
| import { Context } from '../context'; | ||
|
|
||
| type Platform = 'android' | 'ios' | 'all'; |
There was a problem hiding this comment.
I can be wrong but I believe this is already defined somewhere.
There was a problem hiding this comment.
I didn't want to import it from build directory, and there it means sth else. I moved that function to credentialsSync/action.ts so Platform type is not necessary anymore
Comment on lines
83
to
84
| const rawFile = await fs.readFile(credentialsJsonFilePath); | ||
| rawCredentialsJsonObject = JSON.parse(rawFile.toString()); |
There was a problem hiding this comment.
Suggested change
| const rawFile = await fs.readFile(credentialsJsonFilePath); | |
| rawCredentialsJsonObject = JSON.parse(rawFile.toString()); | |
| const rawFile = await fs.readFile(credentialsJsonFilePath, 'utf-8'); | |
| rawCredentialsJsonObject = JSON.parse(rawFile); |
Comment on lines
129
to
134
| await _updateFileAsync( | ||
| ctx.projectDir, | ||
| pprofilePath, | ||
| appCredentials?.credentials?.provisioningProfile | ||
| ); | ||
| await _updateFileAsync(ctx.projectDir, distCertPath, distCredentials?.certP12); |
There was a problem hiding this comment.
Let's inform the user we created these files.
wkozyra95
force-pushed
the
@wkozyra95/credentials-sync-command
branch
from
500304f to
d843712
Compare
wkozyra95
force-pushed
the
@wkozyra95/credentials-sync-command
branch
from
d843712 to
66ff7e3
Compare
dsokal
approved these changes
Aug 18, 2020
| message: 'What do you want to do?', | ||
| choices: [ | ||
| { | ||
| title: 'Update credentials on the Expo servers with local credentials.json contents', |
There was a problem hiding this comment.
Suggested change
| title: 'Update credentials on the Expo servers with local credentials.json contents', | |
| title: 'Update credentials on the Expo servers with the local credentials.json contents', |
| value: 'remote', | ||
| }, | ||
| { | ||
| title: 'Update or create local credentials.json with credentials from Expo servers', |
There was a problem hiding this comment.
Suggested change
| title: 'Update or create local credentials.json with credentials from Expo servers', | |
| title: 'Update or create local credentials.json with credentials from the Expo servers', |
| if (!ctx.hasProjectContext) { | ||
| throw new Error('project context is required'); // should be checked earlier | ||
| } | ||
| if (['all', 'android'].includes(platform)) { |
There was a problem hiding this comment.
Suggested change
| if (['all', 'android'].includes(platform)) { | |
| if ([BuildCommandPlatform.ALL, BuildCommandPlatform.ANDROID].includes(platform)) { |
| const experienceName = `@${ctx.manifest.owner || ctx.user.username}/${ctx.manifest.slug}`; | ||
| await runCredentialsManager(ctx, new SetupAndroidBuildCredentialsFromLocal(experienceName)); | ||
| } | ||
| if (['all', 'ios'].includes(platform)) { |
There was a problem hiding this comment.
Suggested change
| if (['all', 'ios'].includes(platform)) { | |
| if ([BuildCommandPlatform.ALL, BuildCommandPlatform.IOS].includes(platform)) { |
| @@ -0,0 +1,70 @@ | |||
| import { ExpoConfig, IOSConfig } from '@expo/config'; | |||
| @@ -0,0 +1,98 @@ | |||
| import CommandError from '../../../CommandError'; | |||
| import { Context } from '../../../credentials/context'; | |||
| import * as credentialsJson from '../../../credentials/credentialsJson/update'; | |||
There was a problem hiding this comment.
Suggested change
| import * as credentialsJson from '../../../credentials/credentialsJson/update'; | |
| import * as credentialsJsonUpdateUtils from '../../../credentials/credentialsJson/update'; |
There was a problem hiding this comment.
Suggested change
| import * as credentialsJson from '../../../credentials/credentialsJson/update'; | |
| import { updateAndroidCredentialsAsync, updateIosCredentialsAsync } from '../../../credentials/credentialsJson/update'; |
wkozyra95
force-pushed
the
@wkozyra95/credentials-sync-command
branch
from
66ff7e3 to
49f6580
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
We need a simple way to switch between different methods of providing credentials
How
Added new command
Test plan