Add release notes API record feature #1039

saschagrunert · 2020-01-27T15:25:23Z

This patch adds the ability to record the release notes API via two new
release notes command line flags --record <DIR> and --replay <DIR>.

This allows us to create custom test cases and implement changes to the
release notes tool faster without actually having the need to interact
with the GitHub API.

How does it work? A recorded session can be created for example via:

> go run cmd/release-notes/main.go \
    --github-token $TOKEN \
    --branch release-1.16 \
    --start-rev v1.16.2 \
    --end-rev v1.16.3 \
    --output v1.16.3.md \
    --record rec

Now the created directory rec contains all the serialized API calls:

> ls -1 rec | head
GetCommit-0.json
GetCommit-1.json
GetPullRequest-0.json
GetPullRequest-1.json
GetPullRequest-2.json
GetPullRequest-3.json
GetPullRequest-4.json
GetPullRequest-5.json
GetPullRequest-6.json
GetPullRequest-7.json

To replay the session without having the need for a valid token, we can
now invoke:

> go run cmd/release-notes/main.go \
    --replay rec \
    --output v1.16.3-replayed.md

Signed-off-by: Sascha Grunert sgrunert@suse.com

saschagrunert · 2020-01-28T10:32:08Z

Ready for review. This allows us to record the release notes API and replay for later (mocked) integration testing.

PTAL @hoegaarden @hasheddan @cpanato

pkg/notes/options/options.go

hoegaarden

All in all I think this is a great feature!

Yeah I thought it would be not only good for testing, but also for development: If I want to change the layout/parsing of the release notes it would be good to be able to fastly replay the latest recording without having the need for any GitHub API access. WDYT?

👍

Given I understand your approach and proposed feature correctly, my remaining main concerns are:

Nit: naming things an slice them into packages: things like client.Client and options.Options always read weird to me. I think golint would also flag them as "stutter".
The replay client could be way easier and should not mix prod- and test-code (the counterfeiter client), and frankly I can't see any reason why it would need to wrap any client, neither test/fake nor real. Am I missing something?
It feels a bit strange to me that we kinda pass 2 potentially different clients into the gatherer, one directly and one via the options. Is that just me maybe?

pkg/notes/client/recordreplay/replay_client.go

pkg/notes/client/client.go

pkg/notes/options/options.go

hoegaarden · 2020-01-29T11:52:24Z

pkg/notes/client/recordreplay/record_client.go

+	i := 0
+	if j, ok := c.recordState[api]; ok {
+		i = j + 1
+	}
+	c.recordState[api] = i


Why not simply do

c.recordState[api]++

?

You initialise c.recordState in the constructor anyway and the githubNotesRecordClient is not exported and therefore cannot be used by external users anyway.

I need the i later on for the filename :)

pkg/notes/client/recordreplay/record_client.go

pkg/notes/options/options.go

cmd/krel/cmd/changelog.go

pkg/notes/client/recordreplay/record_client.go

saschagrunert · 2020-01-29T13:56:19Z

All in all I think this is a great feature!

Yeah I thought it would be not only good for testing, but also for development: If I want to change the layout/parsing of the release notes it would be good to be able to fastly replay the latest recording without having the need for any GitHub API access. WDYT?

Given I understand your approach and proposed feature correctly, my remaining main concerns are:

Nit: naming things an slice them into packages: things like client.Client and options.Options always read weird to me. I think golint would also flag them as "stutter".

The replay client could be way easier and should not mix prod- and test-code (the counterfeiter client), and frankly I can't see any reason why it would need to wrap any client, neither test/fake nor real. Am I missing something?

It feels a bit strange to me that we kinda pass 2 potentially different clients into the gatherer, one directly and one via the options. Is that just me maybe?

I changed the layout of the packages and addressed most of your review notes. Thanks for the detailed review 🙏 I think we're moving into the right direction. PTAL. :)

saschagrunert · 2020-01-29T15:15:59Z

/test pull-release-cluster-up

hoegaarden

mostly nits, just remove the hold if you don't agree / don't want to address them :)

/lgtm
/approve
/hold

