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 repo links in updated changelogs to match auto-changelog #165

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
17 changes: 3 additions & 14 deletions src/functional.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,9 @@ describe('create-release-branch (functional)', () => {
},
});

expect(await environment.readJsonFile('package.json')).toStrictEqual({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are updated because there's now an extra repository field in package.json. That isn't really relevant to these tests, so I've simplified the assertions to only consider name and version.

expect(await environment.readJsonFile('package.json')).toMatchObject({
name: '@scope/monorepo',
version: '2.0.0',
private: true,
workspaces: ['packages/*'],
scripts: { foo: 'bar' },
packageManager: '[email protected]',
});
expect(
await environment.readJsonFileWithinPackage('a', 'package.json'),
Expand Down Expand Up @@ -190,13 +186,9 @@ describe('create-release-branch (functional)', () => {
},
});

expect(await environment.readJsonFile('package.json')).toStrictEqual({
expect(await environment.readJsonFile('package.json')).toMatchObject({
name: '@scope/monorepo',
version: '1.1.0',
private: true,
workspaces: ['packages/*'],
scripts: { foo: 'bar' },
packageManager: '[email protected]',
});
expect(
await environment.readJsonFileWithinPackage('a', 'package.json'),
Expand Down Expand Up @@ -945,12 +937,9 @@ __metadata:
},
});

expect(await environment.readJsonFile('package.json')).toStrictEqual({
expect(await environment.readJsonFile('package.json')).toMatchObject({
name: '@scope/monorepo',
version: '2.0.0',
private: true,
workspaces: ['packages/*'],
packageManager: '[email protected]',
});
expect(
await environment.readJsonFileWithinPackage('a', 'package.json'),
Expand Down
46 changes: 46 additions & 0 deletions src/misc-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
runCommand,
getStdoutFromCommand,
getLinesFromCommand,
convertToHttpsGitHubRepositoryUrl,
} from './misc-utils.js';

jest.mock('which');
Expand Down Expand Up @@ -211,4 +212,49 @@ describe('misc-utils', () => {
expect(lines).toStrictEqual([' line 1', 'line 2', ' line 3 ']);
});
});

describe('convertToHttpsRepositoryUrl', () => {
it('returns the URL of the "origin" remote of the given repo if it looks like a HTTPS public GitHub repo URL', () => {
expect(
convertToHttpsGitHubRepositoryUrl(
'https://github.com/example-org/example-repo',
),
).toBe('https://github.com/example-org/example-repo');
});

it('lops ".git" off from the HTTPS public GitHub repo URL', () => {
expect(
convertToHttpsGitHubRepositoryUrl(
'https://github.com/example-org/example-repo.git',
),
).toBe('https://github.com/example-org/example-repo');
});

it('converts an SSH GitHub repo URL into an HTTPS URL (without the trailing ".git")', () => {
expect(
convertToHttpsGitHubRepositoryUrl(
'[email protected]:example-org/example-repo.git',
),
).toBe('https://github.com/example-org/example-repo');
});

it.each([
'foo',
'http://github.com/example-org',
'https://github.com/example-org',
'http://github.com/example-org/example-repo',
'https://github.comzzzz/example-org/example-repo',
'https://gitbar.foo/example-org/example-repo',
'[email protected]:example-org',
'[email protected]:example-org/example-repo.git',
'[email protected]:example-org/example-repo.foo',
])(
'throws if the URL is in an invalid format such as "%s"',
(repositoryUrl) => {
expect(() => convertToHttpsGitHubRepositoryUrl(repositoryUrl)).toThrow(
`Unrecognized repository URL: ${repositoryUrl}`,
);
},
);
});
});
39 changes: 39 additions & 0 deletions src/misc-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,20 @@ export { isObject };
*/
export const debug = createDebug('create-release-branch:impl');

/**
* Matches URLs in the formats:
*
* - "https://github.com/OrganizationName/RepoName"
* - "https://github.com/OrganizationName/RepoName.git"
*/
const HTTPS_GITHUB_URL_REGEX =
/^https:\/\/github\.com\/(.+?)\/(.+?)(?:\.git)?$/u;

/**
* Matches a URL in the format "[email protected]/OrganizationName/RepoName.git".
*/
const SSH_GITHUB_URL_REGEX = /^git@github\.com:(.+?)\/(.+?)\.git$/u;

