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

fix(cli): always exit with exit code 1 after error occurs during command #4536

Merged
merged 7 commits into from
May 13, 2024
Merged
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
5 changes: 1 addition & 4 deletions packages/cli/esbuild.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/* eslint no-console: "off" */

import esbuild from 'esbuild';
import { writeFileSync } from 'fs';
import { writeFileSync } from 'node:fs';

const options = {
entryPoints: ['./src/index.ts'],
Expand All @@ -26,10 +26,7 @@ const options = {
'commander',
'dotenv',
'fast-glob',
'fs',
'node-fetch',
'path',
'readline',
'tar',
],
banner: { js: '#!/usr/bin/env node' },
Expand Down
35 changes: 24 additions & 11 deletions packages/cli/src/auth.test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import { ContentType, MedplumClient } from '@medplum/core';
import { MockClient } from '@medplum/mock';
import cp from 'child_process';
import fs from 'fs';
import http from 'http';
import cp from 'node:child_process';
import fs from 'node:fs';
import http from 'node:http';
import { main } from '.';
import { FileSystemStorage } from './storage';
import { createMedplumClient } from './util/client';

jest.mock('child_process');
jest.mock('http');
jest.mock('node:child_process');
jest.mock('node:http');
jest.mock('./util/client');
jest.mock('fs', () => ({
jest.mock('node:fs', () => ({
existsSync: jest.fn(),
mkdirSync: jest.fn(),
readFileSync: jest.fn(),
Expand All @@ -26,6 +26,14 @@ jest.mock('fs', () => ({
describe('CLI auth', () => {
const env = process.env;
let medplum: MedplumClient;
let processError: jest.SpyInstance;

beforeAll(() => {
process.exit = jest.fn<never, any>().mockImplementation(function exit(exitCode: number) {
throw new Error(`Process exited with exit code ${exitCode}`);
}) as unknown as typeof process.exit;
processError = jest.spyOn(process.stderr, 'write').mockImplementation(jest.fn());
});

beforeEach(() => {
jest.resetModules();
Expand All @@ -34,7 +42,6 @@ describe('CLI auth', () => {
medplum = new MockClient();
console.log = jest.fn();
console.error = jest.fn();
process.exit = jest.fn<never, any>();

(createMedplumClient as unknown as jest.Mock).mockImplementation(async () => medplum);
});
Expand Down Expand Up @@ -87,14 +94,20 @@ describe('CLI auth', () => {
});

test('Login unsupported auth type', async () => {
await main(['node', 'index.js', 'login', '--auth-type', 'foo']);
expect(console.error).toHaveBeenCalledWith(expect.stringContaining('Error: Allowed choices are'));
await expect(main(['node', 'index.js', 'login', '--auth-type', 'foo'])).rejects.toThrow(
'Process exited with exit code 1'
);
expect(processError).toHaveBeenCalledWith(
expect.stringContaining(
"error: option '--auth-type <authType>' argument 'foo' is invalid. Allowed choices are basic, client-credentials, authorization-code, jwt-bearer, token-exchange, jwt-assertion"
)
);
});

test('Login basic auth', async () => {
expect(medplum.getActiveLogin()).toBeUndefined();
await main(['node', 'index.js', 'login', '--auth-type', 'basic']);
expect(console.error).not.toHaveBeenCalled();
expect(processError).not.toHaveBeenCalled();
});

test('Login client credentials', async () => {
Expand All @@ -110,7 +123,7 @@ describe('CLI auth', () => {
'--client-secret',
'abc',
]);
expect(console.error).not.toHaveBeenCalled();
expect(processError).not.toHaveBeenCalled();
});

test('Load credentials from disk', async () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/cli/src/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import {
MedplumClient,
normalizeErrorString,
} from '@medplum/core';
import { exec } from 'child_process';
import { createServer } from 'http';
import { platform } from 'os';
import { exec } from 'node:child_process';
import { createServer } from 'node:http';
import { platform } from 'node:os';
import { createMedplumClient } from './util/client';
import { createMedplumCommand } from './util/command';
import { jwtAssertionLogin, jwtBearerLogin, Profile, saveProfile } from './utils';
Expand Down
16 changes: 15 additions & 1 deletion packages/cli/src/aws/describe.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ import { mockClient } from 'aws-sdk-client-mock';
import { main } from '../index';

describe('describe command', () => {
let processError: jest.SpyInstance;

beforeAll(() => {
process.exit = jest.fn<never, any>().mockImplementation(function exit(exitCode: number) {
throw new Error(`Process exited with exit code ${exitCode}`);
}) as unknown as typeof process.exit;
processError = jest.spyOn(process.stderr, 'write').mockImplementation(jest.fn());

const cfMock = mockClient(CloudFormationClient);

cfMock.on(ListStacksCommand).resolves({
Expand Down Expand Up @@ -87,6 +94,10 @@ describe('describe command', () => {
});
});

beforeEach(() => {
jest.clearAllMocks();
});

test('Describe command', async () => {
console.log = jest.fn();
await main(['node', 'index.js', 'aws', 'describe', 'dev']);
Expand All @@ -95,7 +106,10 @@ describe('describe command', () => {

test('Describe not found', async () => {
console.log = jest.fn();
await main(['node', 'index.js', 'aws', 'describe', 'not-found']);
await expect(main(['node', 'index.js', 'aws', 'describe', 'not-found'])).rejects.toThrow(
'Process exited with exit code 1'
);
expect(console.log).toHaveBeenCalledWith('Stack not found: not-found');
expect(processError).toHaveBeenCalledWith(expect.stringContaining('Error: Stack not found: not-found'));
});
});
2 changes: 1 addition & 1 deletion packages/cli/src/aws/describe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export async function describeStacksCommand(tag: string): Promise<void> {
const details = await getStackByTag(tag);
if (!details) {
await printStackNotFound(tag);
return;
throw new Error(`Stack not found: ${tag}`);
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, but we might eventually want to have some kind of CliError with a defined exit code, similar to OperationOutcomeError on the server side

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that makes sense, CommanderError which is thrown on invalid arguments does have fields for exit code and error code, which would be nice. I'll open an issue to investigate what CliError should look like

}
printStackDetails(details);
}
8 changes: 4 additions & 4 deletions packages/cli/src/aws/init.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import { S3Client } from '@aws-sdk/client-s3';
import { GetParameterCommand, PutParameterCommand, SSMClient } from '@aws-sdk/client-ssm';
import { GetCallerIdentityCommand, STSClient } from '@aws-sdk/client-sts';
import { mockClient } from 'aws-sdk-client-mock';
import { randomUUID } from 'crypto';
import { readFileSync, unlinkSync, writeFileSync } from 'fs';
import fetch from 'node-fetch';
import readline from 'readline';
import { randomUUID } from 'node:crypto';
import { readFileSync, unlinkSync, writeFileSync } from 'node:fs';
import readline from 'node:readline';
import { main } from '../index';
import { mockReadline } from './test.utils';

jest.mock('readline');
jest.mock('node:readline');
jest.mock('node-fetch');

describe('init command', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/aws/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import {
import { CloudFrontClient, CreatePublicKeyCommand } from '@aws-sdk/client-cloudfront';
import { GetCallerIdentityCommand, STSClient } from '@aws-sdk/client-sts';
import { MedplumInfraConfig, normalizeErrorString } from '@medplum/core';
import { generateKeyPairSync, randomUUID } from 'crypto';
import { existsSync } from 'fs';
import { generateKeyPairSync, randomUUID } from 'node:crypto';
import { existsSync } from 'node:fs';
import { getConfigFileName, writeConfig } from '../utils';
import { ask, checkOk, choose, chooseInt, closeTerminal, header, initTerminal, print, yesOrNo } from './terminal';
import { getServerVersions, writeParameters } from './utils';
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/aws/terminal.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import readline from 'readline';
import readline from 'node:readline';

let terminal: readline.Interface;

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/aws/test.utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import readline from 'readline';
import readline from 'node:readline';

export function mockReadline(...answers: string[]): readline.Interface {
const result = { write: jest.fn(), question: jest.fn(), close: jest.fn() };
Expand Down
37 changes: 29 additions & 8 deletions packages/cli/src/aws/update-app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,17 @@ import {
import { PutObjectCommand, S3Client, S3ClientResolvedConfig } from '@aws-sdk/client-s3';
import { AwsStub, mockClient } from 'aws-sdk-client-mock';
import fastGlob from 'fast-glob';
import fs from 'fs';
import fetch from 'node-fetch';
import { Readable, Writable } from 'stream';
import fs from 'node:fs';
import { Readable, Writable } from 'node:stream';
import tar from 'tar';
import { main } from '../index';

jest.mock('fast-glob', () => ({
sync: jest.fn(() => []),
}));

jest.mock('fs', () => ({
jest.mock('node:fs', () => ({
createReadStream: jest.fn(),
existsSync: jest.fn(),
mkdtempSync: jest.fn(() => '/tmp/'),
Expand Down Expand Up @@ -54,7 +54,17 @@ let s3Mock: AwsStub<ServiceInputTypes, ServiceOutputTypes, S3ClientResolvedConfi
let cloudFrontMock: AwsStub<ServiceInputTypes, ServiceOutputTypes, CloudFrontClientResolvedConfig>;

describe('update-app command', () => {
let processError: jest.SpyInstance;

beforeAll(() => {
process.exit = jest.fn<never, any>().mockImplementation(function exit(exitCode: number) {
throw new Error(`Process exited with exit code ${exitCode}`);
}) as unknown as typeof process.exit;
processError = jest.spyOn(process.stderr, 'write').mockImplementation(jest.fn());
});

beforeEach(() => {
jest.clearAllMocks();
cfMock = mockClient(CloudFormationClient);

cfMock.on(ListStacksCommand).resolves({
Expand Down Expand Up @@ -343,16 +353,22 @@ describe('update-app command', () => {
(fs.existsSync as jest.Mock).mockReturnValueOnce(false);

console.log = jest.fn();
await main(['node', 'index.js', 'aws', 'update-app', 'not-found']);
await expect(main(['node', 'index.js', 'aws', 'update-app', 'not-found'])).rejects.toThrow(
'Process exited with exit code 1'
);
expect(console.log).toHaveBeenCalledWith('Config not found: not-found (medplum.not-found.config.json)');
expect(processError).toHaveBeenCalledWith('Error: Config not found: not-found\n');
});

test('Update app config custom filename not found', async () => {
(fs.existsSync as jest.Mock).mockReturnValueOnce(false);

console.log = jest.fn();
await main(['node', 'index.js', 'aws', 'update-app', 'not-found', '--file', 'foo.json']);
await expect(main(['node', 'index.js', 'aws', 'update-app', 'not-found', '--file', 'foo.json'])).rejects.toThrow(
'Process exited with exit code 1'
);
expect(console.log).toHaveBeenCalledWith('Config not found: not-found (foo.json)');
expect(processError).toHaveBeenCalledWith('Error: Config not found: not-found\n');
});

test('Update app stack not found', async () => {
Expand All @@ -361,8 +377,11 @@ describe('update-app command', () => {
(fs.readFileSync as jest.Mock).mockReturnValueOnce('{}');

console.log = jest.fn();
await main(['node', 'index.js', 'aws', 'update-app', 'not-found']);
await expect(main(['node', 'index.js', 'aws', 'update-app', 'not-found'])).rejects.toThrow(
'Process exited with exit code 1'
);
expect(console.log).toHaveBeenCalledWith('Stack not found: not-found');
expect(processError).toHaveBeenCalledWith('Error: Stack not found: not-found\n');
});

test('Update app stack incomplete', async () => {
Expand All @@ -371,7 +390,9 @@ describe('update-app command', () => {
(fs.readFileSync as jest.Mock).mockReturnValueOnce('{}');

console.log = jest.fn();
await main(['node', 'index.js', 'aws', 'update-app', 'incomplete']);
expect(console.log).toHaveBeenCalledWith('App bucket not found');
await expect(main(['node', 'index.js', 'aws', 'update-app', 'incomplete'])).rejects.toThrow(
'Process exited with exit code 1'
);
expect(processError).toHaveBeenCalledWith('Error: App bucket not found for stack incomplete\n');
});
});
17 changes: 8 additions & 9 deletions packages/cli/src/aws/update-app.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { PutObjectCommand } from '@aws-sdk/client-s3';
import { ContentType } from '@medplum/core';
import fastGlob from 'fast-glob';
import { createReadStream, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'fs';
import fetch from 'node-fetch';
import { tmpdir } from 'os';
import { join, sep } from 'path';
import { pipeline } from 'stream/promises';
import { createReadStream, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join, sep } from 'node:path';
import { pipeline } from 'node:stream/promises';
import { readConfig, safeTarExtractor } from '../utils';
import { createInvalidation, getStackByTag, printConfigNotFound, printStackNotFound, s3Client } from './utils';

Expand All @@ -23,18 +23,17 @@ export interface UpdateAppOptions {
export async function updateAppCommand(tag: string, options: UpdateAppOptions): Promise<void> {
const config = readConfig(tag, options);
if (!config) {
await printConfigNotFound(tag, options);
return;
printConfigNotFound(tag, options);
throw new Error(`Config not found: ${tag}`);
}
const details = await getStackByTag(tag);
if (!details) {
await printStackNotFound(tag);
return;
throw new Error(`Stack not found: ${tag}`);
}
const appBucket = details.appBucket;
if (!appBucket) {
console.log('App bucket not found');
return;
throw new Error(`App bucket not found for stack ${tag}`);
}

const version = options?.toVersion ?? 'latest';
Expand Down
Loading
Loading