Golang implementation of release note generating tool #434
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Excellent! Excited to see this so soon. A few quick comments on usage so far:
|
|
Thanks for the comments! I've pushed an update:
|
toolbox/relnotes/main.go
Outdated
| } | ||
|
|
||
| // If githubToken isn't specified in flag, use the GITHUB_TOKEN environment variable | ||
| if *githubToken == "" { |
There was a problem hiding this comment.
Usually we take the path of token file as input and read token from that file.
There was a problem hiding this comment.
@yutongz This is following the convention from existing relnotes script @david-mcmahon
There was a problem hiding this comment.
Flexibility here is good if you'd like to add it. In fact you could just switch the flag to use a file. I don't think anyone would notice or care. $GITHUB_TOKEN is used by the release workflow.
toolbox/util/gitlib.go
Outdated
| // otherwise. | ||
| func IsVer(version string, t string) bool { | ||
| m := make(map[string]string) | ||
| m["release"] = "v(0|[1-9][0-9]*)\\.(0|[1-9][0-9]*)\\.(0|[1-9][0-9]*)(-[a-zA-Z0-9]+)*\\.*(0|[1-9][0-9]*)?" |
There was a problem hiding this comment.
It would be good to add some comments with example string you want to match for each regexp.
| for _, table := range tables { | ||
| result := IsVer(table.v, table.t) | ||
| if result != table.isVer { | ||
| t.Errorf("%v %v: IsVer check failed, want: %v, got: %v", table.v, table.t, table.isVer, result) |
There was a problem hiding this comment.
You may also want to add cases which are expected to fail the matches.
There was a problem hiding this comment.
@yutongz Yes the test has cases that fail the matches.
toolbox/relnotes/main.go
Outdated
|
|
||
| // Bootstrap notes for minor (new branch) releases | ||
| if *full || u.IsVer(releaseTag, "dotzero") { | ||
| draftURL := u.GithubRawURL + *owner + "/features/master/" + *branch + "/release-notes-draft.md" |
toolbox/relnotes/main.go
Outdated
| patchRelease(prFile, startTag, releasePRs, issueMap) | ||
| } | ||
|
|
||
| prFile.Close() |
There was a problem hiding this comment.
You need to use defer functions to handle something like close files, it's a golang trick. Take a look at a example
There was a problem hiding this comment.
@yutongz The problem is that I need to close those files early, for
- opening prFile again and copy the content into mdFile
- running sed on mdFile to htmlize it
- running pandoc on mdFile
I will rearrange main() to be able to call defer functions.
toolbox/relnotes/main.go
Outdated
| log.Printf("failed to generate HTML release note: %s", err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
At the end, you may want to print something like "Successfully generated release-note"
toolbox/relnotes/main.go
Outdated
|
|
||
| // getCIJobStatus runs the script find_green_build and append CI job status to outputFile. | ||
| func getCIJobStatus(outputFile, branch string, htmlize bool) error { | ||
| log.Printf("Adding CI job status (this may take a while)...") |
There was a problem hiding this comment.
I thought it's "getting" instead of "adding"
toolbox/relnotes/main.go
Outdated
|
|
||
| for _, file := range files { | ||
| fn := extractFileName(file) | ||
| sha, _ := u.GetSha256(file) |
toolbox/relnotes/main.go
Outdated
| } | ||
|
|
||
| // extractFileName takes a string and returns the file name after last '/'. | ||
| func extractFileName(filePath string) string { |
There was a problem hiding this comment.
You can use filepath.Base() https://golang.org/pkg/path/filepath/#Base
toolbox/relnotes/main.go
Outdated
| } | ||
|
|
||
| // TODO: find a better way to tell failed response | ||
| if err == nil && (resp.StatusCode == 200 || resp.StatusCode == 304) { |
There was a problem hiding this comment.
@yutongz I saw some times the file was cached and the status code was 304 when I was testing, but maybe that was on the web browser not the program. Not really sure about this one...
There was a problem hiding this comment.
In that case, make sure when you decide it failed, print out the err message or status code. Maybe we can find out some other magic. Actually, you may always make sure your program don't eat err message.
There was a problem hiding this comment.
Status code 304 should not happen here, since we are doing an unconditional GET. I think it's fine to fail if it's anything other than 200.
|
@yutongz Thanks for the detailed review! It's very helpful. I've pushed a commit to address most of the problems. |
|
Would this be a better fit in https://github.com/kubernetes/release repo? (Since we are trying to shrink the main k/k repo) |
|
@dims Yes we are checking this code into the kubernetes/release repo. However it comes with a large vendor directory and there is no guideline about managing Golang dependency in the kubernetes/release (I think there are some guidelines in k/k repo). Is there any suggestion on this? |
|
@roycaihw what are the details behind the ~12 minute data collection times. That's a ~15x increase in runtime. Is there anything we can do to optimize this? |
|
@roycaihw What tool did you use to populate the vendor directory? I didn't see a checked-in metadata file describing dependencies. I tried dep recently and it works pretty well. Would it make sense to avoid checking in |
@david-mcmahon In existing script we use github search API to search for PRs with "release-note" label, but github search API
The 1,000-result limit makes us lose old (but related) PRs (for example when you want to search for "release-note" labelled PRs between v1.5.0..v1.5.2, you may only get PRs after kubernetes/kubernetes#46247 (v1.6)). So we decided to use github list API to fetch all issue&PRs from the repo, which takes ~12 minutes to get 50k issue/PRs (~500 requests). Listing all issues is also good for us to add the new feature, as we don't need to send one request for each issue fixed by one release-note PR. To optimize:
|
@enisoc I didn't use any tool. I just copied the vendor directory (my apologies)
Sounds handy. I will try that. Thanks! |
|
@roycaihw Yes, if there's a way to bound or otherwise restrict by some range (or date), such as using the date range between two tags if tag boundaries are not accepted. |
As a release manager, I can confirm that making this script as fast as possible for common use cases (e.g. looking only at things relevant to the current release) would be greatly appreciated. I often run the current tool to look for PRs whose release notes are wrong or low quality. Then I fix up the PRs and run the tool again to make sure the changes worked as expected. |
|
@david-mcmahon @enisoc Got it. I'm working on using the search API. |
|
@david-mcmahon @enisoc I've pushed an update. Now we can use the search API to get >1,000 results, and the program sleeps for 30 seconds when it hits the rate limit. On my machine it takes 1.5 minutes to run |
|
@enisoc I've pushed an update to use dep and remove vendor/, but I'm not sure how to have bazel run dep commands. Now you have to manually run |
|
@roycaihw Awesome optimization using the search api! No immediate rush on this but can you look at the changes in #437 to make this work for Istio as well? I think your implementation is even closer out the gate than the original relnotes was before #437. At some point if we further generalize this, we should move it out of the kubernetes org, but that can happen after this PR goes in. |
|
@david-mcmahon Sounds good to me. @enisoc @yutongz I've pushed a commit to address some remaining problems on my mind. Please take a look. Let me know when you want me to squash the commits. |
toolbox/util/github.go
Outdated
|
|
||
| // LastReleases looks up the list of releases on github and puts the last release per branch | ||
| // into a branch-indexed dictionary. | ||
| func LastReleases(c *github.Client, owner, repo string) (map[string]string, error) { |
There was a problem hiding this comment.
Based on the OO design, I suggest you make a structure here. You can create something like this https://github.com/istio/test-infra/blob/master/toolbox/util/githubClient.go#L44. And most of functions here can be a method of this structure.
toolbox/util/github.go
Outdated
| // GetCommitDate gets commit time for given tag/commit, provided with repository tags and commits. | ||
| // The function returns ok as false if input tag/commit cannot be found in the repository. | ||
| func GetCommitDate(c *github.Client, owner, repo, tagCommit string, tags []*github.RepositoryTag) (date time.Time, ok bool) { | ||
| // If input string is a tag, convert it into SHA |
There was a problem hiding this comment.
What if you are given both tagCommit and tags. Do you have a perference?
There was a problem hiding this comment.
The input tagCommit can be either a tag or a SHA. The input tags is a list of all the tags in that repo and is used to look up a tag and get the corresponding SHA. This is because github API's git service can only get one commit by SHA (not by tag).
There was a problem hiding this comment.
I know this is your way, which is the proper way to call this function. But what if people call it with the illegal inputs, say giving you both tagCommit and tags, or tags which are not pointing to the same SHA, you need to tell this issue, log out message or/and throw out the error.
You are not the only one who is going to use this function later on, so it's your responsibility to make sure your function will not misbehavior with some wrong input.
toolbox/util/github.go
Outdated
| // The function returns ok as false if input tag/commit cannot be found in the repository. | ||
| func GetCommitDate(c *github.Client, owner, repo, tagCommit string, tags []*github.RepositoryTag) (date time.Time, ok bool) { | ||
| // If input string is a tag, convert it into SHA | ||
| for _, t := range tags { |
There was a problem hiding this comment.
What if those tags are not pointing to the same SHA?
toolbox/relnotes/main.go
Outdated
| return | ||
| } | ||
|
|
||
| func readGithubToken(filename string) string { |
There was a problem hiding this comment.
change it to ReadToken() and put it in one of the util files
toolbox/relnotes/main.go
Outdated
| commitPRs, err := parsePRFromCommit(releaseCommits) | ||
| if err != nil { | ||
| log.Printf("failed to parse release commits: %s", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
You should only use os.Exit() in main(), use return (with/without err)
toolbox/relnotes/main.go
Outdated
| }() | ||
|
|
||
| // Bootstrap notes for minor (new branch) releases | ||
| if *full || u.IsVer(info.releaseTag, "dotzero") { |
toolbox/relnotes/main.go
Outdated
| } | ||
|
|
||
| // TODO: find a better way to tell failed response | ||
| if err == nil && (resp.StatusCode == 200 || resp.StatusCode == 304) { |
There was a problem hiding this comment.
In that case, make sure when you decide it failed, print out the err message or status code. Maybe we can find out some other magic. Actually, you may always make sure your program don't eat err message.
toolbox/relnotes/main.go
Outdated
| } | ||
| f.WriteString("\n") | ||
| } else { | ||
| log.Printf("No draft found - creating generic template...") |
There was a problem hiding this comment.
"No draft found" sounds like the error reason is "There is not such thing." What if it failed because of timeout, or authentication issue? You better say "Failed to do ..." or "Error when ..." plus printing out actual error message you get. Or you can return this error to upper level, and print it out there.
toolbox/relnotes/main.go
Outdated
| } | ||
| } | ||
| f.WriteString("\n") | ||
| } |
There was a problem hiding this comment.
What if err != nil. Should we have some message out?
There was a problem hiding this comment.
Printed out error message/status code
toolbox/relnotes/main.go
Outdated
| } | ||
|
|
||
| // extractReleaseNote tries to fetch release note from PR body, otherwise uses PR title. | ||
| func extractReleaseNote(pr *github.Issue) string { |
There was a problem hiding this comment.
change to name to "extractReleaseNoteFromPR"
|
PTAL |
|
I tried this out with my usual flow, and it LGTM. One caveat to be aware of is that it seems to ignore flags that come after the first positional arg. I think this is a Go stdlib limitation that we can't avoid. We just need to be careful when substituting it in for the script, to make sure positional args come at the end. |
|
LGTM Thanks to make this! |
toolbox/relnotes/main.go
Outdated
|
|
||
| var ( | ||
| // Flags | ||
| // TODO: golang flags and parameters syntex |
toolbox/relnotes/main.go
Outdated
| var err error | ||
| *branch, err = u.GetCurrentBranch() | ||
| if err != nil { | ||
| log.Printf("not a git repository: %s", err) |
There was a problem hiding this comment.
Why is the log leading by "not a git repository"?
There was a problem hiding this comment.
and should we change the placeholder from %s to %v for err in this PR?
There was a problem hiding this comment.
The log "not a git repository" follows the existing shell script. I will change it into "failed to get current branch".
I'm new to Golang so I don't know the convention about placeholder for error. I will make this change.
toolbox/relnotes/main.go
Outdated
| } | ||
|
|
||
| // Generating release note... | ||
| log.Printf("Generating release notes...") |
toolbox/relnotes/main.go
Outdated
| } | ||
|
|
||
| // Start generating markdown file | ||
| log.Printf("Preparing layout...") |
|
|
||
| if *githubToken == "" { | ||
| // If githubToken isn't specified in flag, use the GITHUB_TOKEN environment variable | ||
| *githubToken = os.Getenv("GITHUB_TOKEN") |
There was a problem hiding this comment.
"GITHUB_TOKEN" might be empty, so do we need to check *githubToken == "" again before line 113 ?
There was a problem hiding this comment.
The release note generator doesn't need certain access authentication, but the program will experience bad request rate limit and can fail without Github token. Added check for *githubToken.
toolbox/relnotes/main.go
Outdated
| return nil, fmt.Errorf("failed to parse release commits: %s", err) | ||
| } | ||
|
|
||
| log.Printf("Gathering \"release-note\" labelled PRs using Github search API. This may take a while...") |
toolbox/relnotes/main.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to search release-note labelled PRs: %s", err) | ||
| } | ||
| log.Printf("\"release-note\" labelled PRs gathered.") |
toolbox/relnotes/main.go
Outdated
| } | ||
| log.Printf("\"release-note\" labelled PRs gathered.") | ||
|
|
||
| log.Printf("Gathering \"release-note-action-required\" labelled PRs using Github search API.") |
toolbox/relnotes/main.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("failed to search release-note-action-required labelled PRs: %s", err) | ||
| } | ||
| log.Printf("\"release-note-action-required\" labelled PRs gathered.") |
toolbox/relnotes/main.go
Outdated
| // Bootstrap notes for minor (new branch) releases | ||
| if *full || u.IsVer(info.releaseTag, verDotzero) { | ||
| draftURL := fmt.Sprintf("%s%s/features/master/%s/release-notes-draft.md", u.GithubRawURL, *owner, *branch) | ||
| changelogURL := fmt.Sprintf("%s%s/%s/master/CHANGELOG.md", u.GithubRawURL, *owner, *repo) |
toolbox/relnotes/main.go
Outdated
|
|
||
| // getPendingPRs gets pending PRs on given branch in the repo. | ||
| func getPendingPRs(g *u.GithubClient, f *os.File, owner, repo, branch string) error { | ||
| log.Printf("Getting pending PR status...") |
toolbox/relnotes/main.go
Outdated
| // createHTMLNote generates HTML release note based on the input markdown release note. | ||
| func createHTMLNote(htmlFileName, mdFileName string) error { | ||
| var result error | ||
| log.Printf("Generating HTML release note...") |
toolbox/relnotes/main.go
Outdated
| // before running this function. | ||
| func getCIJobStatus(outputFile, branch string, htmlize bool) error { | ||
| var result error | ||
| log.Printf("Getting CI job status (this may take a while)...") |
toolbox/relnotes/main.go
Outdated
| f.WriteString(content) | ||
| f.WriteString("```\n") | ||
|
|
||
| log.Printf("CI job status fetched.") |
toolbox/relnotes/main.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("failed to create downloads table: %s", err) | ||
| } | ||
| err = createDownloadsTable(f, releaseTag, "Node Binaries", releaseTars+"/kubernetes-node*.tar.gz") |
There was a problem hiding this comment.
May be refactored to use a table-driven for loop for these cases.
toolbox/relnotes/main.go
Outdated
| // minorReleases performs a minor (vX.Y.0) release by fetching the release template and aggregate | ||
| // previous release in series. | ||
| func minorRelease(f *os.File, release, draftURL, changelogURL string) { | ||
|
|
|
|
||
| if *releaseBucket == "kubernetes-release" { | ||
| urlPrefix = k8sReleaseURLPrefix | ||
| } else { |
There was a problem hiding this comment.
What if *releaseBucket == "" ?
There was a problem hiding this comment.
The flag has default value "kubernetes-release" and user can specify alternative google storage bucket to point to in generated notes. The download-table-creation logic is Kubernetes-specified and doesn't apply to other projects yet.
There was a problem hiding this comment.
What if user specifies the flag as ""? We will get https://storage.googleapis.com//release
There was a problem hiding this comment.
Yes, I think it only affect the URL text displayed in generated notes (the program doesn't actually try to fetch resources from that URL), and it's user's responsibility to provide correct address. I don't know if there are cases other than https://dl.k8s.io and https://storage.googleapis.com/<some bucket>/release. In that case user may manually modify the generated text.
I feel like we should keep the program running even if user specifies the flag as "". We can have the program log an error message when *releaseBucket == "", but it won't cover cases if user specifies some invalid non-existing google storage bucket.
| // Regexp Example: | ||
| // Assume the release tag is v1.7.0, this regexp matches "- [v1.7.0-" in | ||
| // "- [v1.7.0-rc.1](#v170-rc1)" | ||
| // "- [v1.7.0-beta.2](#v170-beta2)" |
There was a problem hiding this comment.
also alpha version something like "- v1.7.0-alpha.3", right?
| } | ||
| f.WriteString("\n") | ||
| } | ||
|
|
There was a problem hiding this comment.
I also see in the CHANGELOG we have a section Deprecations. How is it generated? I guess we may want a new release-note-deprecation ?
There was a problem hiding this comment.
I think it's from the release note drafts in kubernetes/features repo. I don't know how are the drafts generated though @david-mcmahon
There was a problem hiding this comment.
Release Notes for new minor (x.y.0) releases are populated from their respective feature branch repos, such as https://github.com/kubernetes/features/blob/master/release-1.7/release-notes-draft.md. These files are updated throughout the release cycle leading up to the .0 release.
There was a problem hiding this comment.
A new label release-note-deprecation will be helpful to auto-generate the section Deprecations. The items in this section are from the release-note-action-required PRs and then trriaged to this section manually I guess.
| if note := re.FindStringSubmatch(*pr.Body); note != nil { | ||
| return note[1] | ||
| } | ||
| return *pr.Title |
There was a problem hiding this comment.
do we still have this case now? i.e., a PR has a release-note label but without release-note block in PR body? @cjwagner
There was a problem hiding this comment.
If the PR has the release-note label then there must be a release-note block. The only release-note-* label that does not require a release-note block is release-note-none.
There was a problem hiding this comment.
I find an example PR which has the release-note label but without a release-note block: kubernetes/kubernetes#54007 (comment)
The author (he is a repo maintainer) manually added the release-note label...
So the code here is correct...
| // https://github.com/kubernetes/kubernetes/blob/master/.github/PULL_REQUEST_TEMPLATE.md | ||
| re, _ := regexp.Compile("```release-note\r\n(.+)\r\n```") | ||
| if note := re.FindStringSubmatch(*pr.Body); note != nil { | ||
| return note[1] |
There was a problem hiding this comment.
We may also want to exclude some corner cases such as a PR with release note NONE. (the PR will get a trelease-note label). I saw many such cases in changelogs. Yeah maybe it's label plugin's responsibility.
There was a problem hiding this comment.
I send a PR kubernetes/test-infra#5116.
@roycaihw @cjwagner
|
@roycaihw does this PR update TOC? I don't find associated code here. I might miss something... |
|
Another thing, we may also need to update https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG.md. Below is its content. For example, when we release 1.9, we need to change Development releases:Current release:Older releases: |
@xiangpengzhao which table do you mean? This tool collects release note and generates markdown/HTML file locally (e.g. at /tmp/release-notes-release-1.9.md). Updating the Github repo is out of this tool's scope. For the |
|
@roycaihw I believe @xiangpengzhao is referring to the call to common::mdtoc(). I haven't verified this myself either in the current implementation. |
|
yeah @david-mcmahon mentioned is what I meant :) |
@david-mcmahon Updated. |
|
I ran There might be two nits here:
RESULT BELOWcloud@ubuntu:/tmp$ cat release-notes-release-1.7.md Branch v1.7.2Downloads for Branch v1.7.2I omitted Changelog since v1.7.0Other notable changesI omitted PENDING PRs on the release-1.7 branch
*I omitted State of release-1.7 branchNOT READY Details |
@xiangpengzhao The prefix "Branch " shows up if the tool runs in preview mode. This follows the existing tool's behavior. |
|
@roycaihw thanks for clarification! |
|
@roycaihw is this PR ready to merge? |
This is a duplication of the shell script "relnotes". Commits have been squashed. Original commits can be found at https://github.com/roycaihw/release/tree/relnotes-dev
roycaihw
force-pushed
the
relnotes-release
branch
from
a5e6e71
to
a126e88
Compare
@xiangpengzhao @david-mcmahon I've squashed the commits. The original commits can be found here. This PR is ready to merge. |
|
@roycaihw will this go version release note tool be invoked by anago the same way as the script version tool is invoked? |
@xiangpengzhao Yes, it supports the same flags. The only difference is as @enisoc pointed out:
|
|
As the new tool is golang and needs to be built during the execution of |
|
@xiangpengzhao Are you ready to give an official LGTM for this? @david-mcmahon After this merges, I can try to work on something. Are we allowed to invoke docker in all the various environments in which anago runs? |
|
@enisoc depends on your definition of "invoke". We explicitly have to work around running docker-in-docker to operate within the containerized GCB environment to build k8s there. Rather than do this, we had to run outside of the build-image. |
k8s-ci-robot
added
the
lgtm
|
I'm merging this now, but I think there's no hurry to get this integrated in anago. My priority is just to be able to run it manually with the hierarchical changes, so we can use it as a starting point for the hand-written release notes. |
|
@enisoc agreed. |
Add 1.14 Lead Shadows
To better produce human-readable release note for Kubernetes. We proposed a hierarchical release note (designe doc shared with kubernetes-dev@ mailing list) design.
The implementation is divided into two PRs for easier review & test. This PR contains a Golang implementation of existing release note generating tool relnotes. The second PR focus on the new hierarchical feature and can be reviewed after this one is merged.
Notes for reviewer:
This PR contains three commits. The first two commits add vendor dependency and bazel build rule changes for Golang. The third commit focus on the implementation.
Unit tests are implemented for util functions and main function.
To build, run:
dep ensurebazel run //:gazellebazel build toolbox/relnotes:relnotesSome e2e tests against the existing shell script relnotes (assume currently in a kubernetes repo):
i)
On branch release-1.7 (run
make quick-releaseto generate the release tarballs):Golang program (the program takes about 15 minutes to run):
../release/bazel-bin/toolbox/relnotes/relnotes --preview --htmlize-md --html-file /tmp/release-note-html-testfile --release-tars=_output/release-tars v1.7.0..v1.7.2Script program:
../release/relnotes v1.7.0..v1.7.2 --preview --htmlize-md --html-file=/tmp/relnotes-testfile.html --release-tars=_output/release-tarsii)
On branch release-1.7:
Golang program:
../release/bazel-bin/toolbox/relnotes/relnotes --preview --html-file /tmp/release-note-html-testfile --release-tars=_output/release-tars v1.7.0..v1.7.0Script program:
../release/relnotes v1.7.0..v1.7.0 --preview --html-file=/tmp/relnotes-testfile.html --release-tars=_output/release-tarsiii)
On branch release-1.6.3:
Golang program:
../release/bazel-bin/toolbox/relnotes/relnotes --html-file /tmp/release-note-html-testfile --fullScript program:
../release/relnotes --html-file=/tmp/relnotes-testfile.html --fullTo compare the output:
vimdiff /tmp/relnotes-release-1.7.md /tmp/release-notes-release-1.7.mdvimdiff /tmp/relnotes-release-1.6.3.md /tmp/release-notes-release-1.6.3.mdvimdiff /tmp/release-note-html-testfile /tmp/relnotes-testfile.html