Skip to content
This repository has been archived by the owner on Jan 18, 2024. It is now read-only.

[expo-cli] add --config flag to credentials:manager #2641

Merged
merged 3 commits into from Sep 15, 2020

Conversation

wkozyra95
Copy link
Contributor

Why

Support credentials manager for people that have multiple projects on a single codebase. For those cases, all credentials management had to be done via build command.

How

Support config flag. I couldn't use logic from exp.ts because that code requires a valid project and in the case of credentials:manager command it's optional

Copy link
Member

@quinlanj quinlanj left a comment

Choose a reason for hiding this comment

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

The code looks good to me.

Regarding the use case:
IIUC the user has separate configs (ie) app-1.json and app-2.json in the same project directory and they want to specify which one to use. Is this because people have published two separate expo apps within the same project directory?

@@ -20,8 +25,23 @@ export default function (program: CommanderStatic) {
.description('Manage your credentials')
.helpGroup('credentials')
.option('-p --platform <platform>', 'Platform: [android|ios]', /^(android|ios)$/i)
.option('--config [file]', 'Specify a path to app.json or app.config.js')
Copy link
Contributor

Choose a reason for hiding this comment

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

we should move the exp.ts logic into a method and attempt to reuse it since this is a lot of stylistic logic. Perhaps we could add a way to skip project validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, skip flag would be probably best solution.

I decided to go this way because it's a bit isolated use cases and we will probably remove that command when web gui will be ready, so i didn't want modify code that handles other command. I don't have strong opinion on that either way.

if (!(await fs.pathExists(configPath))) {
throw new Error(`File ${configPath} does not exist`);
}
ConfigUtils.setCustomConfigPath(projectDir, configPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc this particular method is somewhat fragile and shouldn't be used in more than one place.

Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

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

♻️ reuse existing

@wkozyra95
Copy link
Contributor Author

Replaced validateWithoutNetworkAsync with try catch round get config because validateWithoutNetworkAsync logs error

@wkozyra95 wkozyra95 force-pushed the @wkozyra95/credentials-manager-add-config-flag branch from 9f0cab1 to 2e59040 Compare September 15, 2020 11:04
@wkozyra95 wkozyra95 merged commit 16d43e0 into master Sep 15, 2020
@wkozyra95 wkozyra95 deleted the @wkozyra95/credentials-manager-add-config-flag branch September 15, 2020 14:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants