Follow up to PR#1008: Lazy loading of format spec and Sorting Notes #1148

j0n3lson · 2020-03-04T11:23:06Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
This is a follow up to the mentioned PR#1008.

Deprecated:

Document.NotesByKind is deleted
Added deprecation notice for Document.RenderMarkdown which will be deleted once changelog et al are migrated to template rendering.

Fixes:

Remove blank line at end of rendered document.
Adds support for sorting kinds notes in the final rendered document. This brings feature parity with legacy template rendering (e.g. RenderMarkdown())

Design:

Moves validation and reading of template spec from Options into Document and
collocating the reading of the template file with the template parsing.
Validating the template spec (i.e. "go-template:*") at rendering
time is a bit easier.

Testing

Adds golden file based testing for template rendering. Also enhance golden file testing to simulate reading a user template.

Relevant Issues:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

action required: This change soft-deprecates the `--format="markdown"` option which is excepted but will be deprecated soon. The `go-template:default` option is now the default format. Since [pr#1008](https://github.com/kubernetes/release/pull/1008) when this option is given, the internal default template is used anyway.  Additionally `Document.RenderMarkdown()` is soft deprecated as it will be removed in [#1019](https://github.com/kubernetes/release/issues/1019). Finally for API users `Document.NotesByKind` is deprecated.

k8s-ci-robot · 2020-03-04T11:23:15Z

Hi @j0n3lson. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

j0n3lson · 2020-03-04T11:25:23Z

/cc @saschagrunert Here's a follow up to PR#1008 that refactors template rendering and adds additional test. PTAL.

j0n3lson · 2020-03-04T11:25:51Z

/assign @saschagrunert

saschagrunert · 2020-03-04T11:38:03Z

/ok-to-test

cmd/release-notes/README.md

cmd/release-notes/main.go

pkg/notes/options/options.go

pkg/notes/document/document.go

j0n3lson · 2020-03-04T23:58:02Z

@saschagrunert @marpaia

Not sure why this test is failing. Only thing I can think of is that this change does write more files to tempdir in order to get more end-to-end coverage. Do we need to bump up the timeout on the test config?

saschagrunert · 2020-03-05T07:50:30Z

Verify bazel fails, please run ./hack/update-all.sh

--- pkg/notes/document/BUILD.bazel	1970-01-01 00:00:00.000000000 +0000
+++ pkg/notes/document/BUILD.bazel	1970-01-01 00:00:00.000000000 +0000
@@ -22,7 +22,6 @@
     embed = [":go_default_library"],
     deps = [
         "//pkg/notes/options:go_default_library",
-        "@com_github_kr_pretty//:go_default_library",
         "@com_github_stretchr_testify//require:go_default_library",
         "@io_bazel_rules_go//go/tools/bazel:go_default_library",
     ],

pkg/notes/options/options.go

pkg/notes/document/testdata/document_without_downloads.md.golden

saschagrunert · 2020-03-05T12:54:49Z

pkg/notes/document/testdata/document_without_downloads.md.golden

+## Changes by Kind
+
+### Other (Bug, Cleanup or Flake)
+ - This should definitely get you promoted.
+
+### API Change
+ - This might make people sad...or happy.
+
+### Bug
+ - This will likely get you promoted.
+
+### Cleanup
+ - This usually does not get you promoted but it should.
+
+### Deprecation
+ - Delorted.
+
+### Design
+ - Change the world.
+
+### Documentation
+ - There was  a library in Alexandria, Egypt once.
+
+### Failing Test
+ - Please run presubmit test!
+
+### Feature
+ - This will get you promoted.
+
+### Flake


The notes by kind should be sorted by their priority.

This is how I thought about solving for sorting. It's kinda a challenge to sort currently since the template is given a map. We need the kind information for rendering but to preserve order we need a slice (i.e. we loose kind information):

#1154

I thought about other ways to solve this in the template but I wanted to keep the template deadly simple and avoid putting logic (like sorting) in there.
Another way I thought of was just to have a function

See my comments there :)

Okay sgtm. I'll implement that PR, submit, and then submit this.

/hold

For #1154

