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 2 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
2 changes: 1 addition & 1 deletion packages/eas-cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"@expo/config": "8.1.2",
"@expo/config-plugins": "7.2.4",
"@expo/config-types": "49.0.0",
"@expo/eas-build-job": "1.0.43",
"@expo/eas-build-job": "1.0.46",
"@expo/eas-json": "5.3.1",
"@expo/json-file": "8.2.37",
"@expo/multipart-body-parser": "1.1.0",
Expand Down
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 @@ -26,10 +26,7 @@ export async function collectMetadataAsync<T extends Platform>(
): Promise<Metadata> {
const vcsClient = getVcsClient();
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>(
buildMode: ctx.buildProfile.config ? BuildMode.CUSTOM : BuildMode.BUILD,
customWorkflowName: ctx.customBuildConfigMetadata?.workflowName,
developmentClient: ctx.developmentClient,
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',
}
2 changes: 1 addition & 1 deletion packages/eas-json/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"bugs": "https://github.com/expo/eas-cli/issues",
"dependencies": {
"@babel/code-frame": "7.18.6",
"@expo/eas-build-job": "1.0.39",
"@expo/eas-build-job": "1.0.46",
"chalk": "4.1.2",
"env-string": "1.0.1",
"fs-extra": "10.1.0",
Expand Down
16 changes: 4 additions & 12 deletions yarn.lock
View file
Open in desktop

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