-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ Add dependencies section to release notes tool #10043
✨ Add dependencies section to release notes tool #10043
Conversation
36e3588
to
8509142
Compare
e444bab
to
326a0c3
Compare
@willie-yao nice! |
/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.
Very nice. Thank you this will be very useful!
In turn we should probably get rid of the "bump" entries for regular Go dependencies in our release notes
(at least as a manual step)
324e4b5
to
525d254
Compare
hack/tools/release/notes/process.go
Outdated
@@ -132,8 +132,13 @@ func (g prEntriesProcessor) generateNoteEntry(p *pr) *notesEntry { | |||
// Release trigger PRs from previous releases are not included in the release notes | |||
return nil | |||
case strings.HasPrefix(entry.title, ":seedling:"), strings.HasPrefix(entry.title, "🌱"): | |||
title := removePrefixes(entry.title, []string{":seedling:", "🌱"}) | |||
// Skip PRs bump dependencies as they will be listed separately. | |||
if area == "Dependency" { |
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.
We are flagging a lot more than just dependabot go dependency bumps with area/dependency (e.g. Go, Trivy, ... bumps, but I think even more).
I think dropping all area/dependency PRs is too much, we might lose something important there.
Can we drop all PRs from dependabot instead (filter on author:app/dependabot )? (we would additionally drop GitHub actions bumps, but I think that is fine)
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.
(also works better with other repositories that don't set the label but are also using dependabot like CAPV and controller-runtime)
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 trying to filter the GitHub API response for the author but I'm a bit confused on how the code works here. How do we go from returning the collection
here to the prs []githubPR
object here? I tried adding an author
field to the githubPR
type, but I'm a bit confused where the existing fields (Number, Title, Labels) are populated. pageRead
just seems to give me the entire PR page. @g-gaston do you know if there's a way to add an author field to the current flow?
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.
Sorry I don't fully understand the question. If the search api returns an author field for pr objects (I'm not sure if it does, you would need to check the docs or just run a query yourself and check the output), then adding it to the githubPR
struct will make that available when calling listMergedPRs
.
If you want to make it available to the processor (and the printer), you'll need to add the field to the pr
struct as well: https://github.com/kubernetes-sigs/cluster-api/blob/main/hack/tools/release/notes/list.go#L117-L121
What I would do here is add the author information to the pr
domain object so you have it available in the processor (this is the one responsible from processing each pr and deciding which kind of note entry it should be). In your case it seems you want the processor to discard PRs based on the author, so they should no be converted to notes entries. Beware this will affect the total number of commits printed at the beginning of the notes. If you don't want this to happen, then maybe slightly changing the responsibilities of processor and printer is a better idea.
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.
Can we drop all PRs from dependabot instead (filter on author:app/dependabot )?
There still seems to be a good amount of dependencies bump PRs that are not created by dependabot. Should we just leave those as is?
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 it's better if we sort through manual bump PRs manually compared to dropping all dependency PRs from the release notes.
We even might just leave them in there. Usually we only create manual bumps if either dependabot can't bump something or if we have to bump manually because we have to change more.
Maybe let's just go with dropping PRs from author dependabot and leaving everything else in the release notes.
hack/tools/release/notes/print.go
Outdated
repoURL, p.fromTag, p.toTag, | ||
) | ||
if err != nil { | ||
fmt.Printf("Error getting dependencies: %v", err) |
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 would add a new error return parameter and propagate it up (with a bit of error wrapping).
Then we can fail properly
525d254
to
433585b
Compare
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.
Could you run the integration tests and make sure you update the golden files to match the new output?
hack/tools/release/notes/print.go
Outdated
@@ -24,6 +24,8 @@ import ( | |||
"sort" | |||
"strings" | |||
|
|||
"k8s.io/release/pkg/notes" |
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.
curious why didn't get added to the go.mod?
eae6f46
to
179a08f
Compare
In regards to the broken links: I think there is an issue with the kubernetes dependency tool right now that messes up the links when a tag includes a "/". We faced this issue when creating release notes for the latest CAPZ release. We might need to hold this PR until that issue is fixed. |
@willie-yao Let's please try to figure out if the markdownlinkchecker can skip this test folder (see .markdownlinkcheck.json) |
- Dependency: Bump cert-manager to v1.13.0 (#9413) | ||
- Dependency: Bump cert-manager to v1.13.1 (#9507) | ||
- Dependency: Bump cert-manager to v1.13.2 (#9658) | ||
- Dependency: Bump controller runtime to v1.15.3 (#9624) | ||
- Dependency: Bump controller-runtime to v0.15.1 (#9127) | ||
- Dependency: Bump corefile-migration library to v1.0.21 (#9309) |
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 curious to see why there are new PRs appearing here.
I was expecting to only see two types of diff: PRs being omitted (authored by dependabot) in this section and the new deps section
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.
Looks like the tool is listing merged PRs up to the date of the latest commit on the release branch. It looks like there are PRs still being cherry-picked into v1.5.0 which is why these PRs show up. Is this something we should worry about? In a normal release cycle, we'll be generating the release notes at the correct day, but if we retroactively generate them, we might be getting some new commits that aren't part of the release.
I'm thinking maybe we can add a flag for a specific end date, or we can just manually remove the newer PRs since I don't think we'd be retroactively generating release notes in the normal release flow.
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.
Umm I see
I suspect there is bug in how the tests are setup then
Because they are supposed to use "static" config so the output doesn't change even if we merge more commits in the original branch.
We might be missing a to
ref or the ref might be incorrect
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 used the command RELEASE_TAG=v1.5.0 make release-notes
to generate the golden files so I don't think it's just a bug in the test. I think it looks for the latest commit in the branch rather than when the tag is created: https://github.com/willie-yao/cluster-api/blob/release-notes-dependencies/hack/tools/release/notes/list.go#L65
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 see that it is being defaulted to heads/branch
instead of tags/tagname
here. Should we make tags/
the default if RELEASE_TAG
is specified? I don't see why a user would need to read from heads/branch
if they are generating release notes for a specific release tag.
928f44f
to
2c71c7d
Compare
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 great. Thanks a lot for pushing this all the way @willie-yao, I know it took a lot with those bugs in the deps module
/lgtm
LGTM label has been added. Git tree hash: 58d5d9864aba007d8578ee387010d23df78b7cce
|
2c71c7d
to
7c388ad
Compare
Squashed and rebased! |
/lgtm |
LGTM label has been added. Git tree hash: 75bab9edff1b64d19cfbed1e2cc7bcade414f10c
|
dont think i can approve since there are changes in /lgtm |
7c388ad
to
3777030
Compare
/lgtm |
LGTM label has been added. Git tree hash: c06d4bf2d84c9795f9801f78f38371705598ddcc
|
/lgtm /hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR adds a dependencies section to the release notes generator. It uses the kubernetes release notes tool: https://github.com/kubernetes/release/blob/master/cmd/release-notes/README.md
Example of what is printed:
This block is added aft er the
Others
section of the release notes.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #9854
/area release