Skip to content
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-1271] make iOS credentials manager work with multi-target projects #441

Merged
merged 5 commits into from
Jun 9, 2021

Conversation

dsokal
Copy link
Contributor

@dsokal dsokal commented Jun 4, 2021

Checklist

  • I've added an entry to CHANGELOG.md if necessary.
  • I've tagged the changelog entry with [EAS BUILD API] if it's a part of a breaking change to EAS Build API (only possible when updating @expo/eas-build-job package).

Why

How

I know this is quite big PR but I didn't know how to divide work into smaller pieces.

Notable changes:

  • Manager can now print credentials for all targets - similarly to how SetupBuildCredentials prints the summary after all targets are set up.
  • When a project has multiple schemes, the user is asked which scheme they want to configure.
  • If the scheme has multiple targets and the user selects an action that works in the scope of the target credentials (UseExistingDistributionCertificate, CreateDistributionCertificate and RemoveProvisioningProfile) they are asked which target to use.
  • I changed the default directory for backing up credentials on the local machine (this fixes the linear task):
    • ios/certs -> credentials/ios
    • android/keystores -> credentials/android

Test Plan

I tested credentials manager manually (by executing all available commands) for 3 different projects:

  • multi-target generic project
  • single-target generic project
  • managed project

@dsokal dsokal force-pushed the @dsokal/multitarget-credentials-manager branch from 104f046 to 7076162 Compare June 4, 2021 10:32
@github-actions
Copy link

github-actions bot commented Jun 4, 2021

Size Change: +2 kB (0%)

Total Size: 37.5 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 37.5 MB +2 kB (0%)

compressed-size-action

@dsokal dsokal force-pushed the @dsokal/multitarget-credentials-manager branch 2 times, most recently from ca992ea to 818b23d Compare June 4, 2021 11:58
@dsokal dsokal changed the title @dsokal/multitarget credentials manager make ios credentials manager work with multitarget projects Jun 4, 2021
@dsokal dsokal requested a review from wkozyra95 June 4, 2021 11:59
@dsokal dsokal changed the title make ios credentials manager work with multitarget projects [ENG-1271] make ios credentials manager work with multitarget projects Jun 7, 2021
@linear
Copy link

linear bot commented Jun 7, 2021

ENG-1271 eas credentials downloads to ios/certs, which conflicts with workflow detection

If you have a managed project and run eas credentials and download credentials.json then eas-cli will start assuming you have a bare project.

Screen_Shot_2021-06-04_at_12.30.19_PM.png

@dsokal dsokal force-pushed the @dsokal/multitarget-credentials-manager branch 4 times, most recently from 2d2dd5b to a7edaeb Compare June 7, 2021 09:20
@dsokal dsokal force-pushed the @dsokal/multitarget-credentials-manager branch from a7edaeb to a296bc0 Compare June 7, 2021 09:30
@dsokal dsokal changed the title [ENG-1271] make ios credentials manager work with multitarget projects [ENG-1271] make iOS credentials manager work with multi-target projects Jun 7, 2021
@dsokal dsokal marked this pull request as ready for review June 7, 2021 10:14
@dsokal dsokal requested a review from quinlanj June 7, 2021 10:14
if (notConfiguredTargets.length > 0) {
throw new Error(
`Credentials for target${
notConfiguredTargets.length === 1 ? '' : 's'
Copy link
Contributor

Choose a reason for hiding this comment

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

if there is only one target error message should state sth less specific e.g. credentials are not configured or sth like this, without talking about targets

const provisioningProfilePath: string =
currentRawTargetCredentialsObject?.provisioningProfilePath ??
`credentials/ios/${target.targetName}-profile.mobileprovision`;
const distCertPath: string =
Copy link
Contributor

Choose a reason for hiding this comment

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

what if existing paths pointed to the same file on the disk, when updating you will replace it for all of them

exp: ctx.exp,
},
{
workflow: buildProfile.workflow,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe pass buildProfile here

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.

thanks!

Comment on lines +63 to +65
targets: Target[]
): void {
const projectFullName = `@${app.account.name}/${app.projectName}`;
Copy link
Member

Choose a reason for hiding this comment

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

ignore this suggestion if theres an edge case (ie) targets is a subset of the appCredentials keys

Suggested change
targets: Target[]
): void {
const projectFullName = `@${app.account.name}/${app.projectName}`;
): void {
const targets = Object.keys(appCredentials);
const projectFullName = `@${app.account.name}/${app.projectName}`;

Copy link
Contributor Author

@dsokal dsokal Jun 9, 2021

Choose a reason for hiding this comment

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

I use the Target[] type here because it contains bundle identifiers of the targets. If the build credentials for a target in appCredentials don't exist, then I couldn't determine the bundle id. See line 79.

export function displayIosAppCredentials(credentials: CommonIosAppCredentialsFragment): void {
export function displayIosCredentials(
app: App,
appCredentials: IosAppCredentialsMap,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
appCredentials: IosAppCredentialsMap,
appCredentialsMap: IosAppCredentialsMap,

Comment on lines 99 to 126
app = { account, projectName: ctx.exp.slug };
const easJsonReader = new EasJsonReader(ctx.projectDir, 'ios');
const easConfig = await new SelectBuildProfileFromEasJson(easJsonReader).runAsync(ctx);
buildProfile = nullthrows(easConfig.builds.ios, 'iOS build profile must be defined');
const xcodeBuildContext = await resolveXcodeBuildContextAsync(
{
projectDir: ctx.projectDir,
nonInteractive: ctx.nonInteractive,
exp: ctx.exp,
},
{
workflow: buildProfile.workflow,
...(buildProfile.workflow === Workflow.GENERIC && {
buildConfiguration: buildProfile.schemeBuildConfiguration,
buildScheme: buildProfile.scheme,
}),
}
);
if (!iosAppCredentials) {
displayEmptyIosCredentials(appLookupParams);
} else {
displayIosAppCredentials(iosAppCredentials);
targets = await resolveTargetsAsync(
{ exp: ctx.exp, projectDir: ctx.projectDir },
xcodeBuildContext
);
const iosAppCredentialsMap: IosAppCredentialsMap = {};
for (const target of targets) {
const appLookupParams = getAppLookupParamsFromContext(ctx, target);
iosAppCredentialsMap[
target.targetName
] = await ctx.ios.getIosAppCredentialsWithCommonFieldsAsync(appLookupParams);
Copy link
Member

Choose a reason for hiding this comment

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

might be cleaner to refactor this into a separate method that returns {isProjectContext: true, app: App, targets: Target[], buildProfile: BuildProfile} | {isProjectContext: false, app: null, targets: null, buildProfile: null} and populate it on line 83-86 (moving line 87-96 out of the while loop so you can use account)

@dsokal dsokal force-pushed the @dsokal/multitarget-credentials-manager branch from 3982742 to b03c1c9 Compare June 9, 2021 11:41
@dsokal dsokal force-pushed the @dsokal/multitarget-credentials-manager branch from b03c1c9 to fe314e7 Compare June 9, 2021 11:52
@dsokal dsokal merged commit 2bdaa0d into main Jun 9, 2021
@dsokal dsokal deleted the @dsokal/multitarget-credentials-manager branch June 9, 2021 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants