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

Sort release notes by Kind(s) instead of SIG(s) #923

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Sort release notes by Kind(s) instead of SIG(s) #923

merged 1 commit into from
Jan 17, 2020

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Nov 5, 2019

This is a short demo how it looks like if we sort the release notes by its kind instead of the SIG.

For example:

> go run cmd/release-notes/main.go --start-rev v1.16.0 --end-rev v1.17.0

https://gist.github.com/saschagrunert/3eaf2d16b3f845fa69cf7a6661d81b05

@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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Nov 5, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2019
@tpepper
Copy link
Member

tpepper commented Nov 5, 2019

This form is definitely interesting. I wonder if it could be visually simplified still. Having every other line be "Courtesy of ..." is a bit visually distracting. Maybe if each stanza were more like:

-<p>Graduate ScheduleDaemonSetPods to GA. (feature gate will be removed in 1.18) action required. (<a href="https://github.com/kubernetes/kubernetes/pull/82795">#82795</a>, <a href="https://github.com/draveness">@draveness</a>)</p>
-<p>Courtesy of SIG Apps, SIG Scheduling, and SIG Testing</p>
+<p>Graduate ScheduleDaemonSetPods to GA. (feature gate will be removed in 1.18) action required. (<a href="https://github.com/kubernetes/kubernetes/pull/82795">#82795</a>, <a href="https://github.com/draveness">@draveness</a>, SIGs Apps, Scheduling, and Testing</p>

That removes 50 lines only though. And the results are still MANY MANY pages.

@saschagrunert
Copy link
Member Author

Alright, I tried to improve the density and removed some unnecessary categorization, now it looks like this (--start-rev v1.16.0 --end-rev master):

https://gist.github.com/saschagrunert/baf7cb9114d7a58e6728592fac3659ed

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2019
@saschagrunert
Copy link
Member Author

/retest

1 similar comment
@jeefy
Copy link
Member

jeefy commented Nov 7, 2019

/retest

@saschagrunert
Copy link
Member Author

@jeefy do you think this format makes sense and is better than the current one?

@jeefy
Copy link
Member

jeefy commented Nov 26, 2019

I like the readability of it better, but I also think we should somehow identify what sigs the PRs come from.

Also the API Changes section is based on things with kind/api-change so really that label should be ignored under "Other Changes"

@saschagrunert
Copy link
Member Author

I like the readability of it better, but I also think we should somehow identify what sigs the PRs come from.

Yeah I agree, so for the markdown we could always add the SIG to every entry. Which would look like this (current state demo of v1.17.0-beta.0 to v1.17.0-beta.1):
https://gist.github.com/saschagrunert/a7c13d8b3e6a45d5c0f0c8f9da1e004b

Also the API Changes section is based on things with kind/api-change so really that label should be ignored under "Other Changes"
It behaves now like this, the only section treated as something special is New Features (see demo link above)

@jeefy
Copy link
Member

jeefy commented Nov 27, 2019

I'm good with that! :)

/lgtm

@saschagrunert Unhold when you're ready.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2019
@justaugustus
Copy link
Member

Explicit hold for consensus check with the community.
I'll draft something later.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2019
@saschagrunert saschagrunert changed the title WIP: Sort release notes by kind instead of SIG Sort release notes by kind instead of SIG Nov 27, 2019
@k8s-ci-robot k8s-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 27, 2019
@saschagrunert saschagrunert changed the title Sort release notes by kind instead of SIG Sort release notes by Kind(s) instead of SIG(s) Dec 11, 2019
@saschagrunert
Copy link
Member Author

I did some fine-adjustments regarding notes having multiple kinds. I also prettify the kind list that they have some sort of Title Case. Updated the first comment to contain the latest demo version.

@justaugustus
Copy link
Member

@saschagrunert -- This is really awesome! One thing I think I'd like to see here is an idea of ranking the kind labels...

Some thoughts:

  • We recently introduced kind/deprecation. I'd expected deprecation notices to be presented at the top.
  • API Changes would probably come next
  • New features
  • ...

When two kind labels are applied, it'd be cool for the one of higher rank to contain the release note e.g., deprecation has higher rank than a cleanup, so the note would land in the "deprecation" section.

It's probably fine to do that as a follow-up though, if we think this is a good idea.

Sent a note to k-dev: https://groups.google.com/d/topic/kubernetes-dev/xwFKQfRWmAc/discussion

We'll merge this Thursday EOD PT, barring any strong objections.
Feel free to lift the hold then.

/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 Jan 13, 2020
@qnetter
Copy link

qnetter commented Jan 13, 2020

Much more useful, to me.

@cartyc
Copy link

cartyc commented Jan 13, 2020

Awesome work @saschagrunert , looks great!

@onlydole
Copy link
Member

SO much better for sure! Thank you for all the hard work, @saschagrunert!

@jeremyrickard
Copy link
Contributor

I think this looks really great @saschagrunert!

I also really like @justaugustus's suggestion that kind/deprecation stuff should be at the top.

+1 from me

@rikatz
Copy link
Contributor

rikatz commented Jan 14, 2020

+1 on moving Deprecation and maybe Breaking Changes to the top.

Amazing job btw :)

@sftim
Copy link
Contributor

sftim commented Jan 14, 2020

Does it make sense to label this for SIG Docs as well as SIG Release?
(because the release notes are part of the documentation)

@saschagrunert
Copy link
Member Author

saschagrunert commented Jan 14, 2020

Some thoughts:

  • We recently introduced kind/deprecation. I'd expected deprecation notices to be presented at the top.
  • API Changes would probably come next
  • New features
  • ...

I added another commit on top of the recent changes, which sort the kinds by their priority.

When two kind labels are applied, it'd be cool for the one of higher rank to contain the release note e.g., deprecation has higher rank than a cleanup, so the note would land in the "deprecation" section.

It's probably fine to do that as a follow-up though, if we think this is a good idea.

I really like that idea and think it would result in a more clean document. So this is part of the latest commit as well. :)

