Add commit support for changelog subcommand #1014

saschagrunert · 2020-01-14T11:34:52Z

This adds the ability to modify the master branch and the release branch
to contain the updated markdown changelog. The output dir is now
obsolete since we're directly writing into the repo for the markdown file
and to the local working path for the HTML file.

Demo:

> go run cmd/krel/main.go changelog -t $TOKEN --tars $TARS --tag v1.16.3
INFO Using release branch release-1.16
INFO Using local repository path /tmp/k8s
INFO Generating release notes
...
INFO Generating TOC
INFO Adding new content to changelog file /tmp/k8s/CHANGELOG-1.16.md
INFO Writing single HTML to /home/sascha/go/src/k8s.io/release/CHANGELOG-1.16.html
INFO Using dry mode, which does not modify any remote content
INFO Adding CHANGELOG-1.16.md to repository
INFO Committing changes to master branch in repository
INFO Checking out changelog from master branch
INFO Committing changes to release branch in repository

> cd /tmp/k8s/
> git show HEAD
commit d49546d5b301939dd6dbb6a24876801738f5999e (HEAD -> release-1.16)
Author: Anago GCB <[email protected]>
Date:   Tue Jan 14 12:31:39 2020 +0100

    Update CHANGELOG-1.16.md for v1.16.3

diff --git a/CHANGELOG-1.16.md b/CHANGELOG-1.16.md
index e40275e79b2..cf94f0eba9f 100644
--- a/CHANGELOG-1.16.md
+++ b/CHANGELOG-1.16.md
@@ -1,133 +1,252 @@
 <!-- BEGIN MUNGE: GENERATED_TOC -->
