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

feat(npm): Allow to configure checkPackageName for npm target #504

Merged
merged 4 commits into from
Nov 17, 2023
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
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -567,9 +567,10 @@ The `npm` utility must be installed on the system.
**Configuration**
| Option | Description |
| -------- | -------------------------------------------------------------------------------- |
| `access` | **optional**. Visibility for scoped packages: `restricted` (default) or `public` |
| Option | Description |
| ------------------ | ------------------------------------------------------------------------------------------------------------------------------- |
| `access` | **optional**. Visibility for scoped packages: `restricted` (default) or `public` |
| `checkPackageName` | **optional**. If defined, check this package on the registry to get the current latest version to compare for the `latest` tag. The package(s) to be published will only be tagged with `latest` if the new version is greater than the checked package's version|
**Example**
Expand Down
173 changes: 173 additions & 0 deletions src/targets/__tests__/npm.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
import { getPublishTag, getLatestVersion } from '../npm';
import * as system from '../../utils/system';

const defaultNpmConfig = {
useYarn: false,
token: 'xxx',
};

describe('getLatestVersion', () => {
let spawnProcessMock: jest.SpyInstance;

beforeEach(() => {
spawnProcessMock = jest
.spyOn(system, 'spawnProcess')
.mockImplementation(() => Promise.reject('does not exist'));
});

afterEach(() => {
spawnProcessMock.mockReset();
});

it('returns undefined if package name does not exist', async () => {
const actual = await getLatestVersion(
'sentry-xx-this-does-not-exist',
defaultNpmConfig
);
expect(actual).toEqual(undefined);
expect(spawnProcessMock).toBeCalledTimes(1);
expect(spawnProcessMock).toBeCalledWith(
'npm',
['info', 'sentry-xx-this-does-not-exist', 'version'],
expect.objectContaining({})
);
});

it('returns version for valid package name', async () => {
spawnProcessMock = jest
.spyOn(system, 'spawnProcess')
.mockImplementation(() =>
Promise.resolve(Buffer.from('7.20.0\n', 'utf-8'))
);
const actual = await getLatestVersion('@sentry/browser', defaultNpmConfig);
expect(actual).toBe('7.20.0');
expect(spawnProcessMock).toBeCalledTimes(1);
expect(spawnProcessMock).toBeCalledWith(
'npm',
['info', '@sentry/browser', 'version'],
expect.objectContaining({})
);
});
});

describe('getPublishTag', () => {
let spawnProcessMock: jest.SpyInstance;

beforeEach(() => {
spawnProcessMock = jest
.spyOn(system, 'spawnProcess')
.mockImplementation(() => Promise.reject('does not exist'));
});

afterEach(() => {
spawnProcessMock.mockReset();
});

it('returns undefined without a checkPackageName', async () => {
const logger = {
warn: jest.fn(),
} as any;
const actual = await getPublishTag(
Copy link
Member

Choose a reason for hiding this comment

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

hmmm is this hitting actual public npm during test? we probably want to avoid that if possible since it makes these tests dependent on external state

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this hits npm. I tried to write the test in a way that it works with whatever version we get back. But yes, if NPM would be down this would fail. Any idea how else to test this? 🤔 if we don't want to hit NPM the test also gets less realistic. The only thing I can think of is to mock spawnProcess and have it return a static version?

Copy link
Member

Choose a reason for hiding this comment

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

I would record some example invocations of spawnProcess and patch them in there

Copy link
Member Author

Choose a reason for hiding this comment

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

@asottile-sentry I've updated the test to use a mocked process there instead!

'1.0.0',
undefined,
defaultNpmConfig,
logger
);
expect(actual).toEqual(undefined);
expect(logger.warn).not.toHaveBeenCalled();
expect(spawnProcessMock).not.toBeCalled();
});

it('returns undefined for unexisting package name', async () => {
const logger = {
warn: jest.fn(),
} as any;
const actual = await getPublishTag(
'1.0.0',
'sentry-xx-does-not-exist',
defaultNpmConfig,
logger
);
expect(actual).toEqual(undefined);
expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.warn).toHaveBeenCalledWith(
'Could not fetch current version for package sentry-xx-does-not-exist'
);
expect(spawnProcessMock).toBeCalledTimes(1);
});

it('returns undefined for invalid package version', async () => {
spawnProcessMock = jest
.spyOn(system, 'spawnProcess')
.mockImplementation(() =>
Promise.resolve(Buffer.from('weird-version', 'utf-8'))
);

const logger = {
warn: jest.fn(),
} as any;
const actual = await getPublishTag(
'1.0.0',
'@sentry/browser',
defaultNpmConfig,
logger
);
expect(actual).toEqual(undefined);
expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.warn).toHaveBeenCalledWith(
'Could not fetch current version for package @sentry/browser'
);
expect(spawnProcessMock).toBeCalledTimes(1);
});