j0n3lson · 2020-03-07T18:26:40Z

/hold cancel

Let’s follow up with the sorting. :) unfortunately I cannot approve this one.

Are you saying this PR /must/ include the fix for sorting? Or are you saying that other PR needs to be merged /before/ this PR.

Should I combine the sorting into this PR?

saschagrunert · 2020-03-07T20:28:55Z

/hold cancel
Let’s follow up with the sorting. :) unfortunately I cannot approve this one.

Are you saying this PR /must/ include the fix for sorting? Or are you saying that other PR needs to be merged /before/ this PR.

Should I combine the sorting into this PR?

Oh you can decide what you prefer. Since I cannot approve this PR and we cannot merge it now just do it like you want. :)

j0n3lson · 2020-03-09T03:12:46Z

/hold cancel
Let’s follow up with the sorting. :) unfortunately I cannot approve this one.

Are you saying this PR /must/ include the fix for sorting? Or are you saying that other PR needs to be merged /before/ this PR.
Should I combine the sorting into this PR?

Oh you can decide what you prefer. Since I cannot approve this PR and we cannot merge it now just do it like you want. :)

Hey, I've added another commit in this PR. Can you PTAL and let me know :)

saschagrunert

/lgtm

Thank you @j0n3lson 🙏

j0n3lson · 2020-03-09T14:25:16Z

/assign @justaugustus

cpanato

/approve
/lgtm

j0n3lson · 2020-03-11T04:18:46Z

Ping @justaugustus can we merge it?

Fixes: * Fixes new line at the end of rendered templates. Refactoring: * Moves validation and reading of template spec from Options into Document and colocate the reading of the template file with the template parsing. validating the template spec (i.e. "go-template:*") at rendering time is a bit easier than trying to parse/validate a spec that also contains go template actions. * Soft-deprecated FormatSpecMarkdown and Document.RenderMarkdown. The soft deprecation adds a deprecation notice but exludes them from golint until so linting passes (this will be removed in issue #1019). Testing: * Enhance golden file testing to simulate reading a user template. * Enhance golden file testing to cover rendering without downloads. * Enhance testing to provide line-level diffing between between expected and golden files.

Design changes: * Deprecate `Document.NotesByKind` * Introduce `NoteCollection` which is a simple list that can be passed to the template. * Introduce `NoteCategory` which is a container maping kind to a slice of notes.

j0n3lson · 2020-03-14T08:27:59Z

Rebased. Can this be merged? I have a clean up FR pending this one.

saschagrunert · 2020-03-14T08:51:55Z

/lgtm
/approve

k8s-ci-robot · 2020-03-14T08:52:39Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: cpanato, j0n3lson, 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saschagrunert · 2020-03-14T08:54:16Z

@j0n3lson its in. Feel free to follow up on this one. :)

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 4, 2020

k8s-ci-robot requested review from cpanato and marpaia March 4, 2020 11:23

k8s-ci-robot added 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 Mar 4, 2020

k8s-ci-robot assigned saschagrunert Mar 4, 2020

saschagrunert mentioned this pull request Mar 4, 2020

Refactor CreateDownloadsTable to use fetchMetadata #1149

Merged

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 4, 2020

saschagrunert reviewed Mar 4, 2020

View reviewed changes

cmd/release-notes/README.md Show resolved Hide resolved

cmd/release-notes/main.go Outdated Show resolved Hide resolved

pkg/notes/options/options.go Outdated Show resolved Hide resolved

pkg/notes/document/document.go Outdated Show resolved Hide resolved

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 4, 2020

j0n3lson requested a review from saschagrunert March 4, 2020 18:48

saschagrunert reviewed Mar 5, 2020

View reviewed changes

pkg/notes/options/options.go Outdated Show resolved Hide resolved

saschagrunert reviewed Mar 5, 2020

View reviewed changes

pkg/notes/options/options.go Outdated Show resolved Hide resolved

saschagrunert reviewed Mar 5, 2020

View reviewed changes

pkg/notes/document/testdata/document_without_downloads.md.golden Outdated Show resolved Hide resolved

j0n3lson mentioned this pull request Mar 5, 2020