-- [v1.16.4](#v1164)
-  - [Downloads for v1.16.4](#downloads-for-v1164)
+
+- [v1.16.3](#v1163)
+  - [Downloads for v1.16.3](#downloads-for-v1163)
     - [Client Binaries](#client-binaries)
     - [Server Binaries](#server-binaries)
     - [Node Binaries](#node-binaries)
-  - [Changelog since v1.16.3](#changelog-since-v1163)
-    - [Other notable changes](#other-notable-changes)
-- [v1.16.3](#v1163)
-  - [Downloads for v1.16.3](#downloads-for-v1163)
+  - [Changelog since v1.16.2](#changelog-since-v1162)
+    - [Notes from Multiple SIGs](#notes-from-multiple-sigs)
+      - [SIG API Machinery, and SIG Instrumentation](#sig-api-machinery-and-sig-instrumentation)
...

> git show master
commit 3c199939dc2a6b7f0e3d3f7b85fe344b35ca93af (master)
Author: Anago GCB <[email protected]>
Date:   Tue Jan 14 12:31:34 2020 +0100

    Add CHANGELOG-1.16.md for v1.16.3

diff --git a/CHANGELOG-1.16.md b/CHANGELOG-1.16.md
index e40275e79b2..cf94f0eba9f 100644
--- a/CHANGELOG-1.16.md
+++ b/CHANGELOG-1.16.md
@@ -1,133 +1,252 @@
 <!-- BEGIN MUNGE: GENERATED_TOC -->
-- [v1.16.4](#v1164)
-  - [Downloads for v1.16.4](#downloads-for-v1164)
+
+- [v1.16.3](#v1163)
+  - [Downloads for v1.16.3](#downloads-for-v1163)
...

hasheddan · 2020-01-14T14:23:37Z

pkg/git/git.go

@@ -598,3 +607,35 @@ func (r *Repo) tagsForBranch(branch string) ([]string, error) {

 	return strings.Fields(status.Output()), nil
 }
+
+// Add adds a file to the staging area of the repo
+func (r *Repo) Add(filename string) error {


Do you think it would be useful to add tests for Add and Commit? Per usual, I would be fine with them coming in a follow-up PR.

Yes sure, I added three more unit tests on top. I was not completely sure how to test a failing commit (should usually succeed), but I think this is not a critical path.

hasheddan

/lgtm
/hold

hoegaarden · 2020-01-14T15:06:51Z

cmd/krel/cmd/changelog.go

+func pushChanges(repo *git.Repo, branch string, tag semver.Version) error {
+	if !rootOpts.nomock {
+		logrus.Info("Using dry mode, which does not modify any remote content")
+		repo.SetDry()


It seems a bit strange to me that the repo is set to "dryrun mode" only when this more or less random function is called. I'd assume, that all the components are constructed or setup as early with settings like that.

Okay, I moved this part to happen directly after the CloneOrOpenDefaultGitHubRepoSSH().

hoegaarden · 2020-01-14T15:15:33Z

pkg/git/git_test.go

+func TestCheckoutSuccess(t *testing.T) {
+	testRepo := newTestRepo(t)
+	defer testRepo.cleanup(t)
+
+	err := testRepo.sut.Checkout(Master, testRepo.testFileName)
+	require.Nil(t, err)
+}
+
+func TestCheckoutFailureWrongRevision(t *testing.T) {
+	testRepo := newTestRepo(t)
+	defer testRepo.cleanup(t)
+
+	err := testRepo.sut.Checkout("wrong")
+	require.NotNil(t, err)
+}
+
+func TestAddSuccess(t *testing.T) {
+	testRepo := newTestRepo(t)
+	defer testRepo.cleanup(t)
+
+	f, err := ioutil.TempFile(testRepo.sut.Dir(), "test")
+	require.Nil(t, err)
+	require.Nil(t, f.Close())
+
+	err = testRepo.sut.Add(filepath.Base(f.Name()))
+	require.Nil(t, err)
+}
+
+func TestAddFailureWrongPath(t *testing.T) {
+	testRepo := newTestRepo(t)
+	defer testRepo.cleanup(t)
+
+	err := testRepo.sut.Add("wrong")
+	require.NotNil(t, err)
+}
+
+func TestCommitSuccess(t *testing.T) {
+	testRepo := newTestRepo(t)
+	defer testRepo.cleanup(t)
+
+	err := testRepo.sut.Commit("msg")
+	require.Nil(t, err)
+}


As far as I can tell, many of those don't really test much. If I'd change e.g. the commit method to

func (r *Repo) Commit(_ string) error { return nil }

the tests would still pass.

I suggest to either mock stuff you wrap (e.g. the "inner" git.Repository) and assert that certain methods have been called (with certain args, ...) (e.g. the Worktree or the worktree.Commit methods). Alternatively, as those tests are already integration-y tests anyway, assert on the expected end-state (check that a git object has actually been created)

I see your point. Mocking would be a bit hard because git.Repository aka inner is a structure and not a public interface we can mock easily. I'll update the tests to test the end-state

(Probably not for this PR, but we are heading in an interesting direction!)

Mocking would be a bit hard because git.Repository aka inner is a structure and not a public interface

Right now, inner is a struct. But what if you decided it should be an interface (currently) with the methods ResolveRevision, Tags, Remote, Worktree, CommitObject, Head? 😎💥

Yes this sounds good, I can follow up on that 👍
The next step after this PR would be to write some kind of integration test for the tool, probably side-by-side to the executable.

My follow up on this: #1017

This adds the ability to modify the master branch and the release branch to contain the updated markdown changelog. Signed-off-by: Sascha Grunert <[email protected]>

cpanato

/lgtm

k8s-ci-robot · 2020-01-16T18:33:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, hasheddan, 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~~ [hasheddan,saschagrunert]
~~pkg/OWNERS~~ [hasheddan,saschagrunert]

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

justaugustus · 2020-01-17T07:02:35Z

This looks good to go!
/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2020

k8s-ci-robot requested review from hasheddan and hoegaarden January 14, 2020 11:35

k8s-ci-robot added 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 Jan 14, 2020

saschagrunert mentioned this pull request Jan 14, 2020

[krel] Refactor existing bash scripts into go tools #918

Closed

16 tasks

hasheddan reviewed Jan 14, 2020

View reviewed changes

hasheddan approved these changes Jan 14, 2020

View reviewed changes

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

k8s-ci-robot assigned hasheddan Jan 14, 2020

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

hoegaarden reviewed Jan 14, 2020

View reviewed changes

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

Add commit support for changelog subcommand

8f9ba1b

This adds the ability to modify the master branch and the release branch to contain the updated markdown changelog. Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert mentioned this pull request Jan 15, 2020

Split-up git test into unit and integration parts #1017

Merged

cpanato approved these changes Jan 16, 2020

View reviewed changes

k8s-ci-robot assigned cpanato Jan 16, 2020

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

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

k8s-ci-robot merged commit 946bada into kubernetes:master Jan 17, 2020

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

saschagrunert deleted the commit-changes branch January 17, 2020 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add commit support for changelog subcommand #1014

Add commit support for changelog subcommand #1014

Uh oh!

saschagrunert commented Jan 14, 2020

Uh oh!

hasheddan Jan 14, 2020

Uh oh!

saschagrunert Jan 14, 2020 •

edited

Loading

Uh oh!

hasheddan left a comment

Uh oh!

hoegaarden Jan 14, 2020

Uh oh!

saschagrunert Jan 15, 2020

Uh oh!

hoegaarden Jan 14, 2020 •

edited

Loading

Uh oh!

saschagrunert Jan 15, 2020

Uh oh!

hoegaarden Jan 15, 2020

Uh oh!

saschagrunert Jan 15, 2020

Uh oh!

saschagrunert Jan 15, 2020

Uh oh!

cpanato left a comment

Uh oh!

k8s-ci-robot commented Jan 16, 2020

Uh oh!

justaugustus commented Jan 17, 2020

Uh oh!

Uh oh!

Add commit support for changelog subcommand #1014

Add commit support for changelog subcommand #1014

Uh oh!

Conversation

saschagrunert commented Jan 14, 2020

Uh oh!

hasheddan Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

saschagrunert Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasheddan left a comment

Choose a reason for hiding this comment

Uh oh!

hoegaarden Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

saschagrunert Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

hoegaarden Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschagrunert Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

hoegaarden Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

saschagrunert Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

saschagrunert Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 16, 2020

Uh oh!

justaugustus commented Jan 17, 2020

Uh oh!

Uh oh!

saschagrunert Jan 14, 2020 •

edited

Loading

hoegaarden Jan 14, 2020 •

edited

Loading