pkg/notes/client/client.go

pkg/notes/options/options.go

hoegaarden · 2020-01-30T09:54:10Z

pkg/notes/client/clientfakes/fake_client.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors.
+Copyright 2020 The Kubernetes Authors.


JFYI: There are a couple of things going on which hopefully help a bit with counterfeiter fakes in future:

Counterfeiter files are generated repo-infra#174

Allow adding headers to generated fakes maxbrunsfeld/counterfeiter#153

Ah cool, awesome! 👍

pkg/notes/client/record.go

This patch adds the ability to record the release notes API via two new release notes command line flags `--record <DIR>` and `--replay <DIR>`. This allows us to create custom test cases and implement changes to the release notes tool faster without actually having the need to interact with the GitHub API. How does it work? A recorded session can be created for example via: ``` > go run cmd/release-notes/main.go \ --github-token $TOKEN \ --branch release-1.16 \ --start-rev v1.16.2 \ --end-rev v1.16.3 \ --output v1.16.3.md \ --record rec ``` Now the created directory `rec` contains all the serialized API calls: ``` > ls -1 rec | head GetCommit-0.json GetCommit-1.json GetPullRequest-0.json GetPullRequest-1.json GetPullRequest-2.json GetPullRequest-3.json GetPullRequest-4.json GetPullRequest-5.json GetPullRequest-6.json GetPullRequest-7.json ``` To replay the session without having the need for a valid token, we can now invoke: ``` > go run cmd/release-notes/main.go \ --replay rec \ --output v1.16.3-replayed.md ``` Signed-off-by: Sascha Grunert <sgrunert@suse.com>

hoegaarden · 2020-01-30T11:27:53Z

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2020-01-30T11:28:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hoegaarden, 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~~ [hoegaarden]

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 27, 2020

k8s-ci-robot requested review from hasheddan and marpaia January 27, 2020 15:25

saschagrunert changed the title ~~Add release notes API record feature~~ WIP: Add release notes API record feature Jan 28, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2020

saschagrunert changed the title ~~WIP: Add release notes API record feature~~ Add release notes API record feature Jan 28, 2020

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 Jan 28, 2020

hoegaarden suggested changes Jan 28, 2020

View reviewed changes

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

hoegaarden suggested changes Jan 29, 2020

View reviewed changes

hoegaarden reviewed Jan 30, 2020

View reviewed changes

k8s-ci-robot assigned hoegaarden Jan 30, 2020

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 30, 2020

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 30, 2020

k8s-ci-robot merged commit 1f5421c into kubernetes:master Jan 30, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 30, 2020

saschagrunert deleted the release-notes-record branch January 30, 2020 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add release notes API record feature #1039

Add release notes API record feature #1039

saschagrunert commented Jan 27, 2020

saschagrunert commented Jan 28, 2020 •

edited

Loading

hoegaarden left a comment

hoegaarden Jan 29, 2020

saschagrunert Jan 29, 2020

saschagrunert commented Jan 29, 2020

saschagrunert commented Jan 29, 2020

hoegaarden left a comment

hoegaarden Jan 30, 2020

saschagrunert Jan 30, 2020

hoegaarden commented Jan 30, 2020

k8s-ci-robot commented Jan 30, 2020

Add release notes API record feature #1039

Add release notes API record feature #1039

Conversation

saschagrunert commented Jan 27, 2020

saschagrunert commented Jan 28, 2020 • edited Loading

hoegaarden left a comment

Choose a reason for hiding this comment

hoegaarden Jan 29, 2020

Choose a reason for hiding this comment

saschagrunert Jan 29, 2020

Choose a reason for hiding this comment

saschagrunert commented Jan 29, 2020

saschagrunert commented Jan 29, 2020

hoegaarden left a comment

Choose a reason for hiding this comment

hoegaarden Jan 30, 2020

Choose a reason for hiding this comment

saschagrunert Jan 30, 2020

Choose a reason for hiding this comment

hoegaarden commented Jan 30, 2020

k8s-ci-robot commented Jan 30, 2020

saschagrunert commented Jan 28, 2020 •

edited

Loading