-
Notifications
You must be signed in to change notification settings - Fork 369
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
Changes from 10 commits
277821b
39a2415
6f9af54
5648538
cc25227
f9b38c1
3707bb8
0d5d844
cd9666f
9ca04c5
c8a9503
8459bc8
c8e1e15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,6 +138,84 @@ Release notes for path: node1, releaseType: node | |
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). | ||
` | ||
|
||
exports['NodeWorkspace plugin run includes headers for packages with configured strategies 1'] = ` | ||
:robot: I have created a release *beep* *boop* | ||
--- | ||
|
||
|
||
<details><summary>@here/pkgA: 3.3.4</summary> | ||
|
||
Release notes for path: node1, releaseType: node | ||
</details> | ||
|
||
<details><summary>@here/pkgB: 2.2.3</summary> | ||
|
||
## [2.2.3](https://github.com/googleapis/node-test-repo/compare/pkgB-v2.2.2...pkgB-v2.2.3) (1983-10-10) | ||
|
||
|
||
### Dependencies | ||
|
||
* The following workspace dependencies were updated | ||
* dependencies | ||
* @here/pkgA bumped from 3.3.3 to 3.3.4 | ||
</details> | ||
|
||
<details><summary>@here/pkgC: 1.1.2</summary> | ||
|
||
## 1.1.2 (1983-10-10) | ||
|
||
|
||
### Dependencies | ||
|
||
* The following workspace dependencies were updated | ||
* dependencies | ||
* @here/pkgB bumped from 2.2.2 to 2.2.3 | ||
</details> | ||
|
||
<details><summary>@here/pkgE: 1.0.1</summary> | ||
|
||
## 1.0.1 (1983-10-10) | ||
|
||
|
||
### Dependencies | ||
|
||
* The following workspace dependencies were updated | ||
* dependencies | ||
* @here/pkgA bumped to 3.3.4 | ||
</details> | ||
|
||
--- | ||
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). | ||
` | ||
|
||
exports['NodeWorkspace plugin run includes headers for packages with configured strategies 2'] = ` | ||
other notes | ||
` | ||
|
||
exports['NodeWorkspace plugin run includes headers for packages with configured strategies 3'] = ` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This demonstrates the correct header line being added to the changelog. |
||
## [2.2.3](https://github.com/googleapis/node-test-repo/compare/pkgB-v2.2.2...pkgB-v2.2.3) (2023-12-15) | ||
` | ||
|
||
exports['NodeWorkspace plugin run includes headers for packages with configured strategies 4'] = ` | ||
### Dependencies | ||
|
||
* The following workspace dependencies were updated | ||
* dependencies | ||
* @here/pkgA bumped from 3.3.3 to 3.3.4 | ||
` | ||
|
||
exports['NodeWorkspace plugin run includes headers for packages with configured strategies 5'] = ` | ||
## 1.1.2 (2023-12-15) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also would not have been present. This doesn't include a diff, because it didn't have the previous version information included. |
||
` | ||
|
||
exports['NodeWorkspace plugin run includes headers for packages with configured strategies 6'] = ` | ||
### Dependencies | ||
|
||
* The following workspace dependencies were updated | ||
* dependencies | ||
* @here/pkgB bumped from 2.2.2 to 2.2.3 | ||
` | ||
|
||
exports['NodeWorkspace plugin run respects version prefix 1'] = ` | ||
{ | ||
"name": "@here/plugin1", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,9 @@ import { | |
addPath, | ||
} from './workspace'; | ||
import {PatchVersionUpdate} from '../versioning-strategy'; | ||
import {Strategy} from '../strategy'; | ||
import {Commit} from '../commit'; | ||
import {Release} from '../release'; | ||
import {CompositeUpdater} from '../updaters/composite'; | ||
import {PackageJson, newVersionWithRange} from '../updaters/node/package-json'; | ||
import {Logger} from '../util/logger'; | ||
|
@@ -67,6 +70,10 @@ interface NodeWorkspaceOptions extends WorkspacePluginOptions { | |
*/ | ||
export class NodeWorkspace extends WorkspacePlugin<Package> { | ||
private alwaysLinkLocal: boolean; | ||
|
||
private strategiesByPath: Record<string, Strategy> = {}; | ||
private releasesByPath: Record<string, Release> = {}; | ||
|
||
readonly updatePeerDependencies: boolean; | ||
constructor( | ||
github: GitHub, | ||
|
@@ -230,10 +237,10 @@ export class NodeWorkspace extends WorkspacePlugin<Package> { | |
} | ||
return existingCandidate; | ||
} | ||
protected newCandidate( | ||
protected async newCandidate( | ||
pkg: Package, | ||
updatedVersions: VersionsMap | ||
): CandidateReleasePullRequest { | ||
): Promise<CandidateReleasePullRequest> { | ||
// Update version of the package | ||
const newVersion = updatedVersions.get(pkg.name); | ||
if (!newVersion) { | ||
|
@@ -250,20 +257,31 @@ export class NodeWorkspace extends WorkspacePlugin<Package> { | |
updatedVersions, | ||
this.logger | ||
); | ||
|
||
const strategy = this.strategiesByPath[updatedPackage.path]; | ||
const latestRelease = this.releasesByPath[updatedPackage.path]; | ||
|
||
const basePullRequest = strategy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
}) | ||
: undefined; | ||
|
||
const pullRequest: ReleasePullRequest = { | ||
title: PullRequestTitle.ofTargetBranch(this.targetBranch), | ||
body: new PullRequestBody([ | ||
{ | ||
component: updatedPackage.name, | ||
version: newVersion, | ||
notes: appendDependenciesSectionToChangelog( | ||
'', | ||
basePullRequest?.body.notes() ?? '', | ||
dependencyNotes, | ||
this.logger | ||
), | ||
}, | ||
]), | ||
updates: [ | ||
...(basePullRequest ? basePullRequest?.updates : []), | ||
{ | ||
path: addPath(updatedPackage.path, 'package.json'), | ||
createIfMissing: false, | ||
|
@@ -277,6 +295,9 @@ export class NodeWorkspace extends WorkspacePlugin<Package> { | |
createIfMissing: false, | ||
updater: new Changelog({ | ||
version: newVersion, | ||
versionHeaderRegex: `\n###? v?[${newVersion | ||
.toString() | ||
.replace('.', '.')}]`, | ||
changelogEntry: appendDependenciesSectionToChangelog( | ||
'', | ||
dependencyNotes, | ||
|
@@ -286,7 +307,9 @@ export class NodeWorkspace extends WorkspacePlugin<Package> { | |
}, | ||
], | ||
labels: [], | ||
headRefName: BranchName.ofTargetBranch(this.targetBranch).toString(), | ||
headRefName: | ||
basePullRequest?.headRefName ?? | ||
BranchName.ofTargetBranch(this.targetBranch).toString(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For #2109 |
||
version: newVersion, | ||
draft: false, | ||
}; | ||
|
@@ -350,6 +373,18 @@ export class NodeWorkspace extends WorkspacePlugin<Package> { | |
: {}), | ||
}; | ||
} | ||
|
||
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; | ||
} | ||
} | ||
|
||
function getChangelogDepsNotes( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -261,19 +261,19 @@ export abstract class BaseStrategy implements Strategy { | |
commits: ConventionalCommit[], | ||
latestRelease?: Release, | ||
draft?: boolean, | ||
labels: string[] = [] | ||
labels: string[] = [], | ||
bumpOnlyOptions?: BumpReleaseOptions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -309,7 +309,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' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -39,6 +43,10 @@ export interface Strategy { | |
* component if available. | ||
* @param {boolean} draft Optional. Whether or not to create the pull | ||
* request as a draft. Defaults to `false`. | ||
* @param {BumpReleaseOptions} bumpOnlyOptions Optional. Options, that when | ||
* present, indicate a release should be created even if there are no | ||
* conventional commits. This is used when a release is required for | ||
* a dependency update with a workspace plugin. | ||
* @returns {ReleasePullRequest | undefined} The release pull request to | ||
* open for this path/component. Returns undefined if we should not | ||
* open a pull request. | ||
|
@@ -47,7 +55,8 @@ export interface Strategy { | |
commits: Commit[], | ||
latestRelease?: Release, | ||
draft?: boolean, | ||
labels?: string[] | ||
labels?: string[], | ||
bumpOnlyOptions?: BumpReleaseOptions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
): Promise<ReleasePullRequest | undefined>; | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line here represents one of the primary differences. Without this change, there would be no header line here. Both in the PR and in the Changelog.