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

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ This is the log of notable changes to EAS CLI and related packages.

### 🎉 New features

- Warn for misconfigured channel configuration when using EAS Update. ([#1082](https://github.com/expo/eas-cli/pull/1082)) by [@kbrandwijk](https://github.com/kbrandwijk))

### 🐛 Bug fixes

- Rebind `console.info` correctly after `ora` instance stops. ([#1113](https://github.com/expo/eas-cli/pull/1113) by [@EvanBacon](https://github.com/EvanBacon))
Expand Down
7 changes: 7 additions & 0 deletions packages/eas-cli/src/build/runBuildAndSubmit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
waitToCompleteAsync as waitForSubmissionsToCompleteAsync,
} from '../submit/submit';
import { printSubmissionDetailsUrls } from '../submit/utils/urls';
import { checkBuildProfileConfigMatchesProjectConfigAsync } from '../update/utils';
import { printJsonOnlyOutput } from '../utils/json';
import { ProfileData, getProfilesAsync } from '../utils/profiles';
import { getVcsClient } from '../vcs';
Expand Down Expand Up @@ -121,6 +122,12 @@ export async function runBuildAndSubmitAsync(projectDir: string, flags: BuildFla
}[] = [];
const buildCtxByPlatform: { [p in AppPlatform]?: BuildContext<Platform> } = {};

// Check only first buildprofile (there should be only one unique one)
// Warn only once
if (buildProfiles.length > 0) {
await checkBuildProfileConfigMatchesProjectConfigAsync(projectDir, buildProfiles[0]);
}
Comment on lines +127 to +129
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 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.


for (const buildProfile of buildProfiles) {
const { build: maybeBuild, buildCtx } = await prepareAndStartBuildAsync({
projectDir,
Expand Down
37 changes: 37 additions & 0 deletions packages/eas-cli/src/commands/update/configure.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { ExpoConfig, modifyConfigAsync } from '@expo/config';
import { Platform, Workflow } from '@expo/eas-build-job';
import { EasJsonReader } from '@expo/eas-json';
import { Flags } from '@oclif/core';
import assert from 'assert';
import chalk from 'chalk';
import { promises as fs } from 'fs-extra';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

or import promises from fs.


import { getEASUpdateURL } from '../../api';
import EasCommand from '../../commandUtils/EasCommand';
Expand Down Expand Up @@ -64,6 +66,11 @@ export default class UpdateConfigure extends EasCommand {
});
Log.withTick(`Configured ${chalk.bold('app.json')} for EAS Update`);

const madeChanges = await checkAndConfigureEasJsonReleaseChannelsAsync(projectDir);
if (madeChanges) {
Log.withTick(`Configured ${chalk.bold('eas.json')} for EAS Update`);
}

// configure native files for EAS Update
if (
[RequestedPlatform.Android, RequestedPlatform.All].includes(platform) &&
Expand Down Expand Up @@ -241,6 +248,36 @@ async function configureAppJSONForEASUpdateAsync({
return result.config.expo;
}

async function checkAndConfigureEasJsonReleaseChannelsAsync(projectDir: string): Promise<boolean> {
const reader = new EasJsonReader(projectDir);
const easJson = await reader.readAsync();

let releaseChannelsFound = false;

if (easJson.build) {
for (const [key, value] of Object.entries(easJson.build)) {
if (value.releaseChannel) {
releaseChannelsFound = true;
easJson.build[key].channel = value.releaseChannel;
delete easJson.build[key].releaseChannel;
Copy link
Member

Choose a reason for hiding this comment

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

Can we log that we changed this?

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

}
}
}

if (releaseChannelsFound) {
try {
await fs.writeFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

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,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

EasJsonReader.formatEasJsonPath(projectDir),
JSON.stringify(easJson, null, 2)
);
} catch {
throw new Error('Error occured updating eas.json file');
}
}

return releaseChannelsFound;
}

function isRuntimeEqual(
runtimeVersionA: string | { policy: 'sdkVersion' | 'nativeVersion' | 'appVersion' },
runtimeVersionB: string | { policy: 'sdkVersion' | 'nativeVersion' | 'appVersion' }
Expand Down
17 changes: 15 additions & 2 deletions packages/eas-cli/src/commands/update/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ import {
} from '../../project/publish';
import { resolveWorkflowAsync } from '../../project/workflow';
import { confirmAsync, promptAsync, selectAsync } from '../../prompts';
import { formatUpdate } from '../../update/utils';
import {
checkDeprecatedChannelConfigurationAsync,
ensureEASUpdateURLIsSetAsync,
formatUpdate,
} from '../../update/utils';
import { ensureLoggedInAsync } from '../../user/actions';
import {
checkManifestBodyAgainstUpdateInfoGroup,
Expand Down Expand Up @@ -275,7 +279,7 @@ export default class UpdatePublish extends EasCommand {

const runtimeVersions = await getRuntimeVersionObjectAsync(exp, platformFlag, projectDir);
const projectId = await getProjectIdAsync(exp);
await checkEASUpdateURLIsSetAsync(exp);
await ensureEASUpdateURLIsSetAsync(exp);

if (!branchName && autoFlag) {
branchName =
Expand Down Expand Up @@ -324,6 +328,13 @@ export default class UpdatePublish extends EasCommand {
assert(branchName, 'Branch name must be specified.');
}

const { id: branchId, updates } = await ensureBranchExistsAsync({
appId: projectId,
name: branchName,
});

await checkDeprecatedChannelConfigurationAsync(projectDir);

let unsortedUpdateInfoGroups: UpdateInfoGroup = {};
let oldMessage: string, oldRuntimeVersion: string;
let uploadedAssetCount = 0;
Expand Down Expand Up @@ -749,6 +760,8 @@ function formatUpdateTitle(
'mmm dd HH:MM'
)} by ${actorName}, runtimeVersion: ${runtimeVersion}] ${message}`;
}
<<<<<<< HEAD
=======

async function checkEASUpdateURLIsSetAsync(exp: ExpoConfig): Promise<void> {
const configuredURL = exp.updates?.url;
Expand Down
65 changes: 63 additions & 2 deletions packages/eas-cli/src/update/utils.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { ExpoConfig } from '@expo/config';
import { ExpoConfig, getConfig } from '@expo/config';
import { EasJsonReader } from '@expo/eas-json';
import { format } from '@expo/timeago.js';
import chalk from 'chalk';

import { getEASUpdateURL } from '../api';
import { Maybe, Robot, Update, User } from '../graphql/generated';
import { learnMore } from '../log';
import Log, { learnMore } from '../log';
import { RequestedPlatform } from '../platform';
import { getProjectIdAsync } from '../project/projectUtils';
import { getActorDisplayName } from '../user/User';
import groupBy from '../utils/expodash/groupBy';
import { ProfileData } from '../utils/profiles';

export type FormatUpdateParameter = Pick<Update, 'id' | 'createdAt' | 'message'> & {
actor?: Maybe<Pick<User, 'username' | 'id'> | Pick<Robot, 'firstName' | 'id'>>;
Expand Down Expand Up @@ -105,3 +109,60 @@ export function ensureValidVersions(exp: ExpoConfig, platform: RequestedPlatform
throw error;
}
}

export async function checkDeprecatedChannelConfigurationAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Log.warn(`» One or more build profiles in your eas.json specify the "releaseChannel" property.
For EAS Update, you need to specify the "channel" property, or your build will not be able to receive any updates.
Update your eas.json manually, or run ${chalk.bold('eas update:configure')}.
${learnMore('https://docs.expo.dev/eas-update/getting-started/#configure-your-project')}`);
Log.newLine();
return true;
}

return false;
}

export async function checkBuildProfileConfigMatchesProjectConfigAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

IMO this should be passed as an argument.

skipSDKVersionRequirement: true,
isPublicConfig: true,
});
if ((await checkEASUpdateURLIsSetAsync(exp)) && buildProfile.profile.releaseChannel) {
Log.warn(`» Build profile ${chalk.bold(
buildProfile.profileName
)} in your eas.json specifies the "releaseChannel" property.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

For EAS Update, you need to specify the "channel" property, or your build will not be able to receive any updates.
Update your eas.json manually, or run ${chalk.bold('eas update:configure')}.
${learnMore('https://docs.expo.dev/eas-update/getting-started/#configure-your-project')}`);
return true;
}

return false;
}

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

Choose a reason for hiding this comment

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

I'd also pass this as an argument.

const expectedURL = getEASUpdateURL(projectId);

return configuredURL === expectedURL;
}

export async function ensureEASUpdateURLIsSetAsync(exp: ExpoConfig): Promise<void> {
const configuredURL = exp.updates?.url;
const projectId = await getProjectIdAsync(exp);
const expectedURL = getEASUpdateURL(projectId);

if (configuredURL !== expectedURL) {
throw new Error(
`The update URL is incorrectly configured for EAS Update. Please set updates.url to ${expectedURL} in your app.json.`
);
}
}