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

🐛 Generate warning when release notes can not be generated #9163

Merged

Conversation

killianmuldoon
Copy link
Contributor

Signed-off-by: killianmuldoon [email protected]

Add a warning to the release notes tool when a release note can not be generated at all - i.e. when the body is empty. We noticed this error when using the release tool to generate notes for the Cluster API provider for CAPV.

This error hasn't occured in releases of CAPI AFAICT. Running go run ./hack/tools/release/notes.go --from=v1.4.0 doesn't produce any errors.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 10, 2023
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 10, 2023
@killianmuldoon
Copy link
Contributor Author

/area release

@k8s-ci-robot k8s-ci-robot added the area/release Issues or PRs related to releasing label Aug 10, 2023
@@ -447,6 +444,9 @@ func modifyEntryTitle(title string, prefixes []string) string {
// generateReleaseNoteEntry processes a commit into a PR line item for the release notes.
func generateReleaseNoteEntry(c *commit) (*releaseNoteEntry, error) {
entry := &releaseNoteEntry{}
if c.body == "" {
Copy link
Contributor Author

@killianmuldoon killianmuldoon Aug 10, 2023

Choose a reason for hiding this comment

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

I'm still trying to root cause and find out if we can fix these notes up automatically. For the recent CAPV release it looks like:

The note with no PR linked is this one: kubernetes-sigs/cluster-api-provider-vsphere#1819.

- ERROR: BODY MISSING. FIX MANUALLY
- ERROR: BODY MISSING. FIX MANUALLY (#2147)
- ERROR: BODY MISSING. FIX MANUALLY (#2166)
- ERROR: BODY MISSING. FIX MANUALLY (#2176)

Copy link
Contributor Author

@killianmuldoon killianmuldoon Aug 10, 2023

Choose a reason for hiding this comment

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

Copy link
Member

@sbueringer sbueringer Aug 10, 2023

Choose a reason for hiding this comment

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

Oh damn. Yeah that was me to unblock some Prow stuff (CI was green before I did it :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I think naadir might have also done some manual merges when prow was off - wonder if there's a way you did it that made the history come out weird.

I don't think we have to worry about this too much though - fine with having a best-effort warning until this becomes a constant problem. CAPI history seems fine.

Copy link
Member

@sbueringer sbueringer Aug 10, 2023

Choose a reason for hiding this comment

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

I assume the problem is if the description is left empty (what I did)

image

Copy link
Member

@sbueringer sbueringer Aug 10, 2023

Choose a reason for hiding this comment

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

But it's fine. Just thought let's add some info if it's easily available. (but didn't know if the info is easily available or not :D)

Copy link
Member

Choose a reason for hiding this comment

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

How did you figure out it was this PR?

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 issue is that the body is empty at the point the notes are generated. We could try to detect that a bit earlier, but it would involve either refactoring or looping through all the commits. IMO it's not worth doing unless this happens again - which this change will signal.

Copy link
Member

Choose a reason for hiding this comment

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

I was guessing (without looking at the code) that when we have the body here we should also have the corresponding commit id close by :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not AFAIU

@killianmuldoon
Copy link
Contributor Author

FYI @kubernetes-sigs/cluster-api-release-team - a bug we found when using the tool in another repo.

@sbueringer
Copy link
Member

sbueringer commented Aug 10, 2023

/lgtm
/approve

/hold
up to you, fine to merge from my side

Thank you for taking this over!

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b694efb286b3acc2665e153845c32e151be8bb90

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2023
@killianmuldoon
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit bc9c48a into kubernetes-sigs:main Aug 10, 2023
10 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release Issues or PRs related to releasing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants