-
Notifications
You must be signed in to change notification settings - Fork 379
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
feat!: support stable release branch names #720
Conversation
Codecov Report
@@ Coverage Diff @@
## master #720 +/- ##
==========================================
- Coverage 87.13% 86.72% -0.41%
==========================================
Files 55 56 +1
Lines 6683 7089 +406
Branches 616 662 +46
==========================================
+ Hits 5823 6148 +325
- Misses 859 940 +81
Partials 1 1
Continue to review full report at Codecov.
|
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.
looking great to me so far, I like that you've broken work out the branch handling into its own helper.
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.
Left some initial comments and suggestions.
I like the direction things are going in, a couple things that jumped out at me:
- it seems like we'd want the lookup of
latestTag
and "commits since last release" to handle the stable branch names too -- unless I'm missing something. - I'd love to make sure we move some of the other releasers over to the new branch naming approach ASAP, once we've addressed your immediate need.
strictEqual(created!.tag_name, 'v1.0.3'); | ||
requests.done(); | ||
expect(created).to.not.be.undefined; | ||
strictEqual(created!.tag_name, 'foo-v1.0.3'); | ||
}); | ||
|
||
it('attempts to guess package name for release', async () => { |
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 might make this test case more generic, something like:
it('falls back to lookupPackageName() if no package name provided')
); | ||
snapshot(latestReleaseNotes); | ||
}); | ||
it('parses version from PR title (detectReleaseVersionFromTitle impl: base)', async () => { |
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 seems like a nice simplification over some other approaches we've discussed, e.g., loading the version form package.json
or setup.py
.
mergedPR: MergedGitHubPR, | ||
branchName: BranchName | undefined | ||
): Promise<string | undefined> { | ||
// try from branch name |
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 like where you've landed with this functionality. Perhaps we could add one test that explicitly calls out order of operation, i.e., "it('uses version number in branch name over version number in title')".
// Override this method to use static branch names | ||
// If you modify this, you must ensure that the releaser can parse the tag version | ||
// from the pull request. | ||
protected async buildBranchName( |
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.
Am I following that each language would opt in to this new functionality? Is the reasoning to allow for a gradual upgrade and testing process?
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, this allows each releaser to choose its strategy for branch naming and will allow gradual upgrades. This is protected
so it's not part of the public surface area. If we're happy with using release-please/branches/[branch]/(components/[component])?
globally, then we can refactor this to be done in the base class without breaking anyone.
}; | ||
}); | ||
} | ||
|
||
// The default matcher will rule out pre-releases. | ||
// TODO: make this handle more than 100 results using async iterator. | ||
async findMergedReleasePR( |
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.
Is this dead code now that you've pulled findMergedRelease
into release-pr
?
@@ -39,9 +38,7 @@ export class Node extends ReleasePR { | |||
if (pkg.name) this.packageName = pkg.name; | |||
|
|||
const latestTag: GitHubTag | undefined = await this.gh.latestTag( |
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.
In the cases of releasers with a stable branch name, should latestTag
also be looking up the stable name first, and falling back to iterating over findMergedReleasePR
?
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 was an oversight and is missing in this PR. Finding commits since the last release also needs to be able to find the stable release on the branch.
this.monorepoTags | ||
? `${packageBranchPrefix(this.packageName, 'node')}-` | ||
: undefined | ||
this.monorepoTags ? `${this.packagePrefix}-` : 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.
Wasn't immediately following why we can drop the helper packageBranchPrefix
here.
Also, if the goal is hopefully to move most the releasers over to a stable branch name, I wonder if the idea of a prefix
makes sense, should we just be passing in the full branch name?
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.
It's been appropriately moved over into a new ReleasePR.coercePackagePrefix
- I like that change! However, see my note on the tests as I think it's not quite all the way there yet for Node.
also, in the spirit of "Small CLs" that change could probably have been pulled out as a separate PR ahead of this change.
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.
Nice work Jeff! Really beautifully written code here and some very common sense refactors. I do have some thoughts:
- There's a lot going on in this PR. Is it possible to tease some things apart to submit separately? Maybe something like
packageBranchPrefix
->coercePackagePrefix
change- the switch to using graphql for finding the last merge release if that's the way we decide to go
- adding the
BranchName
class and tests w/out wiring them up yet - wiring up
BranchName
usage? - custom PR body and title
Smaller changes are easier to reason about and catch bugs in :-)
- I'm not sure how I feel about the proliferation of methods for finding a version. On the one hand flexibility gives users more options. On the other hand, it creates higher risk for bugs and might make it harder to keep straight. The aggregate releaser is already going to add another version storing method: the manifest. I might even present an argument to you folks that all release-please version tracking should be migrated to that file (the structure of which might need to be updated to scoped by targetReleaseBranch / LTS branch).
src/github.ts
Outdated
ref(qualifiedName: $targetBranch) { | ||
target { | ||
... on Commit { | ||
history(first: $num) { |
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.
a side effect of this approach to finding PRs is that, because the top level is commit history on the target branch, several entries could point to the same associated PR (i.e. when folks do a regular merge rather than a squash merge). This further exacerbates the problem of actually finding the last merged release pr in the past $num
commits on the target branch.
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 probably need to do some testing.
If you merge or rebase/merge and all the commits are associated to the same PR, this method could return duplicates and could fill the entire page(s) of responses. We would likely still need to paginate the graphql query as well.
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 wonder if rather than paginating, there's a sort we can apply on the lookup, so the most recent interaction with the well known branch name is at the top? we should be able to test this in the sandbox.
src/github.ts
Outdated
async findMergedPullRequests(number?: number): Promise<MergedGitHubPR[]> { | ||
const targetBranch = await this.getDefaultBranch(); | ||
const response = await this.graphqlRequest({ | ||
query: `query findMergedPullRequests($owner: String!, $repo: String!, $num: Int!) { |
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.
Why did you move away from the REST endpoint? Unless I'm missing something I don't see that this query is providing any extra data we weren't getting from /repos/pulls
. I understand the motivation when a single REST call won't get you all the data you need but I wonder if there's a tradeoff given the complexity of a graphql query? If the REST endpoint does the job, and is a simpler interface to understand, then I think we should document why/when we use graphql queries. Or maybe we decide we want to default to graphql going forward? but given the current mix and reading (this comment)[https://github.com/googleapis/release-please/blob/f3a28f3eb98a32ce101a1b76f4e9299a8e6b4c5d/src/github.ts#L246] it seems like the preference till now has been for REST endpoints when reasonable?
If there is strong motivation to use graphql for finding "the last merged release PR" then I still think our top level query should be for PRs rather than commit history on the target branch. something like:
{
repository(owner: $owner, name: $repo) {
pullRequests(baseRefName: $targetBranch, states: MERGED, orderBy: {field: CREATED_AT, direction: DESC}, first: $num) {
nodes {
title
headRefName
.
.
.
mergeCommit {
oid
}
}
}
}
}
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 was using this only for detecting the release PR/commit after the merge was completed and we're looking for the commit on the branch to tag (which in theory should be very close to head of the branch).
@bcoe pointed out that I missed the logic for fetching the commits since the last release on a target branch would also need to check the new branch names (which this PR is not yet doing).
For some reason, I thought that the REST endpoint couldn't filter by base branch, but it totally can. I will switch back to the REST endpoint
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.
Ah, I remember additional context - it fixes the ordering. Currently, we're sorting pull requests by created
which is when the PR was opened. If a release PR has sat open for a long time with many PRs open, it may be buried in the list of closed pull requests and could require much paging to find. Coming from the commit side would give you the history order or when it's merged.
It might be possible to switch to updated
for the sort order, but still a PR being modified after merge might affect this field too.
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.
detecting the release PR/commit after the merge was completed and we're looking for the commit on the branch to tag
I actually think there should be one and only one way of finding this - whether it's for creating a release/tag or for creating/updating the next Release PR. The question is the same regardless of why it's being asked: "what is the commit on targetReleaseBranch that represents the last release" and I think we need to stick with the definition of "it's the last-merged-release-pr's merge commit". I think there should be a single github.ts
method for this that has these inputs:
- targetReleaseBranch (defaultBranch/LTS branch)
- pattern for releasePRBranch name (constructed from your BranchName stuff with a regex placeholder for version if it's a
AutoreleaseBranchName
branch)
and finds the last merged PR given the above combo.
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.
The bug Ben pointed out is exactly this. The 2 need to use the same logic.
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 remember an additional reason why I was using commits to the branch to find the pull requests. The base
ref of the associated pull request may not have been the release branch we're looking for.
Consider 2.0.0, 2.1.0 are both released from main
. We now create a 2.0.x
branch. To find the latest release on the 2.0.x
branch, we need to consider any release pull request in history (the 2.0.0
release PR was against main
, not the 2.0.x
branch - the base
ref is not important).
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.
@chingor13 I want to make sure I understand what you're saying here:
Were versions "2.0.0"
and "2.1.0"
releases both release-please PRs that were merged to main
? How does branch 2.0.x
come into existence? Is it manually created by branching off main
? Ah, I think I get it. The problem is "bootstrapping" the initial version on the release branch, right? I.e. when release-please is configured for the first time with defaultBranch
set to 2.0.x
it won't find the last release/version? I think we can solve this with the manifest file (thoughts forthcoming...) but till then does the release-as
footer suffice as a work-around for that first release on the LTS branch?
protected async buildPullRequestTitle( | ||
version: string, | ||
includePackageName: boolean | ||
): Promise<string> { | ||
const defaultBranch = await this.getDefaultBranch(); | ||
return includePackageName | ||
? `chore(${defaultBranch}): release ${this.packageName} ${version}` | ||
: `chore(${defaultBranch}): release ${version}`; | ||
} | ||
|
||
// Override this method to detect the release version from code (if it cannot be | ||
// inferred from the release PR head branch) | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
protected detectReleaseVersionFromTitle(title: string): string | undefined { | ||
const pattern = /^chore\((?<branch>[^(]+)\): release ?(?<component>.*) (?<version>\d+\.\d+\.\d+)$/; | ||
const match = title.match(pattern); | ||
if (match?.groups) { | ||
return match.groups['version']; | ||
} | ||
return 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.
It seems like we could DRY this out and perhaps formalize PR title building a bit more. The only difference between these and the parent is including the defaultBranch
in the title. Maybe the parent methods are augmented with a includeTargetBranchName
option/flag?
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 also considered making a PullRequestTitle
resource name-like class similar to the BranchName
that knows how to parse and build itself.
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 kinda like that :-)
// If you modify this, you must ensure that the releaser can parse the tag version | ||
// from the pull request. | ||
protected async buildBranchName( | ||
version: string, |
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.
minor head-scratcher for me if anyone has any ideas: my editors show me Info: tsserver: 'version' is declared but its value is never read.
I can't quite parse all the gts
+ @typescript-eslint/eslint-plugin etc. I'll ignore it for now since npm run lint
doesn't flag it but I've seen other projects follow the convention/solution of prefixing with an _
to silence the message. But if anyone else sees this as well then it might be worth investigating what's going on (some global setting in my dev env?).
if (!branchNameClass) { | ||
return undefined; | ||
} | ||
return new branchNameClass(branchName); |
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 really like this BranchName
implementation, nice work!
One small nit: It seems like BranchName.constructor
duplicates the work done by BranchName.matches
.
If BranchName.parse
is the factory entry point that will always get used, then the constructor could be changed to something like constructor(matches: RegExpMatchArray)
. BranchName.matches
would return the match array and BranchName.parse
updated accordingly to instantiate a clazz with that output?
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 was running into typescript issues, but I agree that it's not ideal duplicating the regex match. I will try this again.
@@ -289,4 +289,31 @@ describe('Node', () => { | |||
req.done(); | |||
}); | |||
}); | |||
|
|||
describe('coercePackagePrefix', () => { |
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 think we need a test where packageName
is not provided to new Node({})
and that releasePR.packagePrefix
is set correctly after releasePR._run
is invoked since that's where the Node releaser currently sets it
note: that might not be the best place for a releaser to set it but since it comes from inspecting source code, any usage of this.packagePrefix
needs to wait till the releaser has had a chance to look it up dynamically.
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.
As long as we don't break the existing logic, I think it's shaken out both in this PR and in #725, that the fallback packageName
lookup should be refactored a bit.
I wonder if we could pull it up to releasea-pr.ts
instead, and have all releasers attempt the lookup., if it's supported by their language.
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.
actually, I think this is still broken for node as setting this.packagePrefix
only happens in the constructor on this branch but this.packageName
gets reset in Node._run
...
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, looks like this will cause the PR to be opened with the wrong packagePrefix
this.monorepoTags | ||
? `${packageBranchPrefix(this.packageName, 'node')}-` | ||
: undefined | ||
this.monorepoTags ? `${this.packagePrefix}-` : 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.
It's been appropriately moved over into a new ReleasePR.coercePackagePrefix
- I like that change! However, see my note on the tests as I think it's not quite all the way there yet for Node.
also, in the spirit of "Small CLs" that change could probably have been pulled out as a separate PR ahead of this change.
// Override this method to detect the release version from code (if it cannot be | ||
// inferred from the release PR head branch) | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
protected detectReleaseVersionFromTitle(title: string): string | 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.
@bcoe I recall your aversion to the instability of labels to store the version - I'd argue that the PR title is just as fragile - it could be changed after the last release-please run but before merging. It can also be changed after the PR has already been merged
@chingor13 is this here in support of keeping the version name out of the branch? If so, i think we should stick with just the manifest approach - I promise I'll have that up for review soon!
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 is fallback to support versionless branch names (until we implement the code detection).
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.
do we need versionless branch names right now? i.e. is there a need outside the aggregate PR to keep version names out of branches?
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.
One of our Java repos has a edge case need for this.
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.
If this unblocks @chingor13's work on Java, I'm tempted to run with this approach for the time being.
An additional concern I had with labels, is that our forking GitHub actions have permission to open PRs, but not to label. So using labels had an additional permission headache.
I like that the lookup logic has been encapsulated and tested.
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.
fair enough. I would like to understand the java edge case though for my own edification - we have a kotlin package we'll need to release as part our aggregate. I know nothing about how that all works right now other than it has (or will have) java interop so we need it to be available to kotlin users as well as java users.
protected async detectReleaseVersionFromCode(): Promise<string | undefined> { | ||
return 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.
I don't think we're ready to expose this yet, or even if it's a good idea at all
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 would be the hook for looking up the version(s) from the manifest.
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.
ah, I interpreted FromCode
to mean per-type specific source code (e.g. version.py, package.json)
I'd prefer to wait on this change till we have something actually ready to implement it. I'd actually like to propose that we use a eventually only use a manifest file for tracking versions. I'd add it as part of the aggregate PR work, providing a migration path that seamlessly switches from encoding the version in the branch name to writing it to the manifest.
For now I feel like we should stick with the current functionality of version-in-branch-name
@joeldodge79 I'm a little concerned about this too, I think @chingor13's work to address the |
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.
looking good! one more suggestion for ya and a question about the java edge case for not using version in branch name.
src/github.ts
Outdated
interface MergedPullRequestFilter { | ||
(filter: MergedGitHubPR): boolean; | ||
} | ||
|
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.
interesting, I didn't know you could do it this way. Honestly it's not totally intuitive to me (I'd be looking for a class instance/object that has a method on it named... but wait, there's no method name defined in the interface, wait wut?)
somehow type MergedPullRequestFilter = (filter: MergedGitHubPR) => boolean;
reads more intelligibly to me. Also, in my IDE, hovering over MergedPullRequestFilter
as written tells me
interface MergedPullRequestFilter
which is a little opaque. With my suggestion it tells me: type MergedPullRequestFilter = (filter: MergedGitHubPR) => boolean
which is a little more informative
But I don't feel strongly either way. This whole comment is mostly just an exercise for me in learning more about how TS does types :-)
EDIT: if you go with my /repos/pulls?head=
optimization suggestion then ignore everything I said here and the interface probably changes to something like
interface MergedPullRequestFilter { | |
(filter: MergedGitHubPR): boolean; | |
} | |
interface MergedPullRequestFilter { | |
filter(mergedPR: MergedGitHubPR): boolean; | |
exactReleaseBranchName?: string | |
} | |
async findMergedPullRequest( | ||
targetBranch: string, | ||
filter: MergedPullRequestFilter | ||
): Promise<MergedGitHubPR | undefined> { | ||
let page = 1; | ||
let mergedPullRequests = await this.findMergedPullRequests( | ||
targetBranch, | ||
page | ||
); | ||
while (mergedPullRequests.length > 0) { | ||
const found = mergedPullRequests.find(filter); | ||
if (found) { | ||
return found; | ||
} | ||
page += 1; | ||
mergedPullRequests = await this.findMergedPullRequests( | ||
targetBranch, | ||
page | ||
); | ||
} | ||
return 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.
cool, I like the filter
and the recursive pagination.
I think there's one more optimization to be had somewhere between findMergedPullRequest
and findMergedPullRequests
- the case of a stable branch name (i.e. no version) the /repos/pulls
resource takes a head
argument that can be formatted a few different ways but here's how I did it when hacking on this. It has to be an exact match (no wildcards supported). Maybe you could augment the MergedPullRequestFilter
interface with an releaseBranchName?: string
property and use that to switch between passing in a head
param to findMergedPullRequests
or just calling the filter callback on the results? (in that case ignore my function typing musings above on how you've declared it: it would need to be an interface
with a proper filter
method name)
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 think we can skip the optimization on filtering the exact release branch name until we migrate all releasers to start using the stable branch names -- we need to support the fallback to the old, versioned style during the migration period.
protected async buildPullRequestTitle( | ||
version: string, | ||
includePackageName: boolean | ||
): Promise<string> { | ||
const defaultBranch = await this.getDefaultBranch(); | ||
return includePackageName | ||
? `chore(${defaultBranch}): release ${this.packageName} ${version}` | ||
: `chore(${defaultBranch}): release ${version}`; | ||
} | ||
|
||
// Override this method to detect the release version from code (if it cannot be | ||
// inferred from the release PR head branch) | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
protected detectReleaseVersionFromTitle(title: string): string | undefined { | ||
const pattern = /^chore\((?<branch>[^(]+)\): release ?(?<component>.*) (?<version>\d+\.\d+\.\d+)$/; | ||
const match = title.match(pattern); | ||
if (match?.groups) { | ||
return match.groups['version']; | ||
} | ||
return 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.
I kinda like that :-)
// Override this method to detect the release version from code (if it cannot be | ||
// inferred from the release PR head branch) | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
protected detectReleaseVersionFromTitle(title: string): string | 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.
fair enough. I would like to understand the java edge case though for my own edification - we have a kotlin package we'll need to release as part our aggregate. I know nothing about how that all works right now other than it has (or will have) java interop so we need it to be available to kotlin users as well as java users.
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.
LGTM 🌲
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 is looking good to me, if you're feeling it's ready to land?
I will test the finished state on a few repositories I manage.
@chingor13 would you be able to run the CLI against a testing Java repo, to make sure that the behavior is as expected?
For Java, it's opening pull requests to the right branches chingor13/java-bigtable-hbase#9 (commits are weird in this one because there's no releases to master since the code was migrated here), chingor13/java-bigtable-hbase#10 |
Forming the contents of a release was factored out of GitHubRelease in googleapis#720 The argument provide for the release name was accidentally swapped to be the packagePrefix (e.g. sans @scope for node packages).
Forming the contents of a release was factored out of GitHubRelease in googleapis#720 The argument provide for the release name was accidentally swapped to be the packagePrefix (e.g. sans @scope for node packages).
Forming the contents of a release was factored out of GitHubRelease in googleapis#720 The argument provide for the release name was accidentally swapped to be the packagePrefix (e.g. sans @scope for node packages).
Forming the contents of a release was factored out of GitHubRelease in googleapis#720 The argument provide for the release name was accidentally swapped to be the packagePrefix (e.g. sans @scope for node packages).
Forming the contents of a release was factored out of GitHubRelease in googleapis#720 The argument provide for the release name was accidentally swapped to be the packagePrefix (e.g. sans @scope for node packages).
Forming the contents of a release was factored out of GitHubRelease in #720 The argument provide for the release name was accidentally swapped to be the packagePrefix (e.g. sans `@scope/` for node packages).
Fixes #718
Fixes #719
feat: add
GitHub#findMergedPullRequest
to filter through merged pull requests with paginationBREAKING CHANGE:
removed per page parameter from
GitHub#findMergedReleasePR
and moved some internal helpers