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.

Sign up for GitHub

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

[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds #2073

Merged
merged 24 commits into from
Jan 31, 2024
Merged

[eas-cli] [ENG-9957] Don't overwrite distribution for simulator builds #2073

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c48928b
[eas-cli] Update dependencies
radoslawkrzemien Oct 2, 2023
7679dee
[eas-cli] Don't overwrite distribution
radoslawkrzemien Oct 2, 2023
4b41ba2
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…
radoslawkrzemien Oct 30, 2023
de4712a
[eas-cli] Merge main
radoslawkrzemien Oct 30, 2023
2dd7e3b
[eas-cli] Check for simulator flag
radoslawkrzemien Oct 30, 2023
c9d25b7
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…
radoslawkrzemien Dec 13, 2023
35b99d7
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…
radoslawkrzemien Jan 18, 2024
f9b738a
[eas-cli] Update schema
radoslawkrzemien Jan 19, 2024
4e1da63
[eas-cli] Add flag
radoslawkrzemien Jan 19, 2024
cf09ead
[eas-cli] Fix check
radoslawkrzemien Jan 19, 2024
9e25d60
[eas-cli] Fix tests
radoslawkrzemien Jan 19, 2024
dfa81f3
[eas-cli] Add warning
radoslawkrzemien Jan 19, 2024
e19f1cb
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…
radoslawkrzemien Jan 22, 2024
9b442e2
[eas-cli] Add description
radoslawkrzemien Jan 23, 2024
7bc9744
[eas-cli] Add validation
radoslawkrzemien Jan 23, 2024
c15b9dd
[eas-cli] Change message
radoslawkrzemien Jan 23, 2024
12895bd
[eas-cli] Replace string
radoslawkrzemien Jan 23, 2024
935dcbc
[eas-cli] Remove redundant check
radoslawkrzemien Jan 23, 2024
24156cd
[eas-cli] Update schema
radoslawkrzemien Jan 23, 2024
1fce06f
[eas-cli] Update schema
radoslawkrzemien Jan 23, 2024
59e7978
update CHANGELOG.md
radoslawkrzemien Jan 30, 2024
dd45591
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…
radoslawkrzemien Jan 30, 2024
577edaa
Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…
radoslawkrzemien Jan 31, 2024
049f177
[eas-cli] Update schema
radoslawkrzemien Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions packages/eas-cli/graphql.schema.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/eas-cli/src/build/__tests__/cancel-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,5 +92,6 @@ function createMockBuildFragment({
},
status: BuildStatus.InQueue,
resourceClass: BuildResourceClass.AndroidMedium,
isForSimulator: false,
};
}
1 change: 1 addition & 0 deletions packages/eas-cli/src/build/__tests__/delete-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,5 +92,6 @@ function createMockBuildFragment({
},
status: BuildStatus.InQueue,
resourceClass: BuildResourceClass.AndroidMedium,
isForSimulator: false,
};
}
6 changes: 2 additions & 4 deletions packages/eas-cli/src/build/metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@ export async function collectMetadataAsync<T extends Platform>(
ctx: BuildContext<T>
): Promise<Metadata> {
const channelOrReleaseChannel = await resolveChannelOrReleaseChannelAsync(ctx);
const distribution =
('simulator' in ctx.buildProfile && ctx.buildProfile.simulator
? 'simulator'
: ctx.buildProfile.distribution) ?? 'store';
const distribution = ctx.buildProfile.distribution ?? 'store';
Copy link
Member

Choose a reason for hiding this comment

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

This change will break stuff like eas build:run 🤔

I'm wondering if altering the distributionType behavior is necessary 🤔.

Maybe we can add some new field to Metadata to keep the raw buildProfile.distribution value there? I mean it's probably not optimal but I feel that this change can introduce some regressions in some parts of the codebase.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on why eas build:run would be broken by this?
As for another field for "original distribution" or whatever we want to call it, I'm open for discussion but it feels to me like hacking around an existing hack (overwriting the distribution). I feel like we should remove the reliance on this hardcoded simulator value for distribution instead of building upon it, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, it runs a simulator build and fetches it by filtering by distribution = simulator.
Maybe it should handle both options (distribution = simulator or simulator = true) for a while instead of introducing another field?

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur, we should list builds with both simulator: true and distribution: simulator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's handled in https://github.com/expo/universe/pull/13524

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
const distribution = ctx.buildProfile.distribution ?? 'store';
const distribution = ctx.buildProfile.distribution ?? BuildDistributionType.STORE;

const metadata: Metadata = {
trackingContext: ctx.analyticsEventProperties,
...(await maybeResolveVersionsAsync(ctx)),
Expand Down Expand Up @@ -64,6 +61,7 @@ export async function collectMetadataAsync<T extends Platform>(
requiredPackageManager: ctx.requiredPackageManager ?? undefined,
selectedImage: ctx.buildProfile.image,
customNodeVersion: ctx.buildProfile.node,
simulator: 'simulator' in ctx.buildProfile && ctx.buildProfile.simulator,
};
return sanitizeMetadata(metadata);
}
Expand Down
1 change: 1 addition & 0 deletions packages/eas-cli/src/build/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ export enum BuildStatus {
export enum BuildDistributionType {
STORE = 'store',
INTERNAL = 'internal',
/** @deprecated Use simulator flag instead */
SIMULATOR = 'simulator',
}
8 changes: 8 additions & 0 deletions packages/eas-cli/src/commands/build/list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getPaginatedQueryOptions,
} from '../../commandUtils/pagination';
import { AppPlatform, BuildStatus as GraphQLBuildStatus } from '../../graphql/generated';
import Log from '../../log';
import { RequestedPlatform } from '../../platform';
import { getDisplayNameForProjectIdAsync } from '../../project/projectUtils';
import { buildDistributionTypeToGraphQLDistributionType } from '../../utils/buildDistribution';
Expand Down Expand Up @@ -51,6 +52,7 @@ export default class BuildList extends EasCommand {
...EasPaginatedQueryFlags,
limit: getLimitFlagWithCustomValues({ defaultTo: 10, limit: BUILDS_LIMIT }),
...EasNonInteractiveAndJsonFlags,
simulator: Flags.boolean(),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be good to add a description to this flag, putting emphasis on the fact that it will only filter the iOS simulator builds. People tend to confuse Android emulator with iOS simulator and one could think that this will also query for Android emulator builds.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should also enforce that this is only applicable when using the ios value for the --platform flag? What do you think?

};

static override contextDefinition = {
Expand All @@ -69,6 +71,11 @@ export default class BuildList extends EasCommand {
distribution: buildDistribution,
'non-interactive': nonInteractive,
} = flags;
if (buildDistribution === BuildDistributionType.SIMULATOR) {
Log.warn(
`Using "distribution" flag with "simulator" value is deprecated - use "simulator" flag instead`
);
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👏

Suggested change
Log.warn(
`Using "distribution" flag with "simulator" value is deprecated - use "simulator" flag instead`
);
Log.warn(
`Using --distribution flag with "simulator" value is deprecated - use --simulator flag instead`
);

}
const {
privateProjectConfig: { projectId },
loggedIn: { graphqlClient },
Expand Down Expand Up @@ -100,6 +107,7 @@ export default class BuildList extends EasCommand {
appIdentifier: flags.appIdentifier,
buildProfile: flags.buildProfile,
gitCommitHash: flags.gitCommitHash,
simulator: flags.simulator,
},
paginatedQueryOptions,
});
Expand Down
9 changes: 4 additions & 5 deletions packages/eas-cli/src/commands/build/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
PaginatedQueryOptions,
getPaginatedQueryOptions,
} from '../../commandUtils/pagination';
import { AppPlatform, BuildFragment, BuildStatus, DistributionType } from '../../graphql/generated';
import { AppPlatform, BuildFragment, BuildStatus } from '../../graphql/generated';
import { BuildQuery } from '../../graphql/queries/BuildQuery';
import Log from '../../log';
import { appPlatformDisplayNames } from '../../platform';
Expand Down Expand Up @@ -200,8 +200,7 @@ async function maybeGetBuildAsync(
projectId: string,
paginatedQueryOptions: PaginatedQueryOptions
): Promise<BuildFragment | null> {
const distributionType =
flags.selectedPlatform === AppPlatform.Ios ? DistributionType.Simulator : undefined;
const simulator = flags.selectedPlatform === AppPlatform.Ios ? true : undefined;

if (flags.runArchiveFlags.id) {
const build = await BuildQuery.byIdAsync(graphqlClient, flags.runArchiveFlags.id);
Expand All @@ -219,9 +218,9 @@ async function maybeGetBuildAsync(
} build to run for ${await getDisplayNameForProjectIdAsync(graphqlClient, projectId)} app`,
filter: {
platform: flags.selectedPlatform,
distribution: distributionType,
status: BuildStatus.Finished,
buildProfile: flags.profile,
simulator,
},
paginatedQueryOptions,
selectPromptDisabledFunction: build => !isRunnableOnSimulatorOrEmulator(build),
Expand All @@ -235,9 +234,9 @@ async function maybeGetBuildAsync(
projectId,
filter: {
platform: flags.selectedPlatform,
distribution: distributionType,
status: BuildStatus.Finished,
buildProfile: flags.profile,
simulator,
},
});

Expand Down
Loading
Loading