Implement clone, commit and push of release notes draft to user's fork #1102

puerco · 2020-02-16T03:38:53Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This is the first (partial) implementation of the release notes PR automation we have been discussing mainly in #1061 . Specifically, this PR adds a new option (--create-draft-pr) to the krel release-notes subcommand that, when enabled, will generate the release notes draft to a user's fork of k/sig-release. The user's fork must be determined using the --draft-org and --draft-repo flags

I also added a new option (-o|--output-dir) to output the release notes in markdown and json to a directory, mainly for testing purposes.

Which issue(s) this PR fixes:

Relates to #1061

Special notes for your reviewer:
This PR stops short of talking to GitHub and creating the PR. I wanted to touch base first and see if this is going in the right direction.

I added two new functions to pkg/go.git, one to add a remote (Repo.AddRemote(name, owner, repo string)) and another to push to a specific branch (Repo.PushToRemote())

Does this PR introduce a user-facing change?:

Add --create-draft-pr --draft-org and --draft-repo flags to automate the creation of the release notes draft
Add --output-dir to output a copy of the release notes.

/cc @saschagrunert @JamesLaverack @evillgenius75

justaugustus

@puerco -- I've made some suggestions.

Please also squash down these commits.
Each commit should be logically organized with a commit message you'd be okay with showing up in our git history.

cmd/krel/cmd/release_notes.go

justaugustus · 2020-02-16T05:41:35Z

cmd/krel/cmd/release_notes.go

+
+// createDraftPR pushes the release notes draft to the users fork
+func createDraftPR(tag string, result *releaseNotesResult) error {
+	repoparts := strings.Split(releaseNotesOpts.sigReleaseFork, "/")


Actually, we should probably add the validatePROptions() check here instead.
repoparts becomes unnecessary if the flags get split as suggested above.

I chose to call validatePROptions() before generating the notes. i wanted to avoid running the release notes gatherer (which is very expensive timewise) if the PR was going to fail anyway.

justaugustus · 2020-02-16T05:42:08Z

cmd/krel/cmd/release_notes.go

+	targetdir := srrepo.Dir() + fmt.Sprintf("/releases/release-%d.%d", s.Major, s.Minor)
+	logrus.Debugf("Release notes markdown will be written to %s", targetdir)
+
+	// Ugly hack to match repo.Checkout


Why is the ugly hack necessary?

The Commit() function arguments takes a revision and arguments (Checkout(rev string, args ...string)) . I need to pass -b to git but if i send it in the arguments it appends it at the end and it breaks.

justaugustus · 2020-02-16T05:45:05Z

cmd/krel/cmd/release_notes.go

+	}
+
+	logrus.Infof("Pushing release notes draft to %s", releaseNotesOpts.sigReleaseFork)
+	err = ioutil.WriteFile(targetdir+"/release-notes-draft.md", []byte(result.markdown), 0644)


targetdir can probably become the releaseNotesOpts.outputDir suggested above.

Here, do you mean overriding the default location in the repo with --output-dir if defined ?

justaugustus · 2020-02-16T05:48:13Z

pkg/git/git.go

@@ -658,6 +658,17 @@ func (r *Repo) Add(filename string) error {
 	return nil
 }

+// CommitAll Commit all changes using git command
+func (r *Repo) CommitAll(msg string) ([]string, error) {


Is there a reason Commit() doesn't suit our needs here?
If it's the Author field, you can push those values into consts instead.

I resolved this to Add()ing only the release notes files. The reason I was adding a new function was because the Commit functions does not take arguments besides the message.

puerco · 2020-02-17T14:18:29Z

Thanks @justaugustus for such detailed guidance, changes are comming right up.

puerco · 2020-02-18T21:19:37Z

~~ Due to rebase issues I'm scraping this PR in favour of PR #1110 which includes all your suggestions. I'm still addressing the comments here. ~~

Scratch that. Gentleman extraordinaire @JamesLaverack helped me get this in order.

cmd/krel/cmd/release_notes.go

JamesLaverack · 2020-02-19T01:29:17Z

cmd/krel/cmd/release_notes.go

+		if err != nil {
+			return errors.Wrap(err, "Failed to create release notes draft PR")
+		}
+	}


If releaseNotesOpts.createDraftPR is false, because --create-draft-pr was not given, should we log out to say we're skipping it and why? Might help users understand why it's not working if they missed that flag.

I think if do not specify an output option for the release notes we could print out the help screen. There are and will be more options in addition to --create-draft-pr in the subcommand. For example just writing the release notes to files or generating the json version.

cmd/krel/cmd/release_notes.go

saschagrunert

Just some comments from my side 👍

cmd/krel/cmd/release_notes.go

saschagrunert · 2020-02-20T08:27:37Z

cmd/krel/cmd/release_notes.go

+	branchname := "release-notes-draft-" + tag
+
+	// checkout kubernetes/sig-release
+	sigReleaseRepo, err := git.CloneOrOpenGitHubRepo("", "kubernetes", "sig-release", true)


