[eas-cli] [ENG-11026] Add build:delete command #2178

radoslawkrzemien · 2024-01-12T14:36:28Z

Why

https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

How

Added a new build:delete command. It shares some similarities with build:cancel command, can be used with build id provided or prompt the user for id if not (not available in non-interactive mode). The difference is that build:cancel has restrictions on build status, and build:delete does not. The shared code has been extracted to a helper file under src/commandUtils. Additionally, 2 filter flags has been added for the case when the user does not provide the ID of the build to be deleted and a list of builds is printed, it can be filtered by platform and/or profile. The same has been applied to build:cancel as well

Screens

With build id

1

2

With prompt

1

2

3

Non-interactive with id

1

Non-interactive with prompt (not allowed)

1

New filter flags

No flags

`build:delete` with `platform/p` flag

1

2

`build:delete` with `profile/e` flag

1

2

Same for `build:cancel` command

1

2

3

Using both build ID and filter flags is not allowed

1

2

Test Plan

Added tests similar to those for build:cancel
Tested manually

Deployment Plan

No dependency on other services, just release a new version of CLI when needed

Added a new command build:delete. Shares some code with build:cancel, so I have moved the redundancies to a utils file at src/commandUtils/builds See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Added a test file based on existing tests for build:cancel command. Adding more tests See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

linear · 2024-01-12T14:36:31Z

ENG-11026 Add eas build:delete command

github-actions · 2024-01-12T14:39:58Z

Size Change: +5.42 kB (0%)

Total Size: 55.5 MB

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

_{compressed-size-action}

codecov · 2024-01-12T14:40:25Z

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (189d163) 54.23% compared to head (f2e1d05) 54.24%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
packages/eas-cli/src/commands/build/delete.ts	50.88%	28 Missing ⚠️
packages/eas-cli/src/commandUtils/builds.ts	63.05%	17 Missing ⚠️
packages/eas-cli/src/commands/build/cancel.ts	50.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2178      +/-   ##
==========================================
+ Coverage   54.23%   54.24%   +0.02%     
==========================================
  Files         509      511       +2     
  Lines       18660    18756      +96     
  Branches     3737     3758      +21     
==========================================
+ Hits        10118    10173      +55     
- Misses       8521     8562      +41     
  Partials       21       21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sjchmiela

Cool!

sjchmiela · 2024-01-15T08:44:58Z

packages/eas-cli/src/commandUtils/builds.ts

+        limit: 10,
+        filter: { status },
+      });
+      builds.push(...buildsForStatus);


Should we order them by createdAt?

I think they are sorted like that implicitly, but need to double check

They were, but it fell apart when combining results for different statuses, as this would group the builds by status too. Added explicit sorting before returning

sjchmiela · 2024-01-15T08:46:22Z

packages/eas-cli/src/commandUtils/builds.ts

+  graphqlClient: ExpoGraphqlClient,
+  projectId: string,
+  statuses?: BuildStatus[]


I would use a { graphqlClient, projectId, statuses } argument. This way, if we add more filters (like platform: Platform[]) it's much easier and readable.

sjchmiela · 2024-01-15T08:48:33Z

packages/eas-cli/src/commands/build/delete.ts

+    throw error;
+  }
+  if (builds.length === 0) {
+    Log.warn(`There aren't any builds for the project ${projectDisplayName}.`);


Suggested change

Log.warn(`There aren't any builds for the project ${projectDisplayName}.`);

Log.warn(`There are no builds for the project ${projectDisplayName}.`);

Or maybe something like "we couldn't find any builds"? I think it feels more human?

The message was copied from an existing command, but I can update both 😄

sjchmiela · 2024-01-15T08:52:47Z

packages/eas-cli/src/commands/build/delete.ts

+    let buildId: string | null = buildIdFromArg;
+    if (!buildId) {
+      if (nonInteractive) {
+        throw new Error('BUILD_ID must not be empty in non-interactive mode');


Do users know the argument is called BUILD_ID? If not, maybe we should changes this to say "The build ID argument"?

szdziedzic

It looks really good 🙌

szdziedzic · 2024-01-15T08:58:45Z

packages/eas-cli/src/commandUtils/builds.ts

+      const buildsForStatus = await BuildQuery.viewBuildsOnAppAsync(graphqlClient, {
+        appId: projectId,
+        offset: 0,
+        limit: 10,
+        filter: { status },
+      });


Can we just create promises here and await them all using Promise.all to make it faster, instead of executing it sequentially?

It's so obvious, and yet I always forget about that 😬

szdziedzic · 2024-01-15T09:03:55Z

packages/eas-cli/src/commands/build/delete.ts

+export default class BuildDelete extends EasCommand {
+  static override description = 'delete a build';
+  static override args = [{ name: 'BUILD_ID' }];
+  static override flags = {


Maybe we can allow users to filter by platform and profile as well?

WDYM? If they provide platform to delete all builds from a platform? That seems rather... prone to human errors

Oh, I meant, when displaying the list of builds. So the people can decide what filters should be applied to this list.

Added the filter flags for platform and profile

Explicitly sorts fetched builds by descending order of `created_at`. In case multiple results were combined (for different statuses) they would by already sorted, but grouped by status also See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Changed `fetchBuildsAsync` arguments into single object argument for readability See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Changed the message displayed when no builds are found for deletion/cancellation to be more human-like See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Changed the message displayed when no build ID is provided for non-interactive mode, to not include the argument name but rather its description See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Changed for loop with async calls into await promise.all() See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

When not providing an ID of the build to delete, the user can now filter the listed builds by platform and build profile using new flags. The same change has also been applied to `build:cancel` command See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

radoslawkrzemien · 2024-01-16T13:07:03Z

/changelog-entry new-feature Added build:delete command

radoslawkrzemien · 2024-01-16T13:08:01Z

/changelog-entry new-feature Add filter flags for platform and profile to build:cancel and build:delete commands

Using imperative form in the changelog See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

…-11026-add-build-delete-command # Conflicts: # CHANGELOG.md

szdziedzic

Nice! I have a few last suggestions 🙌

szdziedzic · 2024-01-16T13:05:32Z

packages/eas-cli/src/commandUtils/builds.ts

+  let builds: BuildFragment[];
+  const queryFilters: InputMaybe<BuildFilter> = {};
+  if (filters?.platform && filters?.platform !== 'all') {
+    queryFilters['platform'] = filters?.platform.toUpperCase() as AppPlatform;


What do you think about using some mapping from one type to the other type here? By doing this TS will warn us if something type-related changes and it will be more explicit (for example we will see if due to some changes the toUpperCase isn't enough)

Added ~~a mapping function~~ the mapping

szdziedzic · 2024-01-16T13:10:15Z

packages/eas-cli/src/commandUtils/builds.ts

+    });
+  } else {
+    builds = (
+      await Promise.all(


szdziedzic · 2024-01-16T13:17:17Z

packages/eas-cli/src/commands/build/cancel.ts

+    platform: Flags.string({
+      char: 'p',
+      description: 'Platform for which to list builds when ID not provided',
+      options: ['android', 'ios', 'all'],
+    }),
+    profile: Flags.string({
+      char: 'e',
+      description:
+        'Name of the build profile from eas.json. Defaults to "production" if defined in eas.json.',
+      helpValue: 'PROFILE_NAME',
+    }),


This is probably a nitpick, but can we throw an error if the combination of build ID and these flags is provided? I want to avoid any confusion for the users here. What I mean is a situation where users believe this flag will only delete a build with such id if it is of a given platform and profile.

That's a valid point

szdziedzic · 2024-01-16T13:18:33Z

packages/eas-cli/src/commands/build/cancel.ts

 export default class BuildCancel extends EasCommand {
  static override description = 'cancel a build';

  static override args = [{ name: 'BUILD_ID' }];

  static override flags = {
    ...EASNonInteractiveFlag,
+    platform: Flags.string({
+      char: 'p',
+      description: 'Platform for which to list builds when ID not provided',


Suggested change

description: 'Platform for which to list builds when ID not provided',

description: 'Filter builds by the platform if BUILD_ID is not provided',

WDYT?

szdziedzic · 2024-01-16T13:19:09Z

packages/eas-cli/src/commands/build/cancel.ts

+    profile: Flags.string({
+      char: 'e',
+      description:
+        'Name of the build profile from eas.json. Defaults to "production" if defined in eas.json.',


Suggested change

'Name of the build profile from eas.json. Defaults to "production" if defined in eas.json.',

'Filter builds by profile if BUILD_ID is not provided',

Disallow using both the build ID and filter flags when using `build:delete` and `build:cancel` commands See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Updated the descriptions of the filter flags as per suggestion during review See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Added function for mapping platform flag into generated graphQL value See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

sjchmiela · 2024-01-16T14:27:44Z

packages/eas-cli/src/commandUtils/builds.ts

+function toAppPlatform(platform: string): AppPlatform {
+  const capitalizedPlatform = (platform[0].toUpperCase() +
+    platform.substring(1)) as keyof typeof AppPlatform;
+  return AppPlatform[capitalizedPlatform];
+}


We could also do

const platformToAppPlatform: Record<string, AppPlatform | undefined> = { android: AppPlatform.Android, ios: AppPlatform.Ios, };

A bit simpler and safer.

I like that

Replaced function with a mapping object See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

szdziedzic

Please don't kill me, these are the last 3 comments that can allow you to type everything better 🙈

szdziedzic · 2024-01-16T14:41:49Z

packages/eas-cli/src/commandUtils/builds.ts

+const platformToAppPlatform: Record<string, AppPlatform | undefined> = {
+  android: AppPlatform.Android,
+  ios: AppPlatform.Ios,
+};


Suggested change

const platformToAppPlatform: Record<string, AppPlatform | undefined> = {

android: AppPlatform.Android,

ios: AppPlatform.Ios,

};

const platformToAppPlatform: Record<

Exclude<RequestedPlatform, RequestedPlatform.All>,

AppPlatform

> = {

[RequestedPlatform.Android]: AppPlatform.Android,

[RequestedPlatform.Ios]: AppPlatform.Ios,

};

szdziedzic · 2024-01-16T14:44:32Z

packages/eas-cli/src/commands/build/cancel.ts

+    platform: Flags.string({
+      char: 'p',
+      description: 'Filter builds by the platform if build ID is not provided',
+      options: ['android', 'ios', 'all'],
+    }),


Suggested change

platform: Flags.string({

char: 'p',

description: 'Filter builds by the platform if build ID is not provided',

options: ['android', 'ios', 'all'],

}),

platform: Flags.enum({

char: 'p',

description: 'Filter builds by the platform if build ID is not provided',

options: Object.values(RequestedPlatform),

}),

szdziedzic · 2024-01-16T14:44:55Z

packages/eas-cli/src/commands/build/cancel.ts

+    if (buildIdFromArg && (platform || profile)) {
+      throw new Error(
+        'Build ID cannot be used together with platform and profile flags. They are used to filter the list of builds when not providing the build ID'
+      );
+    }


szdziedzic · 2024-01-16T14:45:41Z

packages/eas-cli/src/commands/build/delete.ts

+    platform: Flags.string({
+      char: 'p',
+      description: 'Filter builds by the platform if build ID is not provided',
+      options: ['android', 'ios', 'all'],
+    }),


Suggested change

platform: Flags.string({

char: 'p',

description: 'Filter builds by the platform if build ID is not provided',

options: ['android', 'ios', 'all'],

}),

platform: Flags.enum({

char: 'p',

description: 'Filter builds by the platform if build ID is not provided',

options: Object.values(RequestedPlatform),

}),

Replaced strings with object enums See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Removed undefined See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

github-actions · 2024-01-16T15:07:14Z

✅ Thank you for adding the changelog entry!

radoslawkrzemien added 2 commits January 12, 2024 15:26

[eas-cli] Add command to delete build

e9fa4e8

Added a new command build:delete. Shares some code with build:cancel, so I have moved the redundancies to a utils file at src/commandUtils/builds See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Add tests

3163eb6

Added a test file based on existing tests for build:cancel command. Adding more tests See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

radoslawkrzemien marked this pull request as ready for review January 12, 2024 14:41

radoslawkrzemien requested review from szdziedzic and khamilowicz as code owners January 12, 2024 14:41

radoslawkrzemien requested review from brentvatne and sjchmiela and removed request for brentvatne January 12, 2024 14:42

sjchmiela approved these changes Jan 15, 2024

View reviewed changes

szdziedzic reviewed Jan 15, 2024

View reviewed changes

radoslawkrzemien added 5 commits January 15, 2024 13:13

[eas-cli] Sort builds

66bb00f

Explicitly sorts fetched builds by descending order of `created_at`. In case multiple results were combined (for different statuses) they would by already sorted, but grouped by status also See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Use object arg

d1c3ada

Changed `fetchBuildsAsync` arguments into single object argument for readability See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Update msg

967a15a

Changed the message displayed when no builds are found for deletion/cancellation to be more human-like See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Update msg

fd40ac2

Changed the message displayed when no build ID is provided for non-interactive mode, to not include the argument name but rather its description See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Parallelize

13a274d

Changed for loop with async calls into await promise.all() See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

khamilowicz approved these changes Jan 16, 2024

View reviewed changes

radoslawkrzemien requested a review from szdziedzic January 16, 2024 13:00

update CHANGELOG.md

b8f97ab

radoslawkrzemien added 3 commits January 16, 2024 13:08

update CHANGELOG.md

30647af

[eas-cli] Rephrase

2a9820c

Using imperative form in the changelog See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

Merge remote-tracking branch 'origin/main' into @radoslawkrzemien/ENG…

9f54321

…-11026-add-build-delete-command # Conflicts: # CHANGELOG.md

szdziedzic reviewed Jan 16, 2024

View reviewed changes

radoslawkrzemien added 2 commits January 16, 2024 15:12

[eas-cli] Validate args

b9fdaae

Disallow using both the build ID and filter flags when using `build:delete` and `build:cancel` commands See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Update descriptions

96a475b

Updated the descriptions of the filter flags as per suggestion during review See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Add mapping

cb96ae3

Added function for mapping platform flag into generated graphQL value See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

radoslawkrzemien requested a review from szdziedzic January 16, 2024 14:25

sjchmiela reviewed Jan 16, 2024

View reviewed changes

[eas-cli] Update mapping

549b62c

Replaced function with a mapping object See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

szdziedzic approved these changes Jan 16, 2024

View reviewed changes

radoslawkrzemien added 2 commits January 16, 2024 16:05

[eas-cli] Use Flags.enum

334c95c

Replaced strings with object enums See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

[eas-cli] Fix type

f2e1d05

Removed undefined See: https://linear.app/expo/issue/ENG-11026/add-eas-builddelete-command

radoslawkrzemien merged commit ccc8d5a into main Jan 16, 2024
9 checks passed

radoslawkrzemien deleted the @radoslawkrzemien/ENG-11026-add-build-delete-command branch January 16, 2024 15:12

	Log.warn(`There aren't any builds for the project ${projectDisplayName}.`);
	Log.warn(`There are no builds for the project ${projectDisplayName}.`);

	description: 'Platform for which to list builds when ID not provided',
	description: 'Filter builds by the platform if BUILD_ID is not provided',

	'Name of the build profile from eas.json. Defaults to "production" if defined in eas.json.',
	'Filter builds by profile if BUILD_ID is not provided',

[eas-cli] [ENG-11026] Add build:delete command #2178

[eas-cli] [ENG-11026] Add build:delete command #2178

Conversation

radoslawkrzemien commented Jan 12, 2024 • edited Loading

Why

How

Screens

With build id

With prompt

Non-interactive with id

Non-interactive with prompt (not allowed)

New filter flags

No flags

build:delete with platform/p flag

build:delete with profile/e flag

Same for build:cancel command

Using both build ID and filter flags is not allowed

Test Plan

Deployment Plan

linear bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024 • edited Loading

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

sjchmiela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szdziedzic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radoslawkrzemien commented Jan 16, 2024

radoslawkrzemien commented Jan 16, 2024

szdziedzic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radoslawkrzemien Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szdziedzic left a comment

Choose a reason for hiding this comment

szdziedzic Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szdziedzic Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2024

radoslawkrzemien commented Jan 12, 2024 •

edited

Loading

`build:delete` with `platform/p` flag

`build:delete` with `profile/e` flag

Same for `build:cancel` command

github-actions bot commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

radoslawkrzemien Jan 16, 2024 •

edited

Loading

szdziedzic Jan 16, 2024 •

edited

Loading

szdziedzic Jan 16, 2024 •

edited

Loading