/**
* Type guard for determining whether the given value is an instance of Error.
* For errors generated via `fs.promises`, `error instanceof Error` won't work,
Expand Down Expand Up @@ -172,3 +186,28 @@ export async function getLinesFromCommand(
const { stdout } = await execa(command, args, options);
return stdout.split('\n').filter((value) => value !== '');
}

/**
* Converts the given GitHub repository URL to its HTTPS version.
*
* A "GitHub repository URL" looks like one of:
*
* - https://github.com/OrganizationName/RepositoryName
* - [email protected]:OrganizationName/RepositoryName.git
*
* If the URL does not match either of these patterns, an error is thrown.
*
* @param url - The URL to convert.
* @returns The HTTPS URL of the repository, e.g.
* `https://github.com/OrganizationName/RepositoryName`.
*/
export function convertToHttpsGitHubRepositoryUrl(url: string): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic is not in auto-changelog, but is in this repo, so we're actually doing extra. I was curious why there was a distinction, so I did a bit of digging:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I've simplified the original logic to take a full-on regex approach. It seemed that the code wanted to go there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will probably end up copying this back to action-create-release-pr so that it is consistent between the two.

const match =
url.match(HTTPS_GITHUB_URL_REGEX) ?? url.match(SSH_GITHUB_URL_REGEX);

if (match) {
return `https://github.com/${match[1]}/${match[2]}`;
}

throw new Error(`Unrecognized repository URL: ${url}`);
}
77 changes: 73 additions & 4 deletions src/project.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ import path from 'path';
import { when } from 'jest-when';
import { SemVer } from 'semver';
import * as actionUtils from '@metamask/action-utils';
import { withSandbox } from '../tests/helpers.js';
import { withProtectedProcessEnv, withSandbox } from '../tests/helpers.js';
import {
buildMockPackage,
buildMockProject,
createNoopWriteStream,
} from '../tests/unit/helpers.js';
import * as miscUtils from './misc-utils.js';
import {
getValidRepositoryUrl,
readProject,
restoreChangelogsForSkippedPackages,
updateChangelogsForChangedPackages,
Expand Down Expand Up @@ -42,6 +44,11 @@ describe('project', () => {
validatedManifest: {
workspaces: ['packages/a', 'packages/subpackages/*'],
},
unvalidatedManifest: {
repository: {
url: projectRepositoryUrl,
},
},
},
);
const workspacePackages = {
Expand All @@ -59,9 +66,6 @@ describe('project', () => {
};
const projectTagNames = ['tag1', 'tag2', 'tag3'];
const stderr = createNoopWriteStream();
when(jest.spyOn(repoModule, 'getRepositoryHttpsUrl'))
.calledWith(projectDirectoryPath)
.mockResolvedValue(projectRepositoryUrl);
when(jest.spyOn(repoModule, 'getTagNames'))
.calledWith(projectDirectoryPath)
.mockResolvedValue(projectTagNames);
Expand Down Expand Up @@ -126,6 +130,71 @@ describe('project', () => {
});
});
});

describe('getValidRepositoryUrl', () => {
describe('if the `npm_package_repository_url` environment variable is set', () => {
it('returns the HTTPS version of this URL', async () => {
await withProtectedProcessEnv(async () => {
process.env.npm_package_repository_url =
'[email protected]:example-org/example-repo.git';
const packageManifest = {};
const repositoryDirectoryPath = '/path/to/project';

expect(
await getValidRepositoryUrl(
packageManifest,
repositoryDirectoryPath,
),
).toBe('https://github.com/example-org/example-repo');
});
});
});

describe('if package.json has a string "repository" field', () => {
it('returns the HTTPS version of this URL', async () => {
const packageManifest = {
repository: '[email protected]:example-org/example-repo.git',
};
const repositoryDirectoryPath = '/path/to/project';

expect(
await getValidRepositoryUrl(packageManifest, repositoryDirectoryPath),
).toBe('https://github.com/example-org/example-repo');
});
});

describe('if package.json has an object "repository" field with a string "url" field', () => {
it('returns the HTTPS version of this URL', async () => {
const packageManifest = {
repository: {
url: '[email protected]:example-org/example-repo.git',
},
};
const repositoryDirectoryPath = '/path/to/project';

expect(
await getValidRepositoryUrl(packageManifest, repositoryDirectoryPath),
).toBe('https://github.com/example-org/example-repo');
});
});

describe('if package.json does not have a "repository" field', () => {
it('returns the HTTPS version of this URL', async () => {
const packageManifest = {};
const repositoryDirectoryPath = '/path/to/project';
when(jest.spyOn(miscUtils, 'getStdoutFromCommand'))
.calledWith('git', ['config', '--get', 'remote.origin.url'], {
cwd: repositoryDirectoryPath,
})
.mockResolvedValue('[email protected]:example-org/example-repo.git');

expect(
await getValidRepositoryUrl(packageManifest, repositoryDirectoryPath),
).toBe('https://github.com/example-org/example-repo');
});
});
});

