-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[expo-updates][cli] Update CLI to separate private keys from code signing certificate #16979
Conversation
if (value === undefined || value === null) { | ||
Log.exit(`${name} must not be null`); | ||
} | ||
if (typeof value !== type) { |
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.
Doesn't assertArgs already check the type?
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 it coerces the type according to the readme: https://github.com/vercel/arg/blob/main/README.md
Though it doesn't support required args: https://github.com/vercel/arg/blob/main/README.md#how-do-i-require-an-argument-with-arg
I think I prefer to leave it just to be sure since it's unclear to me what the lib does and doesn't do.
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.
arg does assert the value i.e. { foobar: String }
+ --foobar
= assertion about a missing value.
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.
Trying this out:
- it coerces the type, so using Number constructor in
arg
but passing a string argument in the CLI will just result in the value beingNaN
rather than throwing. Don't see a fix for this since the coercion happens in the framework. Maybe all args should be String and then conversion to Number should happen in application code? - it doesn't have a way to require args be present. Failing to pass an arg doesn't throw in
arg
code and instead makes to the application code. - Passing empty for an arg also results in
NaN
I believe, so--arg=
is also not validated by the framework.
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.
The answer to "how do I require an arg?" is "do it yourself", so we need a function like this one.
It sounds like the type checking in this function isn't needed because assertArgs handles coercion. We may want to add a NaN check (Number.isNaN) in addition to the null/undefined checks.
Suggestion: rename this function to requireArg
or enforceArg
since what it does is different the existing assertArgs
function and using a different verb will make that distinction clearer. Remove the type check since assertArgs does coercion. Maybe add a NaN check.
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.
Sounds good. Note that this whole discussion also applies to the new expo CLI from which these argument utils were copypasta'd.
Why
The private key should be treated as sensitive information (somewhat like apple credentials are in the old expo-cli). Therefore, when generated, they should be encouraged to be put in a separate directory from the code signing certificate and then either added to .gitignore or encrypted with git-crypt. The user still has the option to put them anywhere they like and store them however they desire (in or out of the repo).
How
Update the generation and configuration CLIs to take multiple paths.
Test Plan
Run tests.