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

🌱 Make release notes tool not dependent on local git #9618

Merged

Conversation

g-gaston
Copy link
Contributor

@g-gaston g-gaston commented Oct 25, 2023

What this PR does / why we need it:

Now all the data is retrieved through GitHub APIs, making the tool more portable and easier to use. It should not increase the rate limiting chances since it now performs less API requests (by getting the label from all PRs at once instead of with one request per PR).

This tool had also outgrown its previous code structure, which made a bit difficult to made these changes and think about how the overall program works. So I went a bit overboard with the original purpose and refactor the code a bit. Mostly splitting it into different files and adding some comments here and there. the new structure should make easier to reuse piece of the code from other release team tools and compose the different pieces to do similar but different tasks (like the weekly update).

I also have simplified the command flags for the tool, since the comms team doesn't need most of them and they needed to be re-implemented with this new "API approach" if we wanted to keep supporting them. We can implement support and re-add them if the need for them comes back in the future.

The new UI is:

Usage of ./bin/notes:
  -add-kubernetes-version-support
        If enabled, will add the Kubernetes version support header. (default true)
  -branch string
        The branch to generate the notes from. If not set, it will be calculated from release.
  -deprecation
        If enabled, will add a templated deprecation warning header. (default true)
  -from string
        The tag or commit to start from. It must be formatted as heads/<branch name> for branches and tags/<tag name> for tags. If not set, it will be calculated from release.
  -pre-release-version
        If enabled, will add a pre-release warning header. (default false)
  -prefix-area-label
        If enabled, will prefix the area label. (default true)
  -release string
        The tag for the enw release.
  -repository string
        The repo to run the tool from. (default "kubernetes-sigs/cluster-api")
  -to string
        The ref (tag, branch or commit to stop at. It must be formatted as heads/<branch name> for branches and tags/<tag name> for tags. If not set, it will default to branch.

For new releases, it's possible to just specify the tag of the new (to be released) release and the program will compute the rest of the values. This also opens the door in the future to alter the behavior depending on if it's a RC/beta release (instead of having individual flags).

./bin/notes --release=v1.6.0 > CHANGELOG/v1.6.0.md

Apologies in advance for the big diff.

I want to merge #9617 first so we can look at the output changes.

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/release Issues or PRs related to releasing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 25, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 25, 2023
@g-gaston g-gaston force-pushed the release-notes-read-from-github branch from d7b55d0 to a3e863f Compare November 30, 2023 21:33
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023
@g-gaston g-gaston force-pushed the release-notes-read-from-github branch 2 times, most recently from ffb51ab to 6db511f Compare December 1, 2023 20:54
@g-gaston g-gaston changed the title [WIP] 🌱 Make release notes tool not dependent on local git 🌱 Make release notes tool not dependent on local git Dec 1, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2023
Comment on lines +60 to -62
- CAPD: Fix fail-swap-on=false flag not being part of kind images anymore (#8767)
- CAPD: Implement watch filter (#8789)
- CAPD: Test/capd: fix kind mapper entry for v1.25.11 (#8914)
- CAPD: Test/e2e fix fail-swap-on=false flag not being part of kind images anymore (#8767)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These the same PR. The title was changed in the PR and since now we read from the PR API and not the commits, now we output the new title. This is expected (and IMO a good change).

@g-gaston g-gaston force-pushed the release-notes-read-from-github branch from 6db511f to 1181097 Compare December 4, 2023 17:07
Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

This is some great stuff! Excited to see this tool in use. Everything looks good pending a few minor comments.


// githubClient uses the gh CLI to make API request to GitHub.
type githubClient struct {
// repo is full [org]/[repo_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i think this comment can be reworded. Is it the full url? I'm not entirely sure what a full [org]/[repo_name] is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example kubernetes-sigs/cluster-api
kubernetes-sigs is the org and cluster-api is the repo name

How would you phrase it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm just a bit confused by the usage of the word full. I would maybe change it to repo in format [org]/[repo_name] but I think it's fine the way it is. Whichever you think is better!

hack/tools/release/notes/github.go Show resolved Hide resolved
hack/tools/release/notes/main.go Show resolved Hide resolved
hack/tools/release/notes/main.go Outdated Show resolved Hide resolved
hack/tools/release/notes/print.go Show resolved Hide resolved
@willie-yao
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 07b70012fa2b3e61d954fc6bb95b9f2eb6d213cf

@furkatgofurov7
Copy link
Member

/test ?

@k8s-ci-robot
Copy link
Contributor

@furkatgofurov7: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-build-main
  • /test pull-cluster-api-e2e-full-dualstack-and-ipv6-main
  • /test pull-cluster-api-e2e-full-main
  • /test pull-cluster-api-e2e-main
  • /test pull-cluster-api-e2e-mink8s-main
  • /test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main
  • /test pull-cluster-api-test-main
  • /test pull-cluster-api-test-mink8s-main
  • /test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-apidiff-main
  • /test pull-cluster-api-e2e-scale-main-experimental

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-apidiff-main
  • pull-cluster-api-build-main
  • pull-cluster-api-e2e-main
  • pull-cluster-api-test-main
  • pull-cluster-api-verify-main

In response to this:

/test ?

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2023
@furkatgofurov7
Copy link
Member

/test pull-cluster-api-e2e-main

@furkatgofurov7
Copy link
Member

@g-gaston looks like it needs a rebase, I'll take a look after it is rebased.

@sbueringer
Copy link
Member

(didn't look at the code yet)

@g-gaston Does this also make it possible to use tide with the merge method so folks don't have to manually squash commits before PR merge? (xref: #9249)

@g-gaston
Copy link
Contributor Author

g-gaston commented Dec 5, 2023

(didn't look at the code yet)

@g-gaston Does this also make it possible to use tide with the merge method so folks don't have to manually squash commits before PR merge? (xref: #9249)

maybe. At least it should make it easier to make it work if it doesn't on its current state
@mjlshen was gonna verify that but I'll take a look when I find some time

@sbueringer

Edit: I just checked. This PR doesn't fix the issue but it's actually easy to fix on top of the new code. I just need to add a second regex condition here. If you give me a review, I'll add it as a bonus 🤝

@g-gaston g-gaston force-pushed the release-notes-read-from-github branch from 2d55fca to 3bc3415 Compare December 5, 2023 17:30
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023
@g-gaston g-gaston force-pushed the release-notes-read-from-github branch from 82b8e95 to ee8d6b1 Compare December 13, 2023 17:58
@g-gaston g-gaston force-pushed the release-notes-read-from-github branch from ee8d6b1 to 4b3c154 Compare December 13, 2023 18:11
@g-gaston
Copy link
Contributor Author

/test pull-cluster-api-test-main

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2023
Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f6c03a1cabe5b95c7a56b45f69c40199d714fab7

@sbueringer
Copy link
Member

@g-gaston Thank you, looks good!

Really appreciate the improvements made to the tool in this PR (and over the last few months)!!

Happy to approve after a rebase for the go.mod conflicts.

Now all the date is retrieved through GitHub APIs, making the tool more
portable and easier to use. It should not increase the rate limiting
chances since it now performs less API requests (by getting the label
from all PRs at once instead of with one request per PR).
@g-gaston g-gaston force-pushed the release-notes-read-from-github branch from 4b3c154 to 4452c01 Compare December 14, 2023 16:53
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2023
@g-gaston
Copy link
Contributor Author

@g-gaston Thank you, looks good!

Really appreciate the improvements made to the tool in this PR (and over the last few months)!!

Happy to approve after a rebase for the go.mod conflicts.

Done!

@sbueringer
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a99197f0c1e71088190393ac70b9ce8a71048afb

@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 Dec 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit baa3aa0 into kubernetes-sigs:main Dec 14, 2023
21 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.7 milestone Dec 14, 2023
@sbueringer
Copy link
Member

@g-gaston Could it be that we have to update release-tasks.md? As far as I can tell the current instructions don't work anymore without modifications

}
// computeConfigDefaults calculates the value the non specified configuration fields
// based on the set fields.
func computeConfigDefaults(config *notesCmdConfig) error {
Copy link
Member

@sbueringer sbueringer Jan 3, 2024

Choose a reason for hiding this comment

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

I tried to run this tool for CR with the following args:

-from tags/v0.16.0 -branch main -release v0.17.0 -repository kubernetes-sigs/controller-runtime -prefix-area-label=false -add-kubernetes-version-support=false

This func returns in l.158. Then l.182 is never executed and it eventually panics in parseRef

panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
main.parseRef({0x0, 0x0})
        /Users/buringerst/code/src/sigs.k8s.io/cluster-api/hack/tools/release/notes/ref.go:43 +0xbc
main.(*notesCmd).run(0x1400004e050)
        /Users/buringerst/code/src/sigs.k8s.io/cluster-api/hack/tools/release/notes/main.go:101 +0x124
main.main()
        /Users/buringerst/code/src/sigs.k8s.io/cluster-api/hack/tools/release/notes/main.go:41 +0x28
Exiting.

Copy link
Member

Choose a reason for hiding this comment

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

If I see correctly we should also check for toRef != "" or maybe drop the if (not sure)

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise it works great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are 100% right
I'll get it fixed

That said, you don't need to specify -from in this case. Specifying -release is enough for the program to infer the rest. You do need to specify the branch bc it seems like in controller-runtime minor releases are cut directly from main. That would work even without the fix.

./bin/notes --branch main --release v0.17.0 --repository kubernetes-sigs/controller-runtime --prefix-area-label=false --add-kubernetes-version-support=false

Copy link
Member

@sbueringer sbueringer Jan 4, 2024

Choose a reason for hiding this comment

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

Thx, yup sounds reasonable. I just started with adding --from without thinking much about it and then I added what was needed :)

But you're right leaving --from empty works to skip the if branch in l.157

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g-gaston
Copy link
Contributor Author

g-gaston commented Jan 4, 2024

@g-gaston Could it be that we have to update release-tasks.md? As far as I can tell the current instructions don't work anymore without modifications

yes totally
#9957

@cahillsf
Copy link
Member

hey @g-gaston 👋 just checking, is this still needed or has your PR accomplished this?

@g-gaston
Copy link
Contributor Author

hey @g-gaston 👋 just checking, is this still needed or has your PR accomplished this?

This PR accomplishes it :)

@sbueringer
Copy link
Member

Thx again for that!!

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants