-
Notifications
You must be signed in to change notification settings - Fork 349
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
fix: Use strategies for workspace plugins. #2112
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
const strategy = this.strategiesByPath[updatedPackage.location]; | ||
const latestRelease = this.releasesByPath[updatedPackage.location]; | ||
|
||
const basePullRequest = strategy |
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.
Make a base pull request that contains the "extra-files" updaters as well as the changelog header.
@@ -254,19 +254,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 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.
src/plugins/node-workspace.ts
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
For #2109
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.
Thank you for this and apologies for the late review.
In general I think this is a fine approach, although I don't fully understand the edge cases you are trying to handle. Test case would demonstrate the primary scenario it's fixing and prevent regressions in the future. Test cases would also demonstrate the edge cases you're trying to handle.
@@ -47,7 +51,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 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.
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.
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.
Thank you @chingor13, Now that I know the general approach is fine I will add some test cases that make this more clear. The situation is not really an edge case. If you have any workspace dependent packages, then it doesn't generate change logs correctly or update files with version numbers. Imagine you have two packages. Package A and package B. Package B depends on package A. You commit a change which affects files in package A: "feat: Add my feature." The release PR that gets generated will have an incorrect changelog. It will just append a "dependencies" section to the top of the changelog. The second major problem is that if package B had any In the PR I used as an example the Both of these are because version bumps only from dependencies do not run any of the steps associated with the base strategy being used. Thank you, |
…elease-please into rlamb/fix-workspace-base-pr
|
||
<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) |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
This demonstrates the correct header line being added to the changelog.
__snapshots__/node-workspace.js
Outdated
` | ||
|
||
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 comment
The 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.
expect(nodeCandidate).to.not.be.undefined; | ||
const updates = nodeCandidate!.pullRequest.updates; | ||
|
||
assertHasUpdate(updates, 'node2/my-file', Generic); |
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.
Without invoking the base strategy we would not have the update for extra-files.
I've added some additional doc comments, added tests, and made some comments in the PR for those tests. In this PR I am only focusing on node, but I believe that these same problems likely affect the other workspace plugins. |
Resolved by using updateCandidate after making the base request. |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1978, #2089 and maybe #2109 🦕
The main goal here is to generate a base pull request using the strategy defined for the release. Then add to that the updates that are needed specifically to the workspace. This will result in generating the correct changelog header as well as updating "extra-files".
I need to implement tests for this yet, but I wanted to get some opinions on the approach before piling too much code on. I've only made changes for the node workspace, but the concept could be extended to to the other workspace plugins.
Example pr created with these changes showing fixes for #1978 #2089:
https://github.com/kinyoklion/release-please-monorepo-tests/pull/10/files