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: Use strategies for workspace plugins. #2112

Merged
merged 13 commits into from
Dec 28, 2023
Merged
4 changes: 2 additions & 2 deletions src/plugins/cargo-workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,10 @@ export class CargoWorkspace extends WorkspacePlugin<CrateInfo> {
return existingCandidate;
}

protected newCandidate(
protected async newCandidate(
pkg: CrateInfo,
updatedVersions: VersionsMap
): CandidateReleasePullRequest {
): Promise<CandidateReleasePullRequest> {
const version = updatedVersions.get(pkg.name);
if (!version) {
throw new Error(`Didn't find updated version for ${pkg.name}`);
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/maven-workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,10 +394,10 @@ export class MavenWorkspace extends WorkspacePlugin<MavenArtifact> {
}
return existingCandidate;
}
protected newCandidate(
protected async newCandidate(
artifact: MavenArtifact,
updatedVersions: VersionsMap
): CandidateReleasePullRequest {
): Promise<CandidateReleasePullRequest> {
const version = updatedVersions.get(artifact.name);
if (!version) {
throw new Error(`Didn't find updated version for ${artifact.name}`);
Expand Down
49 changes: 40 additions & 9 deletions src/plugins/node-workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ import {
addPath,
} from './workspace';
import {PatchVersionUpdate} from '../versioning-strategy';
import {Strategy} from '../strategy';
import {Commit} from '../commit';
import {Release} from '../release';

class Package extends LernaPackage {
constructor(
Expand Down Expand Up @@ -69,6 +72,10 @@ interface NodeWorkspaceOptions extends WorkspacePluginOptions {
export class NodeWorkspace extends WorkspacePlugin<Package> {
private alwaysLinkLocal: boolean;
private packageGraph?: PackageGraph;

private strategiesByPath: Record<string, Strategy> = {};
private releasesByPath: Record<string, Release> = {};

constructor(
github: GitHub,
targetBranch: string,
Expand Down Expand Up @@ -232,10 +239,10 @@ export class NodeWorkspace extends WorkspacePlugin<Package> {
}
return existingCandidate;
}
protected newCandidate(
protected async newCandidate(
pkg: Package,
updatedVersions: VersionsMap
): CandidateReleasePullRequest {
): Promise<CandidateReleasePullRequest> {
const graphPackage = this.packageGraph?.get(pkg.name);
if (!graphPackage) {
throw new Error(`Could not find graph package for ${pkg.name}`);
Expand Down Expand Up @@ -271,20 +278,31 @@ export class NodeWorkspace extends WorkspacePlugin<Package> {
);
const packageJson = updatedPackage.toJSON() as PackageJson;
const version = Version.parse(packageJson.version);

const strategy = this.strategiesByPath[updatedPackage.location];
const latestRelease = this.releasesByPath[updatedPackage.location];

const basePullRequest = strategy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make a base pull request that contains the "extra-files" updaters as well as the changelog header.

? await strategy.buildReleasePullRequest([], latestRelease, false, [], {
newVersion: version,
})
: undefined;

const pullRequest: ReleasePullRequest = {
title: PullRequestTitle.ofTargetBranch(this.targetBranch),
body: new PullRequestBody([
{
component: updatedPackage.name,
version,
notes: appendDependenciesSectionToChangelog(
'',
basePullRequest?.body.notes() ?? '',
dependencyNotes,
this.logger
),
},
]),
updates: [
...(basePullRequest ? basePullRequest?.updates : []),
{
path: addPath(updatedPackage.location, 'package.json'),
createIfMissing: false,
Expand All @@ -297,16 +315,17 @@ export class NodeWorkspace extends WorkspacePlugin<Package> {
createIfMissing: false,
updater: new Changelog({
version,
changelogEntry: appendDependenciesSectionToChangelog(
'',
dependencyNotes,
this.logger
),
versionHeaderRegex: `\n###? v?[${version
kinyoklion marked this conversation as resolved.
Show resolved Hide resolved
.toString()
.replace('.', '.')}]`,
changelogEntry: dependencyNotes,
}),
},
],
labels: [],
headRefName: BranchName.ofTargetBranch(this.targetBranch).toString(),
headRefName:
basePullRequest?.headRefName ??
BranchName.ofTargetBranch(this.targetBranch).toString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For #2109

version,
draft: false,
};
Expand Down Expand Up @@ -375,6 +394,18 @@ export class NodeWorkspace extends WorkspacePlugin<Package> {
...(packageJson.optionalDependencies ?? {}),
};
}

async preconfigure(
strategiesByPath: Record<string, Strategy>,
_commitsByPath: Record<string, Commit[]>,
_releasesByPath: Record<string, Release>
): Promise<Record<string, Strategy>> {
// Using preconfigure to siphon releases and strategies.
this.strategiesByPath = strategiesByPath;
this.releasesByPath = _releasesByPath;

return strategiesByPath;
}
}

enum SUPPORTED_RANGE_PREFIXES {
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export abstract class WorkspacePlugin<T> extends ManifestPlugin {
pkg
)}`
);
const newCandidate = this.newCandidate(pkg, updatedVersions);
const newCandidate = await this.newCandidate(pkg, updatedVersions);
if (newCandidatePaths.has(newCandidate.path)) {
this.logger.info(
`Already created new candidate for path: ${newCandidate.path}`
Expand Down Expand Up @@ -320,7 +320,7 @@ export abstract class WorkspacePlugin<T> extends ManifestPlugin {
protected abstract newCandidate(
pkg: T,
updatedVersions: VersionsMap
): CandidateReleasePullRequest;
): Promise<CandidateReleasePullRequest>;

/**
* Collect all packages being managed in this workspace.
Expand Down
16 changes: 8 additions & 8 deletions src/strategies/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import {Strategy, BuildReleaseOptions} from '../strategy';
import {Strategy, BuildReleaseOptions, BumpReleaseOptions} from '../strategy';
import {GitHub} from '../github';
import {VersioningStrategy} from '../versioning-strategy';
import {Repository} from '../repository';
Expand Down Expand Up @@ -254,19 +254,19 @@ export abstract class BaseStrategy implements Strategy {
commits: ConventionalCommit[],
latestRelease?: Release,
draft?: boolean,
labels: string[] = []
labels: string[] = [],
bumpOnlyOptions?: BumpReleaseOptions
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 needed some indicator to bypass all the early out conditions associated with no commits or changelog entries.

An alternate approach to this whole thing may be to generate all the PRs always, and then prune ones we don't want.

): Promise<ReleasePullRequest | undefined> {
const conventionalCommits = await this.postProcessCommits(commits);
this.logger.info(`Considering: ${conventionalCommits.length} commits`);
if (conventionalCommits.length === 0) {
if (!bumpOnlyOptions && conventionalCommits.length === 0) {
this.logger.info(`No commits for path: ${this.path}, skipping`);
return undefined;
}

const newVersion = await this.buildNewVersion(
conventionalCommits,
latestRelease
);
const newVersion =
bumpOnlyOptions?.newVersion ??
(await this.buildNewVersion(conventionalCommits, latestRelease));
const versionsMap = await this.updateVersionsMap(
await this.buildVersionsMap(conventionalCommits),
conventionalCommits,
Expand Down Expand Up @@ -302,7 +302,7 @@ export abstract class BaseStrategy implements Strategy {
latestRelease,
commits
);
if (this.changelogEmpty(releaseNotesBody)) {
if (!bumpOnlyOptions && this.changelogEmpty(releaseNotesBody)) {
this.logger.info(
`No user facing commits found since ${
latestRelease ? latestRelease.sha : 'beginning of time'
Expand Down
4 changes: 3 additions & 1 deletion src/strategies/java.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {DEFAULT_SNAPSHOT_LABELS} from '../manifest';
import {JavaReleased} from '../updaters/java/java-released';
import {mergeUpdates} from '../updaters/composite';
import {logger as defaultLogger} from '../util/logger';
import {BumpReleaseOptions} from '../strategy';

const CHANGELOG_SECTIONS = [
{type: 'feat', section: 'Features'},
Expand Down Expand Up @@ -77,7 +78,8 @@ export class Java extends BaseStrategy {
commits: ConventionalCommit[],
latestRelease?: Release,
draft?: boolean,
labels: string[] = []
labels: string[] = [],
_bumpOnlyOptions?: BumpReleaseOptions
): Promise<ReleasePullRequest | undefined> {
if (await this.needsSnapshot(commits, latestRelease)) {
this.logger.info('Repository needs a snapshot bump.');
Expand Down
6 changes: 4 additions & 2 deletions src/strategies/php-yoshi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {BranchName} from '../util/branch-name';
import {PullRequestBody} from '../util/pull-request-body';
import {GitHubFileContents} from '@google-automations/git-file-utils';
import {FileNotFoundError} from '../errors';
import {BumpReleaseOptions} from '../strategy';

const CHANGELOG_SECTIONS = [
{type: 'feat', section: 'Features'},
Expand Down Expand Up @@ -68,12 +69,13 @@ export class PHPYoshi extends BaseStrategy {
commits: Commit[],
latestRelease?: Release,
draft?: boolean,
labels: string[] = []
labels: string[] = [],
bumpOnlyOptions?: BumpReleaseOptions
): Promise<ReleasePullRequest | undefined> {
const conventionalCommits = await this.postProcessCommits(
parseConventionalCommits(commits, this.logger)
);
if (conventionalCommits.length === 0) {
if (!bumpOnlyOptions && conventionalCommits.length === 0) {
this.logger.info(`No commits for path: ${this.path}, skipping`);
return undefined;
}
Expand Down
7 changes: 6 additions & 1 deletion src/strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ export interface BuildReleaseOptions {
groupPullRequestTitlePattern?: string;
}

export interface BumpReleaseOptions {
newVersion: Version;
}

/**
* A strategy is responsible for determining which files are
* necessary to update in a release pull request.
Expand All @@ -47,7 +51,8 @@ export interface Strategy {
commits: Commit[],
latestRelease?: Release,
draft?: boolean,
labels?: string[]
labels?: string[],
bumpOnlyOptions?: BumpReleaseOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit unclear on what these options are for. Perhaps a docstring is good enough to describe what it's use is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will add some documentation.

I wasn't 100% sure on this, but basically these serve as an indicator that you are creating a release with a version number update, even if there were no associated conventional commits.

So it is:
A: An indicator that we want to make a release even though there are no calculated changes.
B: The version number we want for that release.

): Promise<ReleasePullRequest | undefined>;

/**
Expand Down