We probably should take the repo path from the options. And use git.DefaultGithubOrg for the org. For the repo we could define a new variable in the git package, like DefaultGithubReleaseRepo

I;ve added a new option to determine the path: sigrelease-fork-path it defaults to os.TempDir(), "k8s-sigrelease". Maybe we can come up with a better name for the flag?

cmd/krel/cmd/release_notes.go

…d other fixes

puerco · 2020-02-20T20:28:06Z

Thanks for your comments @saschagrunert I love the attention you give to these kinds of details in your code. I think I've addressed all of your remarks I'll run a new test in a bit (i've run out of my API quota for the next hour).

saschagrunert

👍 LGTM, let's iterate on this

/lgtm

cmd/krel/cmd/release_notes.go

pkg/git/git.go

saschagrunert

/approve

Thanks for the hard work @puerco 💚

k8s-ci-robot · 2020-02-21T07:57:28Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~cmd/krel/OWNERS~~ [saschagrunert]
~~pkg/OWNERS~~ [saschagrunert]

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 the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 16, 2020

k8s-ci-robot requested review from evillgenius75, JamesLaverack and saschagrunert February 16, 2020 03:38

justaugustus requested changes Feb 16, 2020

View reviewed changes

achempion mentioned this pull request Feb 17, 2020

support multiple requested reviewers, display PRs waiting for you magit/forge#244

Merged

puerco mentioned this pull request Feb 18, 2020

[krel] add options for release notes draft PR automation #1110

Closed

puerco closed this Feb 18, 2020

puerco reopened this Feb 19, 2020

puerco closed this Feb 19, 2020

puerco force-pushed the master branch from 5c9fd4d to cadfbd5 Compare February 19, 2020 00:54

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 19, 2020

puerco reopened this Feb 19, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 19, 2020

JamesLaverack reviewed Feb 19, 2020

View reviewed changes

puerco force-pushed the master branch from edb33af to 4ef9ff4 Compare February 19, 2020 19:19

puerco requested review from JamesLaverack and justaugustus February 19, 2020 19:30

saschagrunert reviewed Feb 20, 2020

View reviewed changes

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

puerco force-pushed the master branch from 4664abf to bee0ef3 Compare February 20, 2020 19:28

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

puerco force-pushed the master branch from bee0ef3 to b2431ee Compare February 20, 2020 19:47

[krel] release-notes add --format and sigrelease-fork-path options an…

4efee45

…d other fixes

puerco force-pushed the master branch from b2431ee to 4efee45 Compare February 20, 2020 20:12

saschagrunert approved these changes Feb 21, 2020

View reviewed changes

cmd/krel/cmd/release_notes.go Show resolved Hide resolved

cmd/krel/cmd/release_notes.go Show resolved Hide resolved

cmd/krel/cmd/release_notes.go Show resolved Hide resolved

pkg/git/git.go Show resolved Hide resolved

k8s-ci-robot assigned saschagrunert Feb 21, 2020

k8s-ci-robot added 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 Feb 21, 2020

saschagrunert approved these changes Feb 21, 2020

View reviewed changes

k8s-ci-robot merged commit cd8150c into kubernetes:master Feb 21, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 21, 2020

This was referenced Feb 21, 2020

Add tests for git.AddRemote and git.PushToRemote #1120

Merged

Default release notes fork repo to 'sig-release' #1121

Merged

Test for available fork remotes before adding them #1122

Merged

Verolop mentioned this pull request Jul 27, 2020

Add a few retries around failure points in release scripting #1353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement clone, commit and push of release notes draft to user's fork #1102

Implement clone, commit and push of release notes draft to user's fork #1102

puerco commented Feb 16, 2020 •

edited

Loading

justaugustus left a comment

justaugustus Feb 16, 2020

puerco Feb 19, 2020

justaugustus Feb 16, 2020

puerco Feb 19, 2020

justaugustus Feb 16, 2020

puerco Feb 19, 2020

justaugustus Feb 16, 2020

puerco Feb 19, 2020

puerco commented Feb 17, 2020

puerco commented Feb 18, 2020 •

edited

Loading

JamesLaverack Feb 19, 2020

puerco Feb 19, 2020

saschagrunert left a comment

saschagrunert Feb 20, 2020

puerco Feb 20, 2020

puerco commented Feb 20, 2020

saschagrunert left a comment

saschagrunert left a comment

k8s-ci-robot commented Feb 21, 2020

Implement clone, commit and push of release notes draft to user's fork #1102

Implement clone, commit and push of release notes draft to user's fork #1102

Conversation

puerco commented Feb 16, 2020 • edited Loading

justaugustus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puerco commented Feb 17, 2020

puerco commented Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puerco commented Feb 20, 2020

saschagrunert left a comment

Choose a reason for hiding this comment

saschagrunert left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 21, 2020

puerco commented Feb 16, 2020 •

edited

Loading

puerco commented Feb 18, 2020 •

edited

Loading