it('returns next for prereleases', async () => {
const logger = {
warn: jest.fn(),
} as any;
const actual = await getPublishTag(
'1.0.0-alpha.1',
undefined,
defaultNpmConfig,
logger
);
expect(actual).toBe('next');
expect(logger.warn).toHaveBeenCalledTimes(2);
expect(logger.warn).toHaveBeenCalledWith(
'Detected pre-release version for npm package!'
);
expect(logger.warn).toHaveBeenCalledWith(
'Adding tag "next" to not make it "latest" in registry.'
);
expect(spawnProcessMock).not.toBeCalled();
});

it('returns old for older versions', async () => {
spawnProcessMock = jest
.spyOn(system, 'spawnProcess')
.mockImplementation(() =>
Promise.resolve(Buffer.from('7.20.0\n', 'utf-8'))
);

const logger = {
warn: jest.fn(),
} as any;

const actual = await getPublishTag(
'1.0.0',
'@sentry/browser',
defaultNpmConfig,
logger
);
expect(actual).toBe('old');
expect(logger.warn).toHaveBeenCalledTimes(2);
expect(logger.warn).toHaveBeenCalledWith(
expect.stringMatching(
/Detected older version than currently published version \(([\d.]+)\) for @sentry\/browser/
)
);
expect(logger.warn).toHaveBeenCalledWith(
'Adding tag "old" to not make it "latest" in registry.'
);
expect(spawnProcessMock).toBeCalledTimes(1);
});
});
130 changes: 120 additions & 10 deletions src/targets/npm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ import { TargetConfig } from '../schemas/project_config';
import { ConfigurationError, reportError } from '../utils/errors';
import { isDryRun } from '../utils/helpers';
import { hasExecutable, spawnProcess } from '../utils/system';
import { isPreviewRelease, parseVersion } from '../utils/version';
import {
isPreviewRelease,
parseVersion,
versionGreaterOrEqualThan,
} from '../utils/version';
import { BaseTarget } from './base';
import {
BaseArtifactProvider,
Expand Down Expand Up @@ -36,6 +40,12 @@ export enum NpmPackageAccess {
RESTRICTED = 'restricted',
}

export interface NpmTargetConfig extends TargetConfig {
access?: NpmPackageAccess;
/** If defined, lookup this package name on the registry to get the current latest version. */
checkPackageName?: string;
}

/** NPM target configuration options */
export interface NpmTargetOptions {
/** Package access specifier */
Expand All @@ -54,6 +64,8 @@ interface NpmPublishOptions {
otp?: string;
/** New version to publish */
version: string;
/** A tag to use for the publish. If not set, defaults to "latest" */
tag?: string;
}

/**
Expand All @@ -66,7 +78,7 @@ export class NpmTarget extends BaseTarget {
public readonly npmConfig: NpmTargetOptions;

public constructor(
config: TargetConfig,
config: NpmTargetConfig,
artifactProvider: BaseArtifactProvider
) {
super(config, artifactProvider);
Expand Down Expand Up @@ -178,14 +190,8 @@ export class NpmTarget extends BaseTarget {
args.push(`--access=${this.npmConfig.access}`);
}

// In case we have a prerelease, there should never be a reason to publish
// it with the latest tag in npm.
if (isPreviewRelease(options.version)) {
this.logger.warn('Detected pre-release version for npm package!');
this.logger.warn(
'Adding tag "next" to not make it "latest" in registry.'
);
args.push('--tag=next');
if (options.tag) {
args.push(`--tag=${options.tag}`);
}

return withTempFile(filePath => {
Expand Down Expand Up @@ -235,6 +241,17 @@ export class NpmTarget extends BaseTarget {
publishOptions.otp = await this.requestOtp();
}

const tag = await getPublishTag(
version,
this.config.checkPackageName,
this.npmConfig,
this.logger,
publishOptions.otp
);
if (tag) {
publishOptions.tag = tag;
}

await Promise.all(
packageFiles.map(async (file: RemoteArtifact) => {
const path = await this.artifactProvider.downloadArtifact(file);
Expand All @@ -246,3 +263,96 @@ export class NpmTarget extends BaseTarget {
this.logger.info('NPM release complete');
}
}

/**
* Get the latest version for the given package.
*/
export async function getLatestVersion(
packageName: string,
npmConfig: NpmTargetOptions,
otp?: NpmPublishOptions['otp']
): Promise<string | undefined> {
const args = ['info', packageName, 'version'];
const bin = NPM_BIN;

try {
const response = await withTempFile(filePath => {
// Pass OTP if configured
const spawnOptions: SpawnOptions = {};
spawnOptions.env = { ...process.env };
if (otp) {
spawnOptions.env.NPM_CONFIG_OTP = otp;
}
spawnOptions.env[NPM_TOKEN_ENV_VAR] = npmConfig.token;
// NOTE(byk): Use npm_config_userconfig instead of --userconfig for yarn compat
spawnOptions.env.npm_config_userconfig = filePath;
writeFileSync(
filePath,
`//registry.npmjs.org/:_authToken=\${${NPM_TOKEN_ENV_VAR}}`
);

return spawnProcess(bin, args, spawnOptions);
});

if (!response) {
return undefined;
}

return response.toString().trim();
} catch {
return undefined;
}
}
/**
* Get the tag to use for publishing to npm.
* If this returns `undefined`, we'll use the default behavior from NPM
* (which is to set the `latest` tag).
*/
export async function getPublishTag(
version: string,
checkPackageName: string | undefined,
npmConfig: NpmTargetOptions,
logger: NpmTarget['logger'],
otp?: NpmPublishOptions['otp']
): Promise<string | undefined> {
if (isPreviewRelease(version)) {
logger.warn('Detected pre-release version for npm package!');
logger.warn('Adding tag "next" to not make it "latest" in registry.');
return 'next';
}

// If no checkPackageName is given, we return undefined
if (!checkPackageName) {
return undefined;
}

const latestVersion = await getLatestVersion(
checkPackageName,
npmConfig,
otp
);
const parsedLatestVersion = latestVersion && parseVersion(latestVersion);
const parsedNewVersion = parseVersion(version);

if (!parsedLatestVersion) {
logger.warn(
`Could not fetch current version for package ${checkPackageName}`
);
return undefined;
}

// If we are publishing a version that is older than the currently latest version,
// We tag it with "old" instead of "latest"
if (
parsedNewVersion &&
!versionGreaterOrEqualThan(parsedNewVersion, parsedLatestVersion)
) {
logger.warn(
`Detected older version than currently published version (${latestVersion}) for ${checkPackageName}`
);
logger.warn('Adding tag "old" to not make it "latest" in registry.');
return 'old';
Comment on lines +353 to +354
Copy link
Member

Choose a reason for hiding this comment

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

theoretically (not saying this should happen now), we could assign a <major>.x tag here if we decide we want to go this route for multi-major version support.

}

return undefined;
}
Loading