Skip to content

feat(ng-dev/release): add marker between generated changelog entries #212

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

Closed
Closed
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
28 changes: 24 additions & 4 deletions ng-dev/release/notes/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
load("//tools:defaults.bzl", "ts_library")
load("//tools:defaults.bzl", "jasmine_node_test", "ts_library")

ts_library(
name = "notes",
srcs = glob([
"**/*.ts",
]),
srcs = glob(
["**/*.ts"],
exclude = ["**/*.spec.ts"],
),
visibility = [
"//ng-dev:__subpackages__",
"//tools/local-actions/changelog/lib:__subpackages__",
Expand All @@ -23,3 +24,22 @@ ts_library(
"@npm//semver",
],
)

ts_library(
name = "test_lib",
testonly = True,
srcs = glob(["**/*.spec.ts"]),
deps = [
":notes",
"//ng-dev/release/publish/test:test_lib",
"//ng-dev/utils",
"//ng-dev/utils/testing",
"@npm//@types/semver",
"@npm//semver",
],
)

jasmine_node_test(
name = "test",
specs = [":test_lib"],
)
117 changes: 117 additions & 0 deletions ng-dev/release/notes/changelog.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import {existsSync, readFileSync} from 'fs';
import {SemVer} from 'semver';
import {dedent} from '../../utils/testing/dedent';
import {getMockGitClient} from '../publish/test/test-utils/git-client-mock';
import {Changelog, splitMarker} from './changelog';

describe('Changelog', () => {
let changelog: Changelog;

beforeEach(() => {
const gitClient = getMockGitClient(
{owner: 'angular', name: 'dev-infra-test', mainBranchName: 'main'},
/* useSandboxGitClient */ false,
);
changelog = new Changelog(gitClient);
});

it('throws an error if it cannot find the anchor containing the version for an entry', () => {
expect(() => changelog.prependEntryToChangelog('does not have version <a> tag')).toThrow();
});

it('throws an error if it cannot determine the version for an entry', () => {
expect(() => changelog.prependEntryToChangelog(createChangelogEntry('NotSemVer'))).toThrow();
});

it('concatenates the changelog entries into the changelog file with the split marker between', () => {
changelog.prependEntryToChangelog(createChangelogEntry('1.0.0'));
changelog.prependEntryToChangelog(createChangelogEntry('2.0.0'));
changelog.prependEntryToChangelog(createChangelogEntry('3.0.0'));

expect(readFileAsString(changelog.filePath)).toBe(
dedent`
<a name="3.0.0"></a>

${splitMarker}

<a name="2.0.0"></a>

${splitMarker}

<a name="1.0.0"></a>
`.trim(),
);

changelog.moveEntriesPriorToVersionToArchive(new SemVer('3.0.0'));

expect(readFileAsString(changelog.archiveFilePath)).toBe(
dedent`
<a name="2.0.0"></a>

${splitMarker}

<a name="1.0.0"></a>
`.trim(),
);

expect(readFileAsString(changelog.filePath)).toBe(`<a name="3.0.0"></a>`);
});

describe('adds entries to the changelog', () => {
it('creates a new changelog file if one does not exist.', () => {
expect(existsSync(changelog.filePath)).toBe(false);

changelog.prependEntryToChangelog(createChangelogEntry('0.0.0'));
expect(existsSync(changelog.filePath)).toBe(true);
});

it('should not include a split marker when only one changelog entry is in the changelog.', () => {
changelog.prependEntryToChangelog(createChangelogEntry('0.0.0'));

expect(readFileAsString(changelog.filePath)).not.toContain(splitMarker);
});

it('separates multiple changelog entries using a standard split marker', () => {
for (let i = 0; i < 2; i++) {
changelog.prependEntryToChangelog(createChangelogEntry(`0.0.${i}`));
}

expect(readFileAsString(changelog.filePath)).toContain(splitMarker);
});
});

describe('adds entries to the changelog archive', () => {
it('only updates or creates the changelog archive if necessary', () => {
changelog.prependEntryToChangelog(createChangelogEntry('1.0.0'));
expect(existsSync(changelog.archiveFilePath)).toBe(false);

changelog.moveEntriesPriorToVersionToArchive(new SemVer('1.0.0'));
expect(existsSync(changelog.archiveFilePath)).toBe(false);

changelog.moveEntriesPriorToVersionToArchive(new SemVer('2.0.0'));
expect(existsSync(changelog.archiveFilePath)).toBe(true);
});

it('from the primary changelog older than a provided version', () => {
changelog.prependEntryToChangelog(createChangelogEntry('1.0.0', 'This is version 1'));
changelog.prependEntryToChangelog(createChangelogEntry('2.0.0', 'This is version 2'));
changelog.prependEntryToChangelog(createChangelogEntry('3.0.0', 'This is version 3'));

changelog.moveEntriesPriorToVersionToArchive(new SemVer('3.0.0'));
expect(readFileAsString(changelog.archiveFilePath)).toContain('version 1');
expect(readFileAsString(changelog.archiveFilePath)).toContain('version 2');
expect(readFileAsString(changelog.archiveFilePath)).not.toContain('version 3');
});
});
});

function readFileAsString(file: string) {
return readFileSync(file, {encoding: 'utf8'});
}

function createChangelogEntry(version: string, content = '') {
return dedent`
<a name="${version}"></a>
${content}
`;
}
142 changes: 142 additions & 0 deletions ng-dev/release/notes/changelog.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import {existsSync, readFileSync, writeFileSync} from 'fs';
import {join} from 'path';
import * as semver from 'semver';
import {GitClient} from '../../utils/git/git-client';

/** Project-relative path for the changelog file. */
export const changelogPath = 'CHANGELOG.md';

/** Project-relative path for the changelog archive file. */
export const changelogArchivePath = 'CHANGELOG_ARCHIVE.md';

/** A marker used to split a CHANGELOG.md file into individual entries. */
export const splitMarker = '<!-- CHANGELOG SPLIT MARKER -->';

/**
* A string to use between each changelog entry when joining them together.
*
* Since all every changelog entry's content is trimmed, when joining back together, two new lines
* must be placed around the splitMarker to create a one line buffer around the comment in the
* markdown.
* i.e.
* <changelog entry content>
*
* <!-- CHANGELOG SPLIT MARKER -->
*
* <changelog entry content>
*/
const joinMarker = `\n\n${splitMarker}\n\n`;

/** A RegExp matcher to extract the version of a changelog entry from the entry content. */
const versionAnchorMatcher = new RegExp(`<a name="(.*)"></a>`);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit unhappy about the reliance on some template-specifics here. It will become problematic if we make changes of the anchor. e.g. even when we switch from <a name to <a id (which is actually better for fragment resolution in w3c).

If you see my comment on moveEntriesPriorToVersionToArchive, an option is that we might not need the version at all, or alternatively we could incorporate the version in a specific comment that is generated here as part of the Changelog class (that keeps the logic more local and self-contained)

Copy link
Member Author

@josephperrott josephperrott Sep 13, 2021

Choose a reason for hiding this comment

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

Should we include a metadata comment object? Something like.

<!--ReleaseMetadata {version: '1.2.3', name: 'release name'} -->

Then we can retrieve it with a regex and parse the results. We can then JSON.parse the matched content into something like Partial<ReleaseMetadata> where we have

interface ReleaseMetadata {
  version: string;
  name: string;
}


const metadataMatcher = RegExp(/<!--ReleaseMetadata.*(\{.*\})/);

for (const entry of entries) {
  const metadata = JSON.parse(entry.match(metadataMatcher)[1]) as Partial<ReleaseMetadata>;
}

There would need to be more validation/assertions I suppose, but this concept.

Copy link
Member

@devversion devversion Sep 13, 2021

Choose a reason for hiding this comment

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

yeah, something around that is what I'd love to see. Could also be just something like:

<!-- ReleaseMetadata: Version|Name -->

Doesn't necessarily need to be JSON. I'm happy with either thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense to use JSON since it will make parsing easier. It won't be as whitespace sensitive and is easy to roll and unroll.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually as I am looking at it now, I think it kind of ends up being a lot more change than we currently have to set this up, how do you feel about doing it in a follow up PR? It just feels like it ends up nearly unrelated to the current change.

Copy link
Member

Choose a reason for hiding this comment

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

sure, works for me!

Copy link
Member Author

Choose a reason for hiding this comment

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

Creating tracking issue at: #219


/** An individual changelog entry. */
interface ChangelogEntry {
content: string;
version: semver.SemVer;
}

export class Changelog {
/** The absolute path to the changelog file. */
readonly filePath = join(this.git.baseDir, changelogPath);
/** The absolute path to the changelog archive file. */
readonly archiveFilePath = join(this.git.baseDir, changelogArchivePath);
/** The changelog entries in the CHANGELOG.md file. */
private entries = this.getEntriesFor(this.filePath);
/**
* The changelog entries in the CHANGELOG_ARCHIVE.md file.
* Delays reading the CHANGELOG_ARCHIVE.md file until it is actually used.
*/
private get archiveEntries() {
if (this._archiveEntries === undefined) {
return (this._archiveEntries = this.getEntriesFor(this.archiveFilePath));
}
return this._archiveEntries;
}
private _archiveEntries: undefined | ChangelogEntry[] = undefined;

constructor(private git: GitClient) {}

/** Prepend a changelog entry to the changelog. */
prependEntryToChangelog(entry: string) {
this.entries.unshift(parseChangelogEntry(entry));
this.writeToChangelogFile();
}

/**
* Move all changelog entries from the CHANGELOG.md file for versions prior to the provided
* version to the changelog archive.
*
* Versions should be used to determine which entries are moved to archive as versions are the
* most accurate piece of context found within a changelog entry to determine its relationship to
* other changelog entries. This allows for example, moving all changelog entries out of the
* main changelog when a version moves out of support.
*/
moveEntriesPriorToVersionToArchive(version: semver.SemVer) {
[...this.entries].reverse().forEach((entry: ChangelogEntry) => {
if (semver.lt(entry.version, version)) {
this.archiveEntries.unshift(entry);
this.entries.splice(this.entries.indexOf(entry), 1);
}
});

this.writeToChangelogFile();
if (this.archiveEntries.length) {
this.writeToChangelogArchiveFile();
}
}

/** Update the changelog archive file with the known changelog archive entries. */
private writeToChangelogArchiveFile(): void {
const changelogArchive = this.archiveEntries.map((entry) => entry.content).join(joinMarker);
writeFileSync(this.archiveFilePath, changelogArchive);
}

/** Update the changelog file with the known changelog entries. */
private writeToChangelogFile(): void {
const changelog = this.entries.map((entry) => entry.content).join(joinMarker);
writeFileSync(this.filePath, changelog);
}

/**
* Retrieve the changelog entries for the provide changelog path, if the file does not exist an
* empty array is returned.
*/
private getEntriesFor(path: string): ChangelogEntry[] {
if (!existsSync(path)) {
return [];
}

return (
readFileSync(path, {encoding: 'utf8'})
// Use the versionMarker as the separator for .split().
.split(splitMarker)
// If the `split()` method finds the separator at the beginning or end of a string, it
// includes an empty string at the respective locaiton, so we filter to remove all of these
// potential empty strings.
.filter((entry) => entry.trim().length !== 0)
// Create a ChangelogEntry for each of the string entry.
.map(parseChangelogEntry)
);
}
}

/** Parse the provided string into a ChangelogEntry object. */
function parseChangelogEntry(content: string): ChangelogEntry {
const versionMatcherResult = versionAnchorMatcher.exec(content);
if (versionMatcherResult === null) {
throw Error(`Unable to determine version for changelog entry: ${content}`);
}
const version = semver.parse(versionMatcherResult[1]);

if (version === null) {
throw Error(
`Unable to determine version for changelog entry, with tag: ${versionMatcherResult[1]}`,
);
}

return {
content: content.trim(),
version,
};
}
19 changes: 5 additions & 14 deletions ng-dev/release/notes/release-notes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@ import changelogTemplate from './templates/changelog';
import githubReleaseTemplate from './templates/github-release';
import {getCommitsForRangeWithDeduping} from './commits/get-commits-in-range';
import {getConfig} from '../../utils/config';
import {existsSync, readFileSync, writeFileSync} from 'fs';
import {join} from 'path';
import {assertValidFormatConfig} from '../../format/config';
import {Changelog} from './changelog';

/** Project-relative path for the changelog file. */
export const changelogPath = 'CHANGELOG.md';
Expand All @@ -34,8 +33,8 @@ export class ReleaseNotes {
return new ReleaseNotes(version, commits, git);
}

/** The absolute path to the changelog file. */
private changelogPath = join(this.git.baseDir, changelogPath);
/** The changelog writer. */
private changelog = new Changelog(this.git);
/** The RenderContext to be used during rendering. */
private renderContext: RenderContext | undefined;
/** The title to use for the release. */
Expand Down Expand Up @@ -70,23 +69,15 @@ export class ReleaseNotes {
* provided by the GitClient.
*/
async prependEntryToChangelog() {
/** The changelog contents in the current changelog. */
let changelog = '';
if (existsSync(this.changelogPath)) {
changelog = readFileSync(this.changelogPath, {encoding: 'utf8'});
}
/** The new changelog entry to add to the changelog. */
const entry = await this.getChangelogEntry();

writeFileSync(this.changelogPath, `${entry}\n\n${changelog}`);
this.changelog.prependEntryToChangelog(await this.getChangelogEntry());

// TODO(josephperrott): Remove file formatting calls.
// Upon reaching a standardized formatting for markdown files, rather than calling a formatter
// for all creation of changelogs, we instead will confirm in our testing that the new changes
// created for changelogs meet on standardized markdown formats via unit testing.
try {
assertValidFormatConfig(this.config);
await formatFiles([this.changelogPath]);
await formatFiles([this.changelog.filePath]);
} catch {
// If the formatting is either unavailable or fails, continue on with the unformatted result.
}
Expand Down
3 changes: 3 additions & 0 deletions ng-dev/release/publish/test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ ts_library(
srcs = glob([
"**/*.ts",
]),
visibility = [
"//ng-dev/release/notes:__subpackages__",
],
deps = [
"//ng-dev/commit-message",
"//ng-dev/release/config",
Expand Down
2 changes: 2 additions & 0 deletions ng-dev/release/publish/test/common.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,9 @@ describe('common release action logic', () => {
expect(changelogContent).toMatch(changelogPattern`
# 10.0.1 <..>

<!-- CHANGELOG SPLIT MARKER -->

<a name="0.0.0"></a>
Existing changelog
`);
});
Expand Down
14 changes: 0 additions & 14 deletions ng-dev/release/publish/test/release-notes/generation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -453,18 +453,4 @@ describe('release notes generation', () => {
`| [${shortSha}](https://github.com/angular/dev-infra-test/commit/${fullSha}) | fix | commit *1 |`,
);
});

it('updates the changelog file by prepending the entry to the current changelog', async () => {
writeFileSync(`${testTmpDir}/CHANGELOG.md`, '<Previous Changelog Entries>');

SandboxGitRepo.withInitialCommit(githubConfig).createTagForHead('startTag');

const releaseNotes = await ReleaseNotes.forRange(parse('13.0.0'), 'startTag', 'HEAD');
await releaseNotes.prependEntryToChangelog();

const changelog = readFileSync(`${testTmpDir}/CHANGELOG.md`, 'utf8');

const entry = await releaseNotes.getChangelogEntry();
expect(changelog).toBe(`${entry}\n\n<Previous Changelog Entries>`);
});
});
2 changes: 1 addition & 1 deletion ng-dev/release/publish/test/test-utils/action-mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export function setupMocksForReleaseAction<T extends boolean>(

// Create an empty changelog and a `package.json` file so that file system
// interactions with the project directory do not cause exceptions.
writeFileSync(join(testTmpDir, 'CHANGELOG.md'), 'Existing changelog');
writeFileSync(join(testTmpDir, 'CHANGELOG.md'), '<a name="0.0.0"></a>\nExisting changelog');
writeFileSync(join(testTmpDir, 'package.json'), JSON.stringify({version: '0.0.0'}));

// Override the default pull request wait interval to a number of milliseconds that can be
Expand Down
Loading