Skip to content

Commit

Permalink
fix(core): improve sync error handling and messaging
Browse files Browse the repository at this point in the history
  • Loading branch information
@leosvelperez
leosvelperez committed Sep 3, 2024
1 parent c5f2083 commit b34ae7e
Show file tree
Hide file tree
Showing 6 changed files with 275 additions and 121 deletions.
44 changes: 33 additions & 11 deletions packages/nx/src/command-line/sync/sync.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import {
collectAllRegisteredSyncGenerators,
flushSyncGeneratorChanges,
getFailedSyncGeneratorsFixMessageLines,
getFlushFailureMessageLines,
getSyncGeneratorChanges,
getSyncGeneratorSuccessResultsMessageLines,
isFailedFlushResult,
processSyncGeneratorResultErrors,
} from '../../utils/sync-generators';
import type { SyncArgs } from './command-object';
Expand Down Expand Up @@ -65,16 +67,16 @@ export function syncHandler(options: SyncOptions): Promise<number> {
if (areAllResultsFailures) {
// if all sync generators failed to run we can't say for sure if the workspace is out of sync
// because they could have failed due to a bug, so we print a warning and exit with code 0
output.warn({
title: `The workspace might be out of sync because ${
output.error({
title: `The workspace is probably out of sync because ${
failedGeneratorsCount === 1
? 'a sync generator'
: 'some sync generators'
} failed to run`,
bodyLines: failedSyncGeneratorsFixMessageLines,
});

return 0;
return 1;
}

const resultBodyLines = getSyncGeneratorSuccessResultsMessageLines(results);
Expand All @@ -85,7 +87,7 @@ export function syncHandler(options: SyncOptions): Promise<number> {
});

if (anySyncGeneratorsFailed) {
output.warn({
output.error({
title:
failedGeneratorsCount === 1
? 'A sync generator failed to run'
Expand All @@ -106,7 +108,17 @@ export function syncHandler(options: SyncOptions): Promise<number> {
spinner.start();

try {
await flushSyncGeneratorChanges(results);
const flushResult = await flushSyncGeneratorChanges(results);

if (isFailedFlushResult(flushResult)) {
spinner.fail();
output.error({
title: 'Failed to sync the workspace',
bodyLines: getFlushFailureMessageLines(flushResult, options.verbose),
});

return 1;
}
} catch (e) {
spinner.fail();
output.error({
Expand All @@ -115,26 +127,36 @@ export function syncHandler(options: SyncOptions): Promise<number> {
'Syncing the workspace failed with the following error:',
'',
e.message,
...(options.verbose ? [`\n${e.stack}`] : []),
...(options.verbose && !!e.stack ? [`\n${e.stack}`] : []),
'',
'Please make sure to run with `--verbose` and report the error at: https://github.com/nrwl/nx/issues/new/choose',
],
});

return 1;
}

spinner.succeed(`The workspace was synced successfully!
Please make sure to commit the changes to your repository.`);
const successTitle = anySyncGeneratorsFailed
? // the identified changes were synced successfully, but the workspace
// is still not up to date, which we'll mention next
'The identified changes were synced successfully!'
: // the workspace is fully up to date
'The workspace was synced successfully!';
const successSubtitle =
'Please make sure to commit the changes to your repository.';
spinner.succeed(`${successTitle}\n\n${successSubtitle}`);

if (anySyncGeneratorsFailed) {
output.warn({
title: `The workspace might still be out of sync because ${
output.error({
title: `The workspace is probably still out of sync because ${
failedGeneratorsCount === 1
? 'a sync generator'
: 'some sync generators'
} failed to run`,
bodyLines: failedSyncGeneratorsFixMessageLines,
});

return 1;
}

return 0;
Expand Down
9 changes: 7 additions & 2 deletions packages/nx/src/daemon/client/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ import {
GET_SYNC_GENERATOR_CHANGES,
type HandleGetSyncGeneratorChangesMessage,
} from '../message-types/get-sync-generator-changes';
import type { SyncGeneratorRunResult } from '../../utils/sync-generators';
import type {
FlushSyncGeneratorChangesResult,
SyncGeneratorRunResult,
} from '../../utils/sync-generators';
import {
GET_REGISTERED_SYNC_GENERATORS,
type HandleGetRegisteredSyncGeneratorsMessage,
Expand Down Expand Up @@ -372,7 +375,9 @@ export class DaemonClient {
return this.sendToDaemonViaQueue(message);
}

flushSyncGeneratorChangesToDisk(generators: string[]): Promise<void> {
flushSyncGeneratorChangesToDisk(
generators: string[]
): Promise<FlushSyncGeneratorChangesResult> {
const message: HandleFlushSyncGeneratorChangesToDiskMessage = {
type: FLUSH_SYNC_GENERATOR_CHANGES_TO_DISK,
generators,
Expand Down
4 changes: 2 additions & 2 deletions packages/nx/src/daemon/server/handle-flush-sync-generator-changes-to-disk.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import { flushSyncGeneratorChangesToDisk } from './sync-generators';
export async function handleFlushSyncGeneratorChangesToDisk(
generators: string[]
): Promise<HandlerResult> {
await flushSyncGeneratorChangesToDisk(generators);
const result = await flushSyncGeneratorChangesToDisk(generators);

return {
response: '{}',
response: JSON.stringify(result),
description: 'handleFlushSyncGeneratorChangesToDisk',
};
}
5 changes: 3 additions & 2 deletions packages/nx/src/daemon/server/sync-generators.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
collectRegisteredGlobalSyncGenerators,
flushSyncGeneratorChanges,
runSyncGenerator,
type FlushSyncGeneratorChangesResult,
type SyncGeneratorRunResult,
type SyncGeneratorRunSuccessResult,
} from '../../utils/sync-generators';
Expand Down Expand Up @@ -71,7 +72,7 @@ export async function getCachedSyncGeneratorChanges(

export async function flushSyncGeneratorChangesToDisk(
generators: string[]
): Promise<void> {
): Promise<FlushSyncGeneratorChangesResult> {
log('flush sync generators changes', generators);

const results = await getCachedSyncGeneratorChanges(generators);
Expand All @@ -80,7 +81,7 @@ export async function flushSyncGeneratorChangesToDisk(
syncGeneratorsCacheResultPromises.delete(generator);
}

await flushSyncGeneratorChanges(results);
return await flushSyncGeneratorChanges(results);
}

export function collectAndScheduleSyncGenerators(
Expand Down
135 changes: 91 additions & 44 deletions packages/nx/src/tasks-runner/run-command.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ import {
collectEnabledTaskSyncGeneratorsFromTaskGraph,
flushSyncGeneratorChanges,
getFailedSyncGeneratorsFixMessageLines,
getFlushFailureMessageLines,
getSyncGeneratorChanges,
getSyncGeneratorSuccessResultsMessageLines,
isFailedFlushResult,
processSyncGeneratorResultErrors,
} from '../utils/sync-generators';
import { workspaceRoot } from '../utils/workspace-root';
Expand Down Expand Up @@ -264,14 +266,16 @@ async function ensureWorkspaceIsInSyncAndGetGraphs(

if (areAllResultsFailures) {
output.warn({
title: `The workspace might be out of sync because ${
title: `The workspace is probably out of sync because ${
failedGeneratorsCount === 1
? 'a sync generator'
: 'some sync generators'
} failed to run`,
bodyLines: failedSyncGeneratorsFixMessageLines,
});

await confirmRunningTasksWithSyncFailures();

// if all sync generators failed to run there's nothing to sync, we just let the tasks run
return { projectGraph, taskGraph };
}
Expand Down Expand Up @@ -313,7 +317,7 @@ async function ensureWorkspaceIsInSyncAndGetGraphs(
title: outOfSyncTitle,
bodyLines: [
...resultBodyLines,
'Your workspace is set to not apply changes automatically (`sync.applyChanges` is set to `false` in your `nx.json`).',
'Your workspace is set to not apply the identified changes automatically (`sync.applyChanges` is set to `false` in your `nx.json`).',
willErrorOnCiMessage,
fixMessage,
],
Expand All @@ -327,6 +331,8 @@ async function ensureWorkspaceIsInSyncAndGetGraphs(
: 'Some sync generators failed to run',
bodyLines: failedSyncGeneratorsFixMessageLines,
});

await confirmRunningTasksWithSyncFailures();
}

return { projectGraph, taskGraph };
Expand All @@ -337,7 +343,7 @@ async function ensureWorkspaceIsInSyncAndGetGraphs(
bodyLines: [
...resultBodyLines,
nxJson.sync?.applyChanges === true
? 'Proceeding to sync the changes automatically (`sync.applyChanges` is set to `true` in your `nx.json`).'
? 'Proceeding to sync the identified changes automatically (`sync.applyChanges` is set to `true` in your `nx.json`).'
: willErrorOnCiMessage,
],
});
Expand All @@ -350,22 +356,24 @@ async function ensureWorkspaceIsInSyncAndGetGraphs(
const spinner = ora('Syncing the workspace...');
spinner.start();

try {
// Flush sync generator changes to disk
await flushSyncGeneratorChanges(results);
} catch (e) {
// Flush sync generator changes to disk
const flushResult = await flushSyncGeneratorChanges(results);

if (isFailedFlushResult(flushResult)) {
spinner.fail();
output.error({
title: 'Failed to sync the workspace',
bodyLines: [
'Syncing the workspace failed with the following error:',
'',
e.message,
...(nxArgs.verbose ? [`\n${e.stack}`] : []),
...getFlushFailureMessageLines(flushResult, nxArgs.verbose),
...('generalFailure' in flushResult
? [
'If needed, you can run the tasks with the `--skip-sync` flag to disable syncing.',
]
: []),
],
});

return { projectGraph, taskGraph };
await confirmRunningTasksWithSyncFailures();
}

// Re-create project graph and task graph
Expand All @@ -379,48 +387,52 @@ async function ensureWorkspaceIsInSyncAndGetGraphs(
extraOptions
);

if (nxJson.sync?.applyChanges === true) {
spinner.succeed(`The workspace was synced successfully!
Please make sure to commit the changes to your repository or this will error on CI.`);
} else {
// The user was prompted and we already logged a message about erroring on CI
// so here we just tell them to commit the changes.
spinner.succeed(`The workspace was synced successfully!
Please make sure to commit the changes to your repository.`);
}
const successTitle = anySyncGeneratorsFailed
? // the identified changes were synced successfully, but the workspace
// is still not up to date, which we'll mention next
'The identified changes were synced successfully!'
: // the workspace is fully up to date
'The workspace was synced successfully!';
const successSubtitle =
nxJson.sync?.applyChanges === true
? 'Please make sure to commit the changes to your repository or this will error on CI.'
: // The user was prompted and we already logged a message about erroring on CI
// so here we just tell them to commit the changes.
'Please make sure to commit the changes to your repository.';
spinner.succeed(`${successTitle}\n\n${successSubtitle}`);

if (anySyncGeneratorsFailed) {
output.warn({
title: `The workspace might still be out of sync because ${
title: `The workspace is probably still out of sync because ${
failedGeneratorsCount === 1
? 'a sync generator'
: 'some sync generators'
} failed to run`,
bodyLines: failedSyncGeneratorsFixMessageLines,
});

await confirmRunningTasksWithSyncFailures();
}
} else {
output.warn({
title: 'Syncing the workspace was skipped',
bodyLines: [
'This could lead to unexpected results or errors when running tasks.',
fixMessage,
...(anySyncGeneratorsFailed
? [
'',
`Additionally, ${
failedGeneratorsCount === 1
? 'a sync generator'
: 'some sync generators'
} failed to run:`,
'',
...failedSyncGeneratorsFixMessageLines,
]
: []),
],
});
if (anySyncGeneratorsFailed) {
output.warn({
title:
failedGeneratorsCount === 1
? 'A sync generator failed to report the sync status'
: 'Some sync generators failed to report the sync status',
bodyLines: failedSyncGeneratorsFixMessageLines,
});

await confirmRunningTasksWithSyncFailures();
} else {
output.warn({
title: 'Syncing the workspace was skipped',
bodyLines: [
'This could lead to unexpected results or errors when running tasks.',
fixMessage,
],
});
}
}

return { projectGraph, taskGraph };
Expand All @@ -432,7 +444,7 @@ async function promptForApplyingSyncGeneratorChanges(): Promise<boolean> {
name: 'applyChanges',
type: 'select',
message:
'Would you like to sync the changes to get your worskpace up to date?',
'Would you like to sync the identified changes to get your worskpace up to date?',
choices: [
{
name: 'yes',
Expand All @@ -457,6 +469,41 @@ async function promptForApplyingSyncGeneratorChanges(): Promise<boolean> {
}
}

async function confirmRunningTasksWithSyncFailures(): Promise<void> {
try {
const promptConfig = {
name: 'runTasks',
type: 'select',
message:
'Would you like to ignore the sync failures and continue running the tasks?',
choices: [
{
name: 'yes',
message: 'Yes, ignore the failures and run the tasks',
},
{
name: 'no',
message: `No, don't run the tasks`,
},
],
footer: () =>
chalk.dim(
`\nWhen running on CI and there are sync failures, the tasks won't run. Addressing the errors above is highly recommended to prevent failures in CI.`
),
};

const runTasks = await prompt<{ runTasks: 'yes' | 'no' }>([
promptConfig,
]).then(({ runTasks }) => runTasks === 'yes');

if (!runTasks) {
process.exit(1);
}
} catch {
process.exit(1);
}
}

function setEnvVarsBasedOnArgs(nxArgs: NxArgs, loadDotEnvFiles: boolean) {
if (
nxArgs.outputStyle == 'stream' ||
Expand Down
Loading

0 comments on commit b34ae7e

Please sign in to comment.