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.

Sign up for GitHub

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
merged 1 commit into from
Aug 10, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion hack/tools/release/notes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ var (
unknown,
}

repo = flag.String("repository", "kubernetes-sigs/cluster-api", "The tag or commit to start from.")
repo = flag.String("repository", "kubernetes-sigs/cluster-api", "The repo to run the tool from.")

fromTag = flag.String("from", "", "The tag or commit to start from.")

Expand Down Expand Up @@ -447,6 +447,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 •
edited
Loading

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 •
edited
Loading

Choose a reason for hiding this comment

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

The three that have PRs references:

All appear to have been merged manually.

Not sure what happened with kubernetes-sigs/cluster-api-provider-vsphere#1819 but it has a very different merge commit: Merge branch 'kubernetes-sigs:main' into 1162

Copy link
Member

@sbueringer sbueringer Aug 10, 2023 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

c.body = "ERROR: BODY MISSING. FIX MANUALLY"
}
entry.title = trimTitle(c.body)
var fork string

Expand Down