Release note tool should not ignore squashed PRs

[sbueringer]

The release note tool has the limitation that it ignores squashed PRs, which is why we always have to ask contributors to squash their commits before we can merge.

It would be nice to take a closer look if there is a way to get rid of this limitation.

I would avoid a lot of toil that we have when we have to ask for squashes.

To be clear, we could then use the tide/merge-method-squash label and tide would squash for us. Today this is not really possible as the PR then would not show up in the release notes.

[sbueringer]

cc @furkatgofurov7 @killianmuldoon

[sbueringer]

I think this would be a major improvement to devex if we can make it work. Maybe worth including in the improvements list for 1.6

[furkatgofurov7]

/triage accepted

Thanks, this is a great improvement, I personally was not aware of this limitation. Adding it to 1.6 improvements tasks

[furkatgofurov7]

cc @kubernetes-sigs/cluster-api-release-team

[mjlshen]

/assign

[killianmuldoon]

This PR is related to the issue: #9163

The tool now outputs a warning when it can't find the correct title for a commit to put in the notes. We haven't seen this in the CAPI repo, we noticed the issue in the CAPV repo initially.

[sbueringer]

Btw the case I'm referencing is not the one where a maintainer manually merges a PR. I'm referring to the situation we end up with if we use the Prow squash label (tide/merge-method-squash) and then tide squashes for us.

We don't have this case today because we know about the limitation of the release notes tool and thus don't use this label. We always ask folks to squash before we merge instead which is super annoying

[g-gaston]

@sbueringer
By any chance, do you remember what the issue was or why the release ignores these commits?

Or instead maybe, could you point us to a problematic commit so we can debug the tool against it?

[killianmuldoon]

@g-gaston There's info on this PR: #9163

If you run the tool as it's currently configured you should get warnings for these commits. I believe this commit: kubernetes-sigs/cluster-api-provider-vsphere#1819 is the example we have of tide/merge-method-squash interfering in the release note.

[sbueringer]

I'm not sure if that CAPV PR is a good example. According to GitHub the squash label was never applied.

I would suggest to just merge a not-super-important-for-release-notes PR in core CAPI with the squash method and then check if it shows up in the generated release notes (I'm pretty sure it won't)

[killianmuldoon]

Should be possible to test this on a fork which might be better.

[sbueringer]

If you have a way to run Prow on a fork - yup :)

[furkatgofurov7]

We set up a new prow instance for internal use recently and I can test it :). Can you give an example workflow, of what needs to be exactly tested?

[sbueringer]

Just remembered. We had a few squashed PRs in controller-runtime. If you run the release note tool on main with previous tag v0.15 there should be a few.

https://github.com/kubernetes-sigs/controller-runtime/pulls?q=is%3Apr+sort%3Aupdated-desc+is%3Aclosed+label%3Atide%2Fmerge-method-squash

e.g.: kubernetes-sigs/controller-runtime#2406

Workflow:

• add the squash label
• merge the PR (via lgtm + approve)

[furkatgofurov7]

Based on the agreement during the call triaging #9104, setting the priority to:

/priority important-longterm

[killianmuldoon]

Note: the PR #9263 should be usable for testing this in CAPI once it's merged. I've added two commits and the label for tide to squash it.

[mjlshen]

I can reproduce it with #9263, now onto fixing :D

Sample output:

❯ go run hack/tools/release/notes.go -from cb36c47679a0581abc9f892e5c1a701267257b4e -workers 1

...

## Changes since cb36c47679a0581abc9f892e5c1a701267257b4e
---
## :chart_with_upwards_trend: Overview
- 16 new commits merged
- 2 breaking changes :warning:

## :warning: Breaking Changes
- Dependency: Bump to controller-runtime v0.16 (#8999)
- util: : Remove go-vcs dependency from releaselink tool (#9288)

## :seedling: Others
- API: Remove reliance on controller-runtime scheme builder for remaining API groups (#9266)
- API: Test and document controller ownerReferences (#9153)
- CI: Bump tj-actions/changed-files from 37.6.1 to 38.0.0 (#9276)
- Dependency: Bump envtest binaries to 1.28 (#9268)
- Dependency: Bump github.com/emicklei/go-restful/v3 from 3.10.2 to 3.11.0 in /test (#9272)
- Dependency: Bump google.golang.org/api from 0.137.0 to 0.138.0 in /hack/tools (#9280)
- Dependency: Bump google.golang.org/grpc from 1.55.0 to 1.55.1 (#9284)
- Dependency: Bump sigs.k8s.io/controller-tools from 0.12.1 to 0.13.0 in /hack/tools (#9281)
- Dependency: Update ensure-kubectl.sh to 1.28 (#9275)
- Devtools: Bump CAPI visualizer to v1.2.0 (#9195)
- e2e: Add CRS re-reconcile to ownerReference test (#9296)
- e2e: Add test for ownerReference apiVersion update (#9269)

:book: Additionally, there have been 2 contributions to our documentation and book. (#9291, #9270) 

[mjlshen]

This is caused by /tide merge-method-squash doing a squash merge instead of creating a merge commit, so we're missing them when we call git with the --merges flag, e.g.

cluster-api/hack/tools/release/notes.go

Line 230 in a0dc548

cmd = exec.Command("git", "rev-list", commitRange+"..HEAD", "--merges", "--pretty=format:%B") //nolint:gosec

[mjlshen]

The kubernetes release tool takes care of this case by considering all commits and identifying a PR from the commit message https://github.com/kubernetes/release/blob/7cac31e9ed4b8afe87af6d34a504f8ee23477ab8/pkg/notes/notes.go#L955-L980

[sbueringer]

Can we do the same?

[mjlshen]

Yeah, definitely - I briefly looked into whether it would be easier to switch to the kubernetes release tool, but it doesn't seem like it, so I can re-implement this in our own tooling for now

[sbueringer]

I would definitely appreciate the improvement. While core CAPI might move to the upstream tool, I don't know what other providers will do.
In CAPV I would in any case like to continue to use this tool (didn't discuss it with the CAPV community yet though) and this might apply to a lot of other providers as well.

This tool is currently used by core CAPI and 5 other providers (https://cs.k8s.io/?q=sigs.k8s.io%2Fcluster-api%2Fhack%2Ftools%2Frelease&i=nope&files=&excludeFiles=&repos=).

So I can imagine that even if the release team doesn't want to invest in this tool anymore we might still keep it in core CAPI for usage by providers (the only alternative would be to fork it into each provider, which doesn't seem like a great option, or everyone has to move to the upstream tool or something in between)

[g-gaston]

I vote to make this fix to our tool as well

[furkatgofurov7]

@mjlshen Hi 👋🏼 Just reaching out to know if there are updates on this issue and anything I can help with to move forward? Thanks!

[mjlshen]

Hi @furkatgofurov7 - unfortunately no real updates to share. I am working on adding the fix to our tool though and just have been delayed due to time constraints and have been putting this off... I will commit to having a PR to share by next Wednesday

[furkatgofurov7]

@mjlshen hey, perhaps I missed it, did you have some time to open a PR 🙂

[g-gaston]

/close
Implemented by #9618

[k8s-ci-robot]

@g-gaston: Closing this issue.

In response to this:

/close
Implemented by #9618

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.