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

Added option to enable corepack #901

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Prev Previous commit
Next Next commit
fix: pr review adjustments
  • Loading branch information
jrparish committed Feb 6, 2024
commit c73bf9098a639e60a138e5caf34368347ba22106
33 changes: 9 additions & 24 deletions __tests__/main.test.ts
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@ import each from 'jest-each';

import * as main from '../src/main';
import * as util from '../src/util';
import * as cacheUtil from '../src/cache-utils';
import OfficialBuilds from '../src/distributions/official_builds/official_builds';

describe('main tests', () => {
@@ -25,6 +26,7 @@ describe('main tests', () => {
let endGroupSpy: jest.SpyInstance;

let getExecOutputSpy: jest.SpyInstance;
let getCommandOutputSpy: jest.SpyInstance;

let getNodeVersionFromFileSpy: jest.SpyInstance;
let cnSpy: jest.SpyInstance;
@@ -56,6 +58,7 @@ describe('main tests', () => {
inSpy.mockImplementation(name => inputs[name]);

getExecOutputSpy = jest.spyOn(exec, 'getExecOutput');
getCommandOutputSpy = jest.spyOn(cacheUtil, 'getCommandOutput');

findSpy = jest.spyOn(tc, 'find');

@@ -274,50 +277,32 @@ describe('main tests', () => {
it('should not enable corepack when no input', async () => {
inputs['corepack'] = '';
await main.run();
expect(getExecOutputSpy).not.toHaveBeenCalledWith(
'corepack',
expect.anything(),
expect.anything()
);
expect(getCommandOutputSpy).not.toHaveBeenCalledWith('corepack');
});

it('should not enable corepack when input is "false"', async () => {
inputs['corepack'] = 'false';
await main.run();
expect(getExecOutputSpy).not.toHaveBeenCalledWith(
'corepack',
expect.anything(),
expect.anything()
);
expect(getCommandOutputSpy).not.toHaveBeenCalledWith('corepack');
});

it('should enable corepack when input is "true"', async () => {
inputs['corepack'] = 'true';
await main.run();
expect(getExecOutputSpy).toHaveBeenCalledWith(
'corepack',
['enable'],
expect.anything()
);
expect(getCommandOutputSpy).toHaveBeenCalledWith('corepack enable');
});

it('should enable corepack with a single package manager', async () => {
inputs['corepack'] = 'npm';
await main.run();
expect(getExecOutputSpy).toHaveBeenCalledWith(
'corepack',
['enable', 'npm'],
expect.anything()
);
expect(getCommandOutputSpy).toHaveBeenCalledWith('corepack enable npm');
});

it('should enable corepack with multiple package managers', async () => {
inputs['corepack'] = 'npm yarn';
await main.run();
expect(getExecOutputSpy).toHaveBeenCalledWith(
'corepack',
['enable', 'npm', 'yarn'],
expect.anything()
expect(getCommandOutputSpy).toHaveBeenCalledWith(
'corepack enable npm yarn'
);
});
});
9 changes: 4 additions & 5 deletions dist/cache-save/index.js
Original file line number Diff line number Diff line change
@@ -83338,6 +83338,7 @@ const core = __importStar(__nccwpck_require__(2186));
const exec = __importStar(__nccwpck_require__(1514));
const fs_1 = __importDefault(__nccwpck_require__(7147));
const path_1 = __importDefault(__nccwpck_require__(1017));
const cache_utils_1 = __nccwpck_require__(1678);
function getNodeVersionFromFile(versionFilePath) {
var _a, _b, _c, _d, _e;
if (!fs_1.default.existsSync(versionFilePath)) {
@@ -83431,15 +83432,13 @@ const unique = () => {
exports.unique = unique;
function enableCorepack(input) {
return __awaiter(this, void 0, void 0, function* () {
const corepackArgs = ['enable'];
if (input.length > 0 && input !== 'false') {
if (input.length && input !== 'false') {
const corepackArgs = ['enable'];
if (input !== 'true') {
const packageManagers = input.split(' ');
corepackArgs.push(...packageManagers);
}
yield exec.getExecOutput('corepack', corepackArgs, {
ignoreReturnCode: true
});
yield (0, cache_utils_1.getCommandOutput)(`corepack ${corepackArgs.join(' ')}`);
}
});
}
9 changes: 4 additions & 5 deletions dist/setup/index.js
Original file line number Diff line number Diff line change
@@ -93787,6 +93787,7 @@ const core = __importStar(__nccwpck_require__(2186));
const exec = __importStar(__nccwpck_require__(1514));
const fs_1 = __importDefault(__nccwpck_require__(7147));
const path_1 = __importDefault(__nccwpck_require__(1017));
const cache_utils_1 = __nccwpck_require__(1678);
function getNodeVersionFromFile(versionFilePath) {
var _a, _b, _c, _d, _e;
if (!fs_1.default.existsSync(versionFilePath)) {
@@ -93880,15 +93881,13 @@ const unique = () => {
exports.unique = unique;
function enableCorepack(input) {
return __awaiter(this, void 0, void 0, function* () {
const corepackArgs = ['enable'];
if (input.length > 0 && input !== 'false') {
if (input.length && input !== 'false') {
const corepackArgs = ['enable'];
if (input !== 'true') {
const packageManagers = input.split(' ');
corepackArgs.push(...packageManagers);
}
yield exec.getExecOutput('corepack', corepackArgs, {
ignoreReturnCode: true
});
yield (0, cache_utils_1.getCommandOutput)(`corepack ${corepackArgs.join(' ')}`);
}
});
}
9 changes: 4 additions & 5 deletions src/util.ts
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ import * as exec from '@actions/exec';

import fs from 'fs';
import path from 'path';
import {getCommandOutput} from './cache-utils';

export function getNodeVersionFromFile(versionFilePath: string): string | null {
if (!fs.existsSync(versionFilePath)) {
@@ -107,14 +108,12 @@ export const unique = () => {
};

export async function enableCorepack(input: string): Promise<void> {
const corepackArgs = ['enable'];
if (input.length > 0 && input !== 'false') {
if (input.length && input !== 'false') {
const corepackArgs = ['enable'];
if (input !== 'true') {
const packageManagers = input.split(' ');
Copy link

@ranisalt ranisalt Dec 19, 2023

Choose a reason for hiding this comment

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

Maybe also trim the input and split by \s+ to avoid issues due to leading, trailing and double-spacing, and also supporting multiline

corepackArgs.push(...packageManagers);
}
await exec.getExecOutput('corepack', corepackArgs, {
ignoreReturnCode: true
});
await getCommandOutput(`corepack ${corepackArgs.join(' ')}`);
Copy link

@mcmire mcmire Apr 26, 2024

Choose a reason for hiding this comment

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

Looking back at previous PRs, it was suggested that if Yarn ends up being enabled, any existing Yarn installation (which is evidently present on GitHub runners) should be uninstalled first: #482 (review). Is this is a concern?

Copy link

@shinebayar-g shinebayar-g Apr 26, 2024

Choose a reason for hiding this comment

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

thought corepack would interop with existing yarn installations (could be wrong)

Copy link

Choose a reason for hiding this comment

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

Yes, the existing Yarn installation can be left as is and it won't cause conflict with the installed Yarn from Corepack.

}
}