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

🌱 Add additional blocks to release note generation #9247

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

mjlshen
Copy link
Contributor

@mjlshen mjlshen commented Aug 20, 2023

What this PR does / why we need it:

This PR allows the existing release notes generation tool to generate (optionally in some cases) commonly used headers that have been manually filled out in the past:

  • An optional header when generating a release candidate
  • A highlights header with filler to manually replace
  • An optional header about deprecation warnings

Which issue(s) this PR fixes:

Fixes #9248
Part of #9104


Sample header output:

❯ go run hack/tools/release/notes.go -from release-1.5 -workers 1 -release-candidate -deprecation
🚨 This is a RELEASE CANDIDATE. Use it only for testing purposes. If you find any bugs, file an [issue](https://github.com/kubernetes-sigs/cluster-api/issues/new).
## 👌 Kubernetes version support

- Management Cluster: v1.**X**.x -> v1.**X**.x
- Workload Cluster: v1.**X**.x -> v1.**X**.x

[More information about version support can be found here](https://cluster-api.sigs.k8s.io/reference/versions.html)

## Highlights

* REPLACE ME

## Deprecation Warning

REPLACE ME: A couple sentences describing the deprecation, including links to docs.

* [GitHub issue #REPLACE ME](REPLACE ME)

## Changes since release-1.5

/area release

@k8s-ci-robot k8s-ci-robot added area/release Issues or PRs related to releasing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 20, 2023
@mjlshen
Copy link
Contributor Author

mjlshen commented Aug 20, 2023

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

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Looks generally good - one overall comment.

As this gets more complex it might be a good idea to move to Go templates. That way the information to fill out the additional headers can be maintained through the release cycle.

hack/tools/release/notes.go Outdated Show resolved Hide resolved
hack/tools/release/notes.go Outdated Show resolved Hide resolved
hack/tools/release/notes.go Show resolved Hide resolved
hack/tools/release/notes.go Outdated Show resolved Hide resolved
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Overall lgtm, pending to clarify few open questions from reviewers and should be good to take it in! Thanks @mjlshen for working on this

hack/tools/release/notes.go Outdated Show resolved Hide resolved
hack/tools/release/notes.go Show resolved Hide resolved
@richardcase
Copy link
Member

As this gets more complex it might be a good idea to move to Go templates. That way the information to fill out the additional headers can be maintained through the release cycle.

I like the idea of using a go template. This would be similar to the kubernetes release-notes tool that uses go templates (see here). It also allows you to override the template

@mjlshen mjlshen force-pushed the missing-notes-sections branch 2 times, most recently from 9289954 to 0736c5a Compare August 24, 2023 19:43
@mjlshen
Copy link
Contributor Author

mjlshen commented Aug 25, 2023

I took a look at switching over to the kubernetes release tool here: #8292 (comment)

I think it's a bigger effort, so I'm leaning towards taking the easy win here and continuing discussion in the issue to see where we want to go next with this release notes tool (bigger refactor or changes upstream + switching over).

@furkatgofurov7
Copy link
Member

I took a look at switching over to the kubernetes release tool here: #8292 (comment)

I think it's a bigger effort, so I'm leaning towards taking the easy win here and continuing discussion in the issue to see where we want to go next with this release notes tool (bigger refactor or changes upstream + switching over).

Agree, currently, the best way forward for us should be to take this in and continue the switching over to the new process in the original issue.

/lgtm

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

LGTM label has been added.

Git tree hash: ef0e5ca56789693be9ea9847c69a7cebbd404705

@richardcase
Copy link
Member

I think it's a bigger effort, so I'm leaning towards taking the easy win here and continuing discussion in the issue to see where we want to go next with this release notes tool (bigger refactor or changes upstream + switching over).

Sorry I wasn't suggesting that you adopt it, just pointing out that i think its a good idea to adopt a go template (as other tools have done).

@mjlshen
Copy link
Contributor Author

mjlshen commented Aug 25, 2023

Sorry I wasn't suggesting that you adopt it, just pointing out that i think its a good idea to adopt a go template (as other tools have done).

Oh no apology needed, it was a good suggestion! I was just sharing my initial findings/conclusion in case I got something wrong too

@g-gaston
Copy link
Contributor

/lgtm

@furkatgofurov7
Copy link
Member

/cc @sbueringer when you have some spare time :)

@furkatgofurov7
Copy link
Member

/cc @vincepri

@@ -73,6 +73,8 @@ var (

prefixAreaLabel = flag.Bool("prefix-area-label", true, "If enabled, will prefix the area label.")

preReleaseVersion = flag.Bool("pre-release-version", false, "If enabled, will add a pre-release warning header. (default false)")
Copy link
Member

Choose a reason for hiding this comment

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

Is this being set locally? Would there be a way to understand which tag we're running against and set this flag automatically? For example: v1.0.0-beta.1 I'd expect the same behavior, and similar for alpha.X or rc.X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah we talked about this here: #9247 (comment) it's doable long-term with a bigger refactor, but we're mostly just trying to make sure we have the standardized formatting somewhere

@mjlshen
Copy link
Contributor Author

mjlshen commented Sep 1, 2023

cc @killianmuldoon / @vincepri wondering if you think this PR is ok to merge? I will be working on a refactor of this tool through #9249 for example, but this PR's goal is mostly to get standardized wording in.

Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just a few nits

hack/tools/release/notes.go Outdated Show resolved Hide resolved
hack/tools/release/notes.go Outdated Show resolved Hide resolved
hack/tools/release/notes.go Show resolved Hide resolved
* An optional header when generating a release candidate
* A highlights header with filler to manually replace
* An optional header about deprecation warnings

Signed-off-by: Michael Shen <mishen@umich.edu>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 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 Sep 1, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 14ce8be4d46142c01e711970c8bdd650c4f2ca43

@furkatgofurov7
Copy link
Member

/lgtm

@sbueringer
Copy link
Member

Thx

/approve

@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 Sep 4, 2023
@k8s-ci-robot k8s-ci-robot merged commit df024c3 into kubernetes-sigs:main Sep 4, 2023
22 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Sep 4, 2023
@mjlshen mjlshen deleted the missing-notes-sections branch September 4, 2023 11:48
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add additional headers to the release notes tool output
9 participants