[eas-cli] warn for misconfigured channel configuration for eas update #1082

kbrandwijk · 2022-04-24T22:09:55Z

Checklist

I've added an entry to CHANGELOG.md if necessary. You can comment this pull request with /changelog-entry [breaking-change|new-feature|bug-fix|chore] [message] and CHANGELOG.md will be updated automatically.

Why

One of the most common issues with EAS Update is that users still have releaseChannel in their eas.json instead of channel. This PR tries to make the user aware of this in three cases.

How

When running eas update:configure, the CLI will now automatically migrate the releaseChannel property to channel for all build profiles.
When running eas update, the CLI will warn if any build profiles are still using the releaseChannel property.
When running eas build, the CLI will warn only if both the project is configured to use EAS Update and the selected build profile has the releaseChannel property.

In that last case, I'm on the fence if the CLI should show and error and abort instead of just warn for interactive builds.

Test Plan

Tested the various use cases manually. There's not many test cases at the points where the changes were made, I can improve on that in a separate PR.

github-actions · 2022-04-24T22:14:13Z

Size Change: +2.44 kB (0%)

Total Size: 25.2 MB

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

_{compressed-size-action}

codecov · 2022-04-24T22:19:38Z

Codecov Report

Merging #1082 (ff94be6) into main (c8935e1) will decrease coverage by 0.99%.
The diff coverage is 22.81%.

❗ Current head ff94be6 differs from pull request most recent head 995bdf7. Consider uploading reports for the commit 995bdf7 to get more accurate results

@@            Coverage Diff             @@
##             main    #1082      +/-   ##
==========================================
- Coverage   51.13%   50.15%   -0.98%     
==========================================
  Files         391      348      -43     
  Lines       14049    12766    -1283     
  Branches     2933     2451     -482     
==========================================
- Hits         7182     6401     -781     
- Misses       6333     6354      +21     
+ Partials      534       11     -523

Impacted Files	Coverage Δ
packages/eas-cli/src/commands/update/configure.ts	`15.68% <10.00%> (-1.71%)`	⬇️
packages/eas-cli/src/update/utils.ts	`29.51% <29.04%> (-2.19%)`	⬇️
packages/eas-cli/src/build/runBuildAndSubmit.ts	`25.24% <33.34%> (+0.04%)`	⬆️
packages/eas-cli/src/commands/update/index.ts	`13.42% <33.34%> (-13.27%)`	⬇️
...ackages/eas-cli/src/graphql/queries/UpdateQuery.ts	`50.00% <0.00%> (-21.42%)`	⬇️
packages/eas-cli/src/graphql/client.ts	`30.31% <0.00%> (-18.18%)`	⬇️
packages/eas-cli/src/project/workflow.ts	`88.89% <0.00%> (-11.11%)`	⬇️
packages/eas-cli/src/uploads.ts	`20.00% <0.00%> (-10.95%)`	⬇️
packages/eas-cli/src/commands/branch/view.ts	`35.42% <0.00%> (-10.41%)`	⬇️
... and 206 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kbrandwijk · 2022-04-25T09:57:06Z

@dsokal pointed out to me that I need to account for the possible difference between android and ios configuration, both in the update:configure scenario and the build scenario. Will update the PR for this.

dsokal · 2022-04-25T09:40:16Z

packages/eas-cli/src/update/utils.ts