Use a release notes generator urfave/cli#965

Closed

saschagrunert reviewed Mar 5, 2020

View reviewed changes

j0n3lson mentioned this pull request Mar 6, 2020

WIP: Pass a list of NoteCategory to template instead of map of kind t… #1154

Closed

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2020

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 8, 2020

j0n3lson changed the title ~~Follow up to PR#1008: Lazy loading of format spec~~ Follow up to PR#1008: Lazy loading of format spec and Sorting Notes Mar 8, 2020

saschagrunert approved these changes Mar 9, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2020

k8s-ci-robot assigned justaugustus Mar 9, 2020

cpanato approved these changes Mar 10, 2020

View reviewed changes

k8s-ci-robot assigned cpanato Mar 10, 2020

saschagrunert mentioned this pull request Mar 12, 2020

Merge notes client and github client #1178

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2020

j0n3lson added 2 commits March 14, 2020 00:28

Support sorted kinds for template rendering.

1709ca0

Design changes: * Deprecate `Document.NotesByKind` * Introduce `NoteCollection` which is a simple list that can be passed to the template. * Introduce `NoteCategory` which is a container maping kind to a slice of notes.

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 14, 2020

j0n3lson requested review from saschagrunert and cpanato March 14, 2020 08:26

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2020

saschagrunert added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2020

k8s-ci-robot merged commit cda4043 into kubernetes:master Mar 14, 2020

k8s-ci-robot added this to the v1.18 milestone Mar 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up to PR#1008: Lazy loading of format spec and Sorting Notes #1148

Follow up to PR#1008: Lazy loading of format spec and Sorting Notes #1148

j0n3lson commented Mar 4, 2020 •

edited

Loading

k8s-ci-robot commented Mar 4, 2020

j0n3lson commented Mar 4, 2020

j0n3lson commented Mar 4, 2020

saschagrunert commented Mar 4, 2020

j0n3lson commented Mar 4, 2020

saschagrunert commented Mar 5, 2020

saschagrunert Mar 5, 2020

j0n3lson Mar 6, 2020

saschagrunert Mar 6, 2020

j0n3lson Mar 6, 2020

j0n3lson Mar 7, 2020

j0n3lson commented Mar 7, 2020

saschagrunert commented Mar 7, 2020

j0n3lson commented Mar 9, 2020

saschagrunert left a comment

j0n3lson commented Mar 9, 2020

cpanato left a comment

j0n3lson commented Mar 11, 2020

j0n3lson commented Mar 14, 2020

saschagrunert commented Mar 14, 2020

k8s-ci-robot commented Mar 14, 2020

saschagrunert commented Mar 14, 2020

Follow up to PR#1008: Lazy loading of format spec and Sorting Notes #1148

Follow up to PR#1008: Lazy loading of format spec and Sorting Notes #1148

Conversation

j0n3lson commented Mar 4, 2020 • edited Loading

k8s-ci-robot commented Mar 4, 2020

j0n3lson commented Mar 4, 2020

j0n3lson commented Mar 4, 2020

saschagrunert commented Mar 4, 2020

j0n3lson commented Mar 4, 2020

saschagrunert commented Mar 5, 2020

saschagrunert Mar 5, 2020

Choose a reason for hiding this comment

j0n3lson Mar 6, 2020

Choose a reason for hiding this comment

saschagrunert Mar 6, 2020

Choose a reason for hiding this comment

j0n3lson Mar 6, 2020

Choose a reason for hiding this comment

j0n3lson Mar 7, 2020

Choose a reason for hiding this comment

j0n3lson commented Mar 7, 2020

saschagrunert commented Mar 7, 2020

j0n3lson commented Mar 9, 2020

saschagrunert left a comment

Choose a reason for hiding this comment

j0n3lson commented Mar 9, 2020

cpanato left a comment

Choose a reason for hiding this comment

j0n3lson commented Mar 11, 2020

j0n3lson commented Mar 14, 2020

saschagrunert commented Mar 14, 2020

k8s-ci-robot commented Mar 14, 2020

saschagrunert commented Mar 14, 2020

j0n3lson commented Mar 4, 2020 •

edited

Loading