-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Add additional blocks to release note generation #9247
Conversation
cc @kubernetes-sigs/cluster-api-release-team |
There was a problem hiding this 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.
29ccb61
to
a8535b6
Compare
There was a problem hiding this 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
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 |
9289954
to
0736c5a
Compare
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 |
LGTM label has been added. Git tree hash: ef0e5ca56789693be9ea9847c69a7cebbd404705
|
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 |
/lgtm |
/cc @sbueringer when you have some spare time :) |
/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)") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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. |
There was a problem hiding this 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
* 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>
0736c5a
to
fe049f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: 14ce8be4d46142c01e711970c8bdd650c4f2ca43
|
/lgtm |
Thx /approve |
[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 |
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:
Which issue(s) this PR fixes:
Fixes #9248
Part of #9104
Sample header output:
/area release