+  if ((await checkEASUpdateURLIsSetAsync(exp)) && buildProfile.profile.releaseChannel) {
+    Log.warn(`» Build profile ${chalk.bold(
+      buildProfile.profileName
+    )} in your eas.json specifies the "releaseChannel" property.


I'd make "releaseChannel" and "channel" bold but I don't have a strong opinion on this.

Our pattern is:

file names should be bold

a value like the build profile should be in quotes

if there is a value that goes into code, we use back ticks

dsokal · 2022-04-25T10:04:51Z

packages/eas-cli/src/build/runBuildAndSubmit.ts

+  if (buildProfiles.length > 0) {
+    await checkBuildProfileConfigMatchesProjectConfigAsync(projectDir, buildProfiles[0]);
+  }


Apply the suggestion shared on Slack.

I'd move it to prepareAndStartBuildAsync. There's an exp object available there so you wouldn't need to call getConfig in checkBuildProfileConfigMatchesProjectConfigAsync.

dsokal · 2022-04-25T10:05:34Z

packages/eas-cli/src/update/utils.ts

+  projectDir: string,
+  buildProfile: ProfileData<'build'>
+): Promise<boolean> {
+  const { exp } = getConfig(projectDir, {


IMO this should be passed as an argument.

dsokal · 2022-04-25T10:07:18Z

packages/eas-cli/src/update/utils.ts

+
+export async function checkEASUpdateURLIsSetAsync(exp: ExpoConfig): Promise<boolean> {
+  const configuredURL = exp.updates?.url;
+  const projectId = await getProjectIdAsync(exp);


I'd also pass this as an argument.

dsokal · 2022-04-25T10:10:16Z

packages/eas-cli/src/update/utils.ts

+  projectDir: string
+): Promise<boolean> {
+  const easJson = await new EasJsonReader(projectDir).readAsync();
+  if (easJson.build && Object.entries(easJson.build).some(([, value]) => value.releaseChannel)) {


IMO the second condition is too complicated to keep it inline.

dsokal · 2022-04-25T10:10:58Z

packages/eas-cli/src/commands/update/configure.ts

 import { Flags } from '@oclif/core';
 import assert from 'assert';
 import chalk from 'chalk';
+import { promises as fs } from 'fs-extra';


Suggested change

import { promises as fs } from 'fs-extra';

import fs from 'fs-extra';

or import promises from fs.

wkozyra95 · 2022-04-25T10:59:02Z

packages/eas-cli/src/commands/update/configure.ts

+
+  if (releaseChannelsFound) {
+    try {
+      await fs.writeFile(


Not a strong opinion, but I would prefer to avoid modifying eas.json automatically:

maybe someone is using both classic and eas updates

we are forcing formating/indentation level

users might have their eas.json structured in a specific way that is helpful for readability that we might break here,

@wkozyra95

You can't use both at the same time, because the releaseChannel and channel fields are mutually exclusive

We modify and write app.json as well

If someone really doesn't like the changes they can decide to rollback the change and not commit it

But most importantly: I think the amount of people missing this migration change in the docs and ending up with updates that don't work outweighs any downsides for the tiny minority who has manually applied some special indentation to the file.

wkozyra95 · 2022-04-25T11:05:44Z

packages/eas-cli/src/update/utils.ts

@@ -74,3 +78,60 @@ export function ensureValidVersions(exp: ExpoConfig, platform: RequestedPlatform
    throw error;
  }
 }
+
+export async function checkDeprecatedChannelConfigurationAsync(


you are mixing here to responsibilities, this function should either:

check if current config is deprecated and return true/false without logging anything. I would also expect in that case that function will be named isUsingDeprcatedChannelCOnfiguration or sth like that

handle the situation(e.g. print warning and return) and do not return any values

wkozyra95 · 2022-04-25T11:07:07Z

packages/eas-cli/src/update/utils.ts

+  return false;
+}
+
+export async function checkBuildProfileConfigMatchesProjectConfigAsync(


The same as above, it should either handle the situation or return boolean(not both)

jonsamp

There are still some comments to address, but overall I think this is the right direction and will help people migrate from classic to EAS Update. Thanks for taking this on!

jonsamp · 2022-04-26T02:57:35Z

packages/eas-cli/src/commands/update/configure.ts

+      if (value.releaseChannel) {
+        releaseChannelsFound = true;
+        easJson.build[key].channel = value.releaseChannel;
+        delete easJson.build[key].releaseChannel;


Can we log that we changed this?

Log.withTick(`Migrated the Classic Updates release channel "${value.releaseChannel}" to be an EAS Update channel.`)

jonsamp · 2022-04-26T02:59:46Z

packages/eas-cli/src/update/utils.ts

+  if ((await checkEASUpdateURLIsSetAsync(exp)) && buildProfile.profile.releaseChannel) {
+    Log.warn(`» Build profile ${chalk.bold(
+      buildProfile.profileName
+    )} in your eas.json specifies the "releaseChannel" property.


Our pattern is:

file names should be bold

a value like the build profile should be in quotes

if there is a value that goes into code, we use back ticks

dsokal · 2023-04-25T09:31:13Z

@kbrandwijk is this PR something we still want to merge? If no can we close it?

kbrandwijk · 2023-04-25T12:08:37Z

@kbrandwijk is this PR something we still want to merge? If no can we close it?

I still like to have this feature, but I don't have the bandwidth currently to address that main comment you left on it.

kbrandwijk added 3 commits April 24, 2022 22:30

feat: migrate releaseChannel on update:configure

0c83275

feat: warn for releaseChannels when running eas update

9ff1f9e

feat: warn during build for eas update + releaseChannel combination

29d7f73

kbrandwijk requested review from dsokal and jonsamp April 24, 2022 22:09

kbrandwijk requested a review from wkozyra95 as a code owner April 24, 2022 22:09

kbrandwijk changed the title ~~@kbrandwijk/releasechannel warning~~ [eas-cli] warn for misconfigured channel configuration for eas update Apr 24, 2022

kbrandwijk removed the request for review from wkozyra95 April 24, 2022 22:10

kbrandwijk added 2 commits April 25, 2022 00:14

style: make linter happier

bbe9426

docs: changelog

ff94be6

dsokal requested a review from wkozyra95 April 25, 2022 09:32

kbrandwijk marked this pull request as draft April 25, 2022 09:57

dsokal reviewed Apr 25, 2022

View reviewed changes

wkozyra95 reviewed Apr 25, 2022

View reviewed changes

jonsamp approved these changes Apr 26, 2022

View reviewed changes

brentvatne mentioned this pull request Aug 8, 2022

[docs] eas update vs eas build with ref. to releaseChannel / channel / branch expo/expo#18489

Closed

Merge branch 'main' into @kbrandwijk/releasechannel-warning

995bdf7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eas-cli] warn for misconfigured channel configuration for eas update #1082

[eas-cli] warn for misconfigured channel configuration for eas update #1082

kbrandwijk commented Apr 24, 2022

github-actions bot commented Apr 24, 2022 •

edited

codecov bot commented Apr 24, 2022 •

edited

kbrandwijk commented Apr 25, 2022

dsokal Apr 25, 2022

jonsamp Apr 26, 2022

dsokal Apr 25, 2022

dsokal Apr 25, 2022

dsokal Apr 25, 2022

dsokal Apr 25, 2022

dsokal Apr 25, 2022

dsokal Apr 25, 2022

wkozyra95 Apr 25, 2022

kbrandwijk Apr 25, 2022

wkozyra95 Apr 25, 2022

wkozyra95 Apr 25, 2022

jonsamp left a comment

jonsamp Apr 26, 2022

jonsamp Apr 26, 2022

dsokal commented Apr 25, 2023

kbrandwijk commented Apr 25, 2023

	import { promises as fs } from 'fs-extra';
	import fs from 'fs-extra';

[eas-cli] warn for misconfigured channel configuration for eas update #1082

Are you sure you want to change the base?

[eas-cli] warn for misconfigured channel configuration for eas update #1082

Conversation

kbrandwijk commented Apr 24, 2022

Checklist

Why

How

Test Plan

github-actions bot commented Apr 24, 2022 • edited

codecov bot commented Apr 24, 2022 • edited

Codecov Report

kbrandwijk commented Apr 25, 2022

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

jonsamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsokal commented Apr 25, 2023

kbrandwijk commented Apr 25, 2023

github-actions bot commented Apr 24, 2022 •

edited

codecov bot commented Apr 24, 2022 •

edited