describe('restoreChangelogsForSkippedPackages', () => {
it('should reset changelog for packages with changes not included in release', async () => {
const project = buildMockProject({
Expand Down
73 changes: 70 additions & 3 deletions src/project.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import { WriteStream } from 'fs';
import { resolve } from 'path';
import { getWorkspaceLocations } from '@metamask/action-utils';
import { isPlainObject } from '@metamask/utils';
import { WriteStreamLike, fileExists } from './fs.js';
import {
Package,
readMonorepoRootPackage,
readMonorepoWorkspacePackage,
updatePackageChangelog,
} from './package.js';
import { getRepositoryHttpsUrl, getTagNames, restoreFiles } from './repo.js';
import { getTagNames, restoreFiles } from './repo.js';
import { SemVer } from './semver.js';
import { PackageManifestFieldNames } from './package-manifest.js';
import {
PackageManifestFieldNames,
UnvalidatedPackageManifest,
} from './package-manifest.js';
import { ReleaseSpecification } from './release-specification.js';
import {
convertToHttpsGitHubRepositoryUrl,
getStdoutFromCommand,
} from './misc-utils.js';

/**
* The release version of the root package of a monorepo extracted from its
Expand Down Expand Up @@ -83,13 +91,16 @@ export async function readProject(
projectDirectoryPath: string,
{ stderr }: { stderr: WriteStreamLike },
): Promise<Project> {
const repositoryUrl = await getRepositoryHttpsUrl(projectDirectoryPath);
const tagNames = await getTagNames(projectDirectoryPath);
const rootPackage = await readMonorepoRootPackage({
packageDirectoryPath: projectDirectoryPath,
projectDirectoryPath,
projectTagNames: tagNames,
});
const repositoryUrl = await getValidRepositoryUrl(
rootPackage.unvalidatedManifest,
projectDirectoryPath,
);
const releaseVersion = examineReleaseVersion(
rootPackage.validatedManifest.version,
);
Expand Down Expand Up @@ -132,6 +143,62 @@ export async function readProject(
};
}

/**
* Returns the https-prefixed GitHub repository URL for the project, so that we
* know how to construct links to releases inside of the changelog.
*
* This URL is obtained by looking at the following sources, in this order:
*
* - `process.env.npm_package_repository_url` (to support MetaMask projects
* which still use NPM or Yarn 1.x)
* - The `repository` field inside of `package.json` (to support most MetaMask
* projects)
* - The URL of the "origin" remote (to support newer projects which for some
* reason do not have a "repository" field)
*
* Note that there is a function in `@metamask/auto-changelog` that offers a
* similar capability, and in fact, this function should match it as closely as
* possible so that when changelogs are updated they also pass
* `@metamask/auto-changelog`'s validation step.
*
* @param packageManifest - The manifest for the package that represents a
* project.
* @param repositoryDirectoryPath - The path to the repository for the project.
* @returns The HTTPS URL of the repository, e.g.
* `https://github.com/OrganizationName/RepositoryName`.
* @throws If the URL obtained via `package.json` or the "origin" remote is in
* in invalid format.
*/
export async function getValidRepositoryUrl(
packageManifest: UnvalidatedPackageManifest,
repositoryDirectoryPath: string,
): Promise<string> {
// Set automatically by NPM or Yarn 1.x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although handling NPM or Yarn 1.x projects is not strictly necessary here, because create-release-branch only supports monorepos, and all of our monorepos use Yarn Modern, I figured I'd include it here because I want to copy this function back to auto-changelog after this PR is merged.

For context the original source for this logic is here: https://github.com/MetaMask/auto-changelog/blob/ef3e86e15b0de7061856a53fd18c4f38e898f5e8/src/repo.ts#L20

const npmPackageRepositoryUrl = process.env.npm_package_repository_url;

if (npmPackageRepositoryUrl) {
return convertToHttpsGitHubRepositoryUrl(npmPackageRepositoryUrl);
}

if (typeof packageManifest.repository === 'string') {
return convertToHttpsGitHubRepositoryUrl(packageManifest.repository);
}

if (
isPlainObject(packageManifest.repository) &&
typeof packageManifest.repository.url === 'string'
) {
return convertToHttpsGitHubRepositoryUrl(packageManifest.repository.url);
}

const gitConfigUrl = await getStdoutFromCommand(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that most of our projects ought to have a repository field in package.json, I am not sure that this is really needed, but I figured I'd leave it here.

I tracked down the original source for this logic, and it comes from here: MetaMask/action-monorepo-release-pr@af8d750#diff-b2d6c9ae0590887f3d213032d057be61bd3e628650fb7ba99391bdc84587e454R25

'git',
['config', '--get', 'remote.origin.url'],
{ cwd: repositoryDirectoryPath },
);
return convertToHttpsGitHubRepositoryUrl(gitConfigUrl);
}

/**
* Updates the changelog files of all packages that have changes since latest release to include those changes.
*
Expand Down
Loading
Loading