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

Jump to bottom

🌱 Add new reusable release note generator #944

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Add new reusable release note generator #944

wants to merge 1 commit into from

Conversation

mquhuy
Copy link
Member

@mquhuy mquhuy commented Jan 9, 2025 •
edited
Loading

This tool uses go-github package to fetch PR information from upstream using github API, hence doesn't need to be triggered from inside a repo, and can work from anywhere. It also avoids the usage of os.Exec(), and rely on go-native methods instead.

Compare to the current script that we are having in all releasing repositories, this tool:

  • Move from using local repo to accessing remote repo via API.
  • Is simpler: multiple custom functionalities are rewritten in simpler and more native way.
  • Take custom repo name and repo owner: It works with multiple repos, and can work on a fork if necessary.
  • Doesn't use os.Exec(): less error-prone and doesn't need to run inside the repo.
  • Doesn't assume local repo configuration, like uptream git fetch before running.
  • Doesn't require GITHUB_TOKEN for public repo. Still support GITHUB_TOKEN if we're to run this tool multiple times in a row.
  • Is more reliable as everything is fetched from github API.
  • As it works with custom repo name, we no longer need to copy the script to all of our repositories.

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 9, 2025
@mquhuy mquhuy force-pushed the mquhuy/new-release-note-generator branch from bb0da43 to 7302c54 Compare January 9, 2025 10:31
adilGhaffarDev
adilGhaffarDev reviewed Jan 9, 2025
View reviewed changes
hack/release/main.go Outdated Show resolved Hide resolved
tuminoid
tuminoid reviewed Jan 9, 2025
View reviewed changes
hack/release/main.go Outdated Show resolved Hide resolved
@mquhuy mquhuy force-pushed the mquhuy/new-release-note-generator branch from 7302c54 to cb74929 Compare January 9, 2025 11:03
@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mboukhalfa for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

tuminoid
tuminoid reviewed Jan 9, 2025
View reviewed changes
hack/release/main.go Outdated Show resolved Hide resolved
hack/release/main.go Show resolved Hide resolved
@mquhuy mquhuy force-pushed the mquhuy/new-release-note-generator branch 7 times, most recently from 0ffe6e5 to 15b019e Compare January 9, 2025 13:47
@mquhuy
Copy link
Member Author

mquhuy commented Jan 9, 2025

/hold It seems that Github rate limit makes this script pretty slow for long lists of PRs. Let's discuss first if that's acceptable.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2025
@mquhuy mquhuy force-pushed the mquhuy/new-release-note-generator branch from 15b019e to eb1dc9a Compare January 10, 2025 11:53
@mquhuy
Copy link
Member Author

mquhuy commented Jan 10, 2025

/unhold
We no longer fetch PR number for all commits.

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2025
tuminoid
tuminoid reviewed Jan 10, 2025
View reviewed changes
hack/release/main.go Outdated
var key string
switch {
case strings.HasPrefix(body, ":sparkles:"), strings.HasPrefix(body, "✨"):
key = features
Copy link
Member

Choose a reason for hiding this comment

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

Indents are wonky again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I fixed it again.

@tuminoid
Copy link
Member

Can you update the description that describes how it actually works and what is the difference to the old way? The current bullet list really doesn't convey the design change.

@mquhuy mquhuy force-pushed the mquhuy/new-release-note-generator branch from eb1dc9a to 507aa9d Compare January 10, 2025 12:23
@mquhuy
Copy link
Member Author

mquhuy commented Jan 10, 2025

Can you update the description that describes how it actually works and what is the difference to the old way? The current bullet list really doesn't convey the design change.

Yes. Could you check if the current description makes more senses?

@tuminoid
Copy link
Member

Can you update the description that describes how it actually works and what is the difference to the old way? The current bullet list really doesn't convey the design change.

Yes. Could you check if the current description makes more senses?

Yes, it a bit better. It would move from reading a local repo to accessing the repo over API.

How would this be used? Do we make released project's makefile depend on project-infra when we run "make release-notes", or ... ?

@mquhuy
Add new reusable release note generator
fc7718d 
Compare to the current script that we are having in all releasing
repositories, this tool:
- Is simpler
- Take custom repo name and repo owner
- Doesn't use os.Exec(), hence doesn't need to run inside the repo
- Doesn't require uptream tag fetching
- Doesn't require, but still supports, GITHUB_TOKEN
- Is more reliable as everything is fetched from github

Signed-off-by: Huy Mai <[email protected]>
@mquhuy mquhuy force-pushed the mquhuy/new-release-note-generator branch from 507aa9d to fc7718d Compare January 10, 2025 13:30
@mquhuy
Copy link
Member Author

mquhuy commented Jan 10, 2025

Yes, it a bit better. It would move from reading a local repo to accessing the repo over API.

Thank you, I will update that.

How would this be used? Do we make released project's makefile depend on project-infra when we run "make release-notes", or ... ?

In the make file, we can do something like go install <project-infra>/hack/release@main, then use the binary to run make release-notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adilGhaffarDev adilGhaffarDev adilGhaffarDev left review comments

@tuminoid tuminoid tuminoid left review comments

@kashifest kashifest Awaiting requested review from kashifest

@mboukhalfa mboukhalfa Awaiting requested review from mboukhalfa

Assignees
No one assigned
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mquhuy @metal3-io-bot @tuminoid @adilGhaffarDev