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

feat!: support stable release branch names #720

Merged
merged 25 commits into from Feb 3, 2021

Conversation

chingor13
Copy link
Contributor

@chingor13 chingor13 commented Jan 26, 2021

Fixes #718
Fixes #719

  • Adds support for building/parsing release branch names (language agnostic feature, but the languages choose an implementation)
  • Moves GitHub release/tag logic into the individual releasers and will allow us to override release behavior on a language-by-language basis.
  • Moves release note functionality into separate module (language agnostic feature)
  • Migrates java-yoshi releaser to use stable branch names (all other languages should maintain their exact same behavior)

feat: add GitHub#findMergedPullRequest to filter through merged pull requests with pagination

BREAKING CHANGE:
removed per page parameter from GitHub#findMergedReleasePR and moved some internal helpers

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 26, 2021
@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #720 (efcca2e) into master (63b4f6e) will decrease coverage by 0.40%.
The diff coverage is 94.13%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/util/release-notes.ts 89.65% <84.61%> (ø)
src/releasers/java-yoshi.ts 87.38% <86.04%> (-0.20%) ⬇️
src/util/branch-name.ts 88.88% <88.88%> (ø)
src/github-release.ts 97.28% <93.33%> (+1.00%) ⬆️
src/github.ts 81.30% <95.06%> (-3.70%) ⬇️
src/release-pr.ts 93.43% <98.95%> (+2.76%) ⬆️
src/releasers/go-yoshi.ts 93.03% <100.00%> (ø)
src/releasers/node.ts 94.77% <100.00%> (+0.32%) ⬆️
src/releasers/ruby.ts 35.53% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b4f6e...3d76bae. Read the comment docs.

Copy link
Contributor

@bcoe bcoe left a 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.

src/util/branch-name.ts Outdated Show resolved Hide resolved
@chingor13 chingor13 marked this pull request as ready for review January 28, 2021 22:25
@chingor13 chingor13 requested a review from a team as a code owner January 28, 2021 22:25
@chingor13 chingor13 requested a review from bcoe January 28, 2021 22:29
src/github-release.ts Show resolved Hide resolved
src/releasers/go-yoshi.ts Show resolved Hide resolved
src/releasers/ruby.ts Show resolved Hide resolved
src/util/release-notes.ts Show resolved Hide resolved
src/release-pr.ts Show resolved Hide resolved
Copy link
Contributor

@bcoe bcoe left a 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:

  1. 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.
  2. 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 () => {
Copy link
Contributor

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 () => {
Copy link
Contributor

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
Copy link
Contributor

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')".

src/release-pr.ts Show resolved Hide resolved
// 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(
Copy link
Contributor

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?

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, 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(
Copy link
Contributor

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(
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Collaborator

@joeldodge79 joeldodge79 left a 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:

  1. 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 :-)
  1. 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 Show resolved Hide resolved
src/github.ts Outdated
ref(qualifiedName: $targetBranch) {
target {
... on Commit {
history(first: $num) {
Copy link
Collaborator

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.

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 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.

Copy link
Contributor

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!) {
Copy link
Collaborator

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
        }
      }
    }
  }
}

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 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

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

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 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).

Copy link
Collaborator

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?

Comment on lines +312 to +332
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;
}
Copy link
Collaborator

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?

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 also considered making a PullRequestTitle resource name-like class similar to the BranchName that knows how to parse and build itself.

Copy link
Collaborator

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,
Copy link
Collaborator

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);
Copy link
Collaborator

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?

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 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', () => {
Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Collaborator

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...

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, 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
Copy link
Collaborator

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 {
Copy link
Collaborator

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!

Copy link
Contributor Author

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).

Copy link
Collaborator

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?

Copy link
Contributor Author

@chingor13 chingor13 Feb 1, 2021

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.

Copy link
Contributor

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.

Copy link
Collaborator

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.

Comment on lines +414 to +415
protected async detectReleaseVersionFromCode(): Promise<string | undefined> {
return undefined;
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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

@bcoe
Copy link
Contributor

bcoe commented Feb 1, 2021

I'm not sure how I feel about the proliferation of methods for finding a version.

@joeldodge79 I'm a little concerned about this too, I think @chingor13's work to address the latestTag bug will hopefully move things closer together.

Copy link
Collaborator

@joeldodge79 joeldodge79 left a 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
Comment on lines 65 to 68
interface MergedPullRequestFilter {
(filter: MergedGitHubPR): boolean;
}

Copy link
Collaborator

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

Suggested change
interface MergedPullRequestFilter {
(filter: MergedGitHubPR): boolean;
}
interface MergedPullRequestFilter {
filter(mergedPR: MergedGitHubPR): boolean;
exactReleaseBranchName?: string
}

Comment on lines +632 to +654
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;
}

Copy link
Collaborator

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)

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 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.

Comment on lines +312 to +332
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;
}
Copy link
Collaborator

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 {
Copy link
Collaborator

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.

Copy link
Collaborator

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

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

LGTM 🌲

Copy link
Contributor

@bcoe bcoe left a 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?

@chingor13
Copy link
Contributor Author

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

@chingor13 chingor13 changed the title feat: support stable release branch names feat!: support stable release branch names Feb 2, 2021
@bcoe bcoe merged commit 36cae96 into googleapis:master Feb 3, 2021
@chingor13 chingor13 deleted the stable-branch branch February 3, 2021 01:25
joeldodge79 added a commit to joeldodge79/release-please that referenced this pull request Feb 10, 2021
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).
joeldodge79 added a commit to joeldodge79/release-please that referenced this pull request Feb 10, 2021
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).
joeldodge79 added a commit to joeldodge79/release-please that referenced this pull request Feb 10, 2021
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).
joeldodge79 added a commit to joeldodge79/release-please that referenced this pull request Feb 11, 2021
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).
joeldodge79 added a commit to joeldodge79/release-please that referenced this pull request Feb 11, 2021
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).
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 11, 2021
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
3 participants