From 48f49bacb43f9acc9204be178754743c3f6238df Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 17 May 2021 19:00:50 +0200 Subject: [PATCH] refactor(dev-infra): improve type safety of NPM dist tags in release tool (#42133) Instead of passing `string` in the release tool for NPM dist tags, we should use a union string type that limits the tags to `latest`, `next` and anything matching `v{number}-lts`. This avoids mistakes at compilation-level if an invalid/unknown tag would be set by a release action. PR Close #42133 --- dev-infra/ng-dev.js | 8 ++++++-- dev-infra/release/publish/actions.ts | 7 ++++--- .../release/publish/actions/cut-lts-patch.ts | 13 +------------ dev-infra/release/publish/external-commands.ts | 3 ++- dev-infra/release/publish/test/common.spec.ts | 6 +++--- dev-infra/release/publish/test/test-utils.ts | 8 ++++---- .../release/versioning/long-term-support.ts | 16 ++++++++++++---- dev-infra/release/versioning/npm-publish.ts | 3 ++- dev-infra/release/versioning/npm-registry.ts | 4 ++++ 9 files changed, 38 insertions(+), 30 deletions(-) diff --git a/dev-infra/ng-dev.js b/dev-infra/ng-dev.js index b486b57518..6d799e1296 100755 --- a/dev-infra/ng-dev.js +++ b/dev-infra/ng-dev.js @@ -1120,7 +1120,7 @@ function fetchLongTermSupportBranchesFromNpm(config) { // their corresponding branches. We assume that an LTS tagged version in NPM belongs to the // last-minor branch of a given major (i.e. we assume there are no outdated LTS NPM dist tags). for (const npmDistTag in distTags) { - if (ltsNpmDistTagRegex.test(npmDistTag)) { + if (isLtsDistTag(npmDistTag)) { const version = semver.parse(distTags[npmDistTag]); const branchName = `${version.major}.${version.minor}.x`; const majorReleaseDate = new Date(time[`${version.major}.0.0`]); @@ -1142,6 +1142,10 @@ function fetchLongTermSupportBranchesFromNpm(config) { return { active, inactive }; }); } +/** Gets whether the specified tag corresponds to a LTS dist tag. */ +function isLtsDistTag(tagName) { + return ltsNpmDistTagRegex.test(tagName); +} /** * Computes the date when long-term support ends for a major released at the * specified date. @@ -6269,7 +6273,7 @@ class ReleaseAction { } /** * Builds and publishes the given version in the specified branch. - * @param newVersion The new version to be published. + * @param releaseNotes The release notes for the version being published. * @param publishBranch Name of the branch that contains the new version. * @param npmDistTag NPM dist tag where the version should be published to. */ diff --git a/dev-infra/release/publish/actions.ts b/dev-infra/release/publish/actions.ts index 6d067e93a1..8318bf8643 100644 --- a/dev-infra/release/publish/actions.ts +++ b/dev-infra/release/publish/actions.ts @@ -15,6 +15,7 @@ import {debug, error, green, info, promptConfirm, red, warn, yellow} from '../.. import {getListCommitsInBranchUrl, getRepositoryGitUrl} from '../../utils/git/github-urls'; import {GitClient} from '../../utils/git/index'; import {BuiltPackage, ReleaseConfig} from '../config/index'; +import {NpmDistTag} from '../versioning'; import {ActiveReleaseTrains} from '../versioning/active-release-trains'; import {runNpmPublish} from '../versioning/npm-publish'; @@ -440,12 +441,12 @@ export abstract class ReleaseAction { /** * Builds and publishes the given version in the specified branch. - * @param newVersion The new version to be published. + * @param releaseNotes The release notes for the version being published. * @param publishBranch Name of the branch that contains the new version. * @param npmDistTag NPM dist tag where the version should be published to. */ protected async buildAndPublish( - releaseNotes: ReleaseNotes, publishBranch: string, npmDistTag: string) { + releaseNotes: ReleaseNotes, publishBranch: string, npmDistTag: NpmDistTag) { const versionBumpCommitSha = await this._getCommitOfBranch(publishBranch); if (!await this._isCommitForVersionStaging(releaseNotes.version, versionBumpCommitSha)) { @@ -482,7 +483,7 @@ export abstract class ReleaseAction { } /** Publishes the given built package to NPM with the specified NPM dist tag. */ - private async _publishBuiltPackageToNpm(pkg: BuiltPackage, npmDistTag: string) { + private async _publishBuiltPackageToNpm(pkg: BuiltPackage, npmDistTag: NpmDistTag) { debug(`Starting publish of "${pkg.name}".`); const spinner = ora.call(undefined).start(`Publishing "${pkg.name}"`); diff --git a/dev-infra/release/publish/actions/cut-lts-patch.ts b/dev-infra/release/publish/actions/cut-lts-patch.ts index a58668141f..8777ca8329 100644 --- a/dev-infra/release/publish/actions/cut-lts-patch.ts +++ b/dev-infra/release/publish/actions/cut-lts-patch.ts @@ -7,23 +7,12 @@ */ import {ListChoiceOptions, prompt} from 'inquirer'; -import * as semver from 'semver'; import {ActiveReleaseTrains} from '../../versioning/active-release-trains'; import {semverInc} from '../../versioning/inc-semver'; -import {fetchLongTermSupportBranchesFromNpm} from '../../versioning/long-term-support'; +import {fetchLongTermSupportBranchesFromNpm, LtsBranch} from '../../versioning/long-term-support'; import {ReleaseAction} from '../actions'; -/** Interface describing an LTS version branch. */ -interface LtsBranch { - /** Name of the branch. */ - name: string; - /** Most recent version for the given LTS branch. */ - version: semver.SemVer; - /** NPM dist tag for the LTS version. */ - npmDistTag: string; -} - /** * Release action that cuts a new patch release for an active release-train in the long-term * support phase. The patch segment is incremented. The changelog is generated for the new diff --git a/dev-infra/release/publish/external-commands.ts b/dev-infra/release/publish/external-commands.ts index 3c9cfbe8f7..2683e3ce46 100644 --- a/dev-infra/release/publish/external-commands.ts +++ b/dev-infra/release/publish/external-commands.ts @@ -12,6 +12,7 @@ import * as semver from 'semver'; import {spawnWithDebugOutput} from '../../utils/child-process'; import {error, green, info, red} from '../../utils/console'; import {BuiltPackage} from '../config/index'; +import {NpmDistTag} from '../versioning'; import {FatalReleaseActionError} from './actions-error'; @@ -36,7 +37,7 @@ import {FatalReleaseActionError} from './actions-error'; * Invokes the `ng-dev release set-dist-tag` command in order to set the specified * NPM dist tag for all packages in the checked out branch to the given version. */ -export async function invokeSetNpmDistCommand(npmDistTag: string, version: semver.SemVer) { +export async function invokeSetNpmDistCommand(npmDistTag: NpmDistTag, version: semver.SemVer) { try { // Note: No progress indicator needed as that is the responsibility of the command. await spawnWithDebugOutput( diff --git a/dev-infra/release/publish/test/common.spec.ts b/dev-infra/release/publish/test/common.spec.ts index 28b6044c04..8bed8d97cf 100644 --- a/dev-infra/release/publish/test/common.spec.ts +++ b/dev-infra/release/publish/test/common.spec.ts @@ -11,7 +11,7 @@ import {join} from 'path'; import * as semver from 'semver'; import {getBranchPushMatcher} from '../../../utils/testing'; -import {_npmPackageInfoCache} from '../../versioning'; +import {NpmDistTag} from '../../versioning'; import {ActiveReleaseTrains} from '../../versioning/active-release-trains'; import * as npm from '../../versioning/npm-publish'; import {ReleaseTrain} from '../../versioning/release-trains'; @@ -44,7 +44,7 @@ describe('common release action logic', () => { fakeNpmPackageQueryRequest(releaseConfig.npmPackages[0], {'dist-tags': {}}); for (const actionCtor of actions) { - if (await actionCtor.isActive(testReleaseTrain)) { + if (await actionCtor.isActive(testReleaseTrain, releaseConfig)) { const action = new actionCtor(testReleaseTrain, gitClient, releaseConfig, testTmpDir); descriptions.push(await action.getDescription()); } @@ -151,7 +151,7 @@ class TestAction extends ReleaseAction { throw Error('Not implemented.'); } - async testBuildAndPublish(version: semver.SemVer, publishBranch: string, distTag: string) { + async testBuildAndPublish(version: semver.SemVer, publishBranch: string, distTag: NpmDistTag) { const releaseNotes = await ReleaseNotes.fromRange(version, '', ''); await this.buildAndPublish(releaseNotes, publishBranch, distTag); } diff --git a/dev-infra/release/publish/test/test-utils.ts b/dev-infra/release/publish/test/test-utils.ts index f463c635da..a2aa6f51a9 100644 --- a/dev-infra/release/publish/test/test-utils.ts +++ b/dev-infra/release/publish/test/test-utils.ts @@ -17,7 +17,7 @@ import {getBranchPushMatcher, installVirtualGitClientSpies, VirtualGitClient} fr import {ReleaseConfig} from '../../config/index'; import {ActiveReleaseTrains} from '../../versioning/active-release-trains'; import * as npm from '../../versioning/npm-publish'; -import {_npmPackageInfoCache, NpmPackageInfo} from '../../versioning/npm-registry'; +import {_npmPackageInfoCache, NpmDistTag, NpmPackageInfo} from '../../versioning/npm-registry'; import {ReleaseAction, ReleaseActionConstructor} from '../actions'; import * as constants from '../constants'; import * as externalCommands from '../external-commands'; @@ -124,8 +124,8 @@ export function parse(version: string): semver.SemVer { export async function expectStagingAndPublishWithoutCherryPick( action: TestReleaseAction, expectedBranch: string, expectedVersion: string, - expectedNpmDistTag: string) { - const {repo, fork, gitClient, releaseConfig} = action; + expectedNpmDistTag: NpmDistTag) { + const {repo, fork, gitClient} = action; const expectedStagingForkBranch = `release-stage-${expectedVersion}`; const expectedTagName = expectedVersion; @@ -173,7 +173,7 @@ export async function expectStagingAndPublishWithoutCherryPick( export async function expectStagingAndPublishWithCherryPick( action: TestReleaseAction, expectedBranch: string, expectedVersion: string, - expectedNpmDistTag: string) { + expectedNpmDistTag: NpmDistTag) { const {repo, fork, gitClient, releaseConfig} = action; const expectedStagingForkBranch = `release-stage-${expectedVersion}`; const expectedCherryPickForkBranch = `changelog-cherry-pick-${expectedVersion}`; diff --git a/dev-infra/release/versioning/long-term-support.ts b/dev-infra/release/versioning/long-term-support.ts index 308bd952ab..676b85b2fd 100644 --- a/dev-infra/release/versioning/long-term-support.ts +++ b/dev-infra/release/versioning/long-term-support.ts @@ -12,6 +12,9 @@ import {ReleaseConfig} from '../config/index'; import {fetchProjectNpmPackageInfo} from './npm-registry'; +/** Type describing a NPM dist tag indicating long-term support. */ +export type LtsNpmDistTag = `v${number}-lts`; + /** Interface describing determined LTS branches. */ export interface LtsBranches { /** List of active LTS version branches. */ @@ -27,7 +30,7 @@ export interface LtsBranch { /** Most recent version for the given LTS branch. */ version: semver.SemVer; /** NPM dist tag for the LTS version. */ - npmDistTag: string; + npmDistTag: LtsNpmDistTag; } /** @@ -57,7 +60,7 @@ export async function fetchLongTermSupportBranchesFromNpm(config: ReleaseConfig) // their corresponding branches. We assume that an LTS tagged version in NPM belongs to the // last-minor branch of a given major (i.e. we assume there are no outdated LTS NPM dist tags). for (const npmDistTag in distTags) { - if (ltsNpmDistTagRegex.test(npmDistTag)) { + if (isLtsDistTag(npmDistTag)) { const version = semver.parse(distTags[npmDistTag])!; const branchName = `${version.major}.${version.minor}.x`; const majorReleaseDate = new Date(time[`${version.major}.0.0`]); @@ -80,6 +83,11 @@ export async function fetchLongTermSupportBranchesFromNpm(config: ReleaseConfig) return {active, inactive}; } +/** Gets whether the specified tag corresponds to a LTS dist tag. */ +export function isLtsDistTag(tagName: string): tagName is LtsNpmDistTag { + return ltsNpmDistTagRegex.test(tagName); +} + /** * Computes the date when long-term support ends for a major released at the * specified date. @@ -93,7 +101,7 @@ export function computeLtsEndDateOfMajor(majorReleaseDate: Date): Date { } /** Gets the long-term support NPM dist tag for a given major version. */ -export function getLtsNpmDistTagOfMajor(major: number): string { +export function getLtsNpmDistTagOfMajor(major: number): LtsNpmDistTag { // LTS versions should be tagged in NPM in the following format: `v{major}-lts`. - return `v${major}-lts`; + return `v${major}-lts` as const; } diff --git a/dev-infra/release/versioning/npm-publish.ts b/dev-infra/release/versioning/npm-publish.ts index 4f71583b11..1f31c268a5 100644 --- a/dev-infra/release/versioning/npm-publish.ts +++ b/dev-infra/release/versioning/npm-publish.ts @@ -8,13 +8,14 @@ import * as semver from 'semver'; import {spawnInteractiveCommand, spawnWithDebugOutput} from '../../utils/child-process'; +import {NpmDistTag} from './npm-registry'; /** * Runs NPM publish within a specified package directory. * @throws With the process log output if the publish failed. */ export async function runNpmPublish( - packagePath: string, distTag: string, registryUrl: string|undefined) { + packagePath: string, distTag: NpmDistTag, registryUrl: string|undefined) { const args = ['publish', '--access', 'public', '--tag', distTag]; // If a custom registry URL has been specified, add the `--registry` flag. if (registryUrl !== undefined) { diff --git a/dev-infra/release/versioning/npm-registry.ts b/dev-infra/release/versioning/npm-registry.ts index 2e0419ee69..de6daf0970 100644 --- a/dev-infra/release/versioning/npm-registry.ts +++ b/dev-infra/release/versioning/npm-registry.ts @@ -10,6 +10,10 @@ import fetch from 'node-fetch'; import * as semver from 'semver'; import {ReleaseConfig} from '../config/index'; +import {LtsNpmDistTag} from './long-term-support'; + +/** Type describing the possible NPM dist tags used by Angular packages. */ +export type NpmDistTag = 'latest'|'next'|LtsNpmDistTag; /** Type describing an NPM package fetched from the registry. */ export interface NpmPackageInfo {