[cli] add support back for owner field when cred checking #1941
Conversation
quinlanj
force-pushed
the
@quin/fixOwnerRegression
branch
from
4d515d7
to
580fe98
Compare
quinlanj
changed the title
| @@ -31,7 +32,8 @@ export async function fetchAndroidKeystoreAsync(projectDir: string): Promise<voi | |||
| await maybeRenameExistingFile(projectDir, keystoreFilename); | |||
| const backupKeystoreOutputPath = path.resolve(projectDir, keystoreFilename); | |||
|
|
|||
| const view = new DownloadKeystore(ctx.manifest.slug); | |||
| invariant(ctx.manifest.slug, 'app.json slug field must be set'); | |||
There was a problem hiding this comment.
in current expo-cli versions we set the slug automatically if it's not provided in app.json, so it would certainly be weird to get into this state! good to have that invariant
There was a problem hiding this comment.
were we encountering cases where slug was not set?
There was a problem hiding this comment.
i changed the type of manifest from any to ExpoConfig in context.ts, and slug could be undefined in this case - so i added invariant checks to get rid of the errors
| @@ -64,6 +70,9 @@ export class Context { | |||
| const { exp } = getConfig(projectDir); | |||
| this._manifest = exp; | |||
| this._hasProjectContext = true; | |||
| this._iosApiClient = new IosApi(this.user).withProjectContext(this); | |||
There was a problem hiding this comment.
could use some more context on what's going on here - why do create an api client if the status is fatal? and why not with the project context?
There was a problem hiding this comment.
i updated this with better comments -- basically we use Doctor.validateWithoutNetworkAsync to check whether there is a manifest in the project directory. If there is (we get a non fatal response), we initialize the necesary project context things, else we do a normal initialization
Codecov Report
@@ Coverage Diff @@
## master #1941 +/- ##
==========================================
+ Coverage 44.52% 44.90% +0.39%
==========================================
Files 99 103 +4
Lines 3956 4283 +327
Branches 976 1036 +60
==========================================
+ Hits 1761 1923 +162
- Misses 1513 1635 +122
- Partials 682 725 +43 |
fix errors when changing manifest type bug fixes from tests
quinlanj
force-pushed
the
@quin/fixOwnerRegression
branch
from
43ac7e0
to
3ff84d0
Compare
why
We used to support builds where if User A gave User B permission to build their app, User B would be able to build User A's app. However, User B would need to specify
UserAwas the owner of the app in the owner field of theapp.json.When we moved to the new credential checker, we overlooked this use case. We add it back now by doing the following:
usernamefield to credential's IosApiownerin the api's request body or query parameter as appropriatetodo