The output of:

> go run cmd/release-notes/main.go --start-rev v1.16.0 --end-rev v1.17.0

now looks like this:
https://gist.github.com/saschagrunert/016041cfe4754f185fb7598e35ad6823
compared to the previous version:
https://gist.github.com/saschagrunert/80385c331a7e97ee408ce4f718594c5a

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2020
@liggitt
Copy link
Member

liggitt commented Jan 14, 2020

I think this provides a much better starting point for our release notes than the current generator. Just a couple thoughts:

  • I'm not sure bug/cleanup/flake are meaningful to end-users... I would probably bucket them together.
  • I think sub-categorization by component is more meaningful to end-users (e.g. changes related to apiserver, scheduler, kubelet, kubectl, kubeadm, etc). I don't think we have sufficient information in the PRs to definitively attribute a PR to a component, so that might be a secondary step for now, but that would help organize some of the massive sections

@neolit123
Copy link
Member

the area labels can be used for partial component distinction. they are applied automatically from OWNERS files.
area/{apiserver|kubectl|kubelet|kubeadm}

we don't have labels for controller-manager and scheduler.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
@saschagrunert
Copy link
Member Author

  • I'm not sure bug/cleanup/flake are meaningful to end-users... I would probably bucket them together.

Okay, I added an option to map kinds, which for now maps bug, cleanup and flake into Other (Bug, Cleanup or Flake).

  • I think sub-categorization by component is more meaningful to end-users (e.g. changes related to apiserver, scheduler, kubelet, kubectl, kubeadm, etc). I don't think we have sufficient information in the PRs to definitively attribute a PR to a component, so that might be a secondary step for now, but that would help organize some of the massive sections

Yes we could do that as a follow up I guess.

I re-generated the notes in the top comment to visualize the latest changes.

@evillgenius75
Copy link

I think this looks great. @saschagrunert, Is this also going to drive changes to the existing Release Note template we currently manually curate before Release day as that template still organizes by SIG in most cases?

I would prefer to see if we can do this all in one shot with just some final prettying of the MD to release.

https://github.com/kubernetes/sig-release/blob/master/release-team/role-handbooks/release-notes/relnotes-template.md

@saschagrunert
Copy link
Member Author

saschagrunert commented Jan 15, 2020

I think this looks great. @saschagrunert, Is this also going to drive changes to the existing Release Note template we currently manually curate before Release day as that template still organizes by SIG in most cases?

I would prefer to see if we can do this all in one shot with just some final prettying of the MD to release.

https://github.com/kubernetes/sig-release/blob/master/release-team/role-handbooks/release-notes/relnotes-template.md

Oh yes, we should update that template as well in my point of view. We could apply the priority of kinds there too and maybe drop the distinction between “Notable” and “Other” changes. 🤔

@puerco
Copy link
Member

puerco commented Jan 17, 2020

@saschagrunert will the format change affect relnotes.k8s.io?

@justaugustus
Copy link
Member

We'll merge this Thursday EOD PT, barring any strong objections.
Feel free to lift the hold then.

Hearing no objections, let's get this merged!
/lgtm
/approve
/hold cancel
/sig docs

@saschagrunert -- Thank you again for your great work on this!
Please capture the review suggestions as a follow up issue.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeefy, justaugustus, saschagrunert

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 merged commit 7ab229a into kubernetes:master Jan 17, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 17, 2020
@saschagrunert
Copy link
Member Author

@saschagrunert will the format change affect relnotes.k8s.io?

No :)

@saschagrunert saschagrunert deleted the kind-vs-sig branch January 17, 2020 07:44
saschagrunert added a commit to saschagrunert/sig-release that referenced this pull request Jan 17, 2020
As a follow-up of kubernetes/release#923, we
should also improve the release notes template to make the work easier
for the release notes team and have a uniform layout of the release
notes.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
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-eng Issues or PRs related to the Release Engineering subproject 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. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.