Merge pull request #735 from weaveworks/695b-include-user-in-git-commit

695b include user in git commit

cmd/fluxctl/args.go

@@ -1,7 +1,10 @@
 package main
 
 import (
-	"os/user"
+	"bytes"
+	"fmt"
+	"os/exec"
+	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -16,11 +19,61 @@ func parseServiceOption(s string) (update.ServiceSpec, error) {
 }
 
 func AddCauseFlags(cmd *cobra.Command, opts *update.Cause) {
-	username := ""
-	user, err := user.Current()
-	if err == nil {
-		username = user.Username
-	}
+	authorInfo := getUserGitconfig()
+	username := getCommitAuthor(authorInfo)
+
 	cmd.Flags().StringVarP(&opts.Message, "message", "m", "", "attach a message to the update")
-	cmd.Flags().StringVar(&opts.User, "user", username, "override the user reported as initating the update")
+	cmd.Flags().StringVar(&opts.User, "user", username, "override the user reported as initiating the update")
+}
+
+func getCommitAuthor(authorInfo map[string]string) string {
+	userName := authorInfo["user.name"]
+	userEmail := authorInfo["user.email"]
+
+	switch {
+	case userName != "" && userEmail != "":
+		return fmt.Sprintf("%s <%s>", userName, userEmail)
+	case userEmail != "":
+		return userEmail
+	case userName != "":
+		return userName
+	}
+	return ""
+}
+
+func getUserGitconfig() map[string]string {
+	var out bytes.Buffer
+	userGitconfig := make(map[string]string)
+	cmd := exec.Command("git", "config", "--list")
+	cmd.Stdout = &out
+
+	err := cmd.Run()
+	if err != nil {
+		return userGitconfig
+	}
+	res := out.String()
+	return userGitconfigMap(res)
+}
+
+func userGitconfigMap(s string) map[string]string {
+	c := make(map[string]string)
+	lines := splitList(s)
+	for _, l := range lines {
+		if l == "" {
+			continue
+		}
+		prop := strings.SplitN(l, "=", 2)
+		p := strings.TrimSpace(prop[0])
+		v := strings.TrimSpace(prop[1])
+		c[p] = v
+	}
+	return c
+}
+
+func splitList(s string) []string {
+	outStr := strings.TrimSpace(s)
+	if outStr == "" {
+		return []string{}
+	}
+	return strings.Split(outStr, "\n")
 }

cmd/fluxctl/args_test.go

@@ -0,0 +1,188 @@
+package main
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestUserGitconfigMap_EmptyString(t *testing.T) {
+	d := ""
+	userGitconfigInfo := userGitconfigMap(d)
+	if len(userGitconfigInfo) != 0 {
+		t.Fatal("expected map with no keys")
+	}
+}
+
+func TestUserGitconfigMap(t *testing.T) {
+	d := `push.default=simple
+	merge.conflictstyle=diff3
+	pull.ff=only
+	core.repositoryformatversion=0
+	core.filemode=true
+	core.bare=false`
+	expected := map[string]string{
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+
+	userGitconfigInfo := userGitconfigMap(d)
+	if len(userGitconfigInfo) != 6 {
+		t.Fatal("got map with unexpected number of keys")
+	}
+	if !reflect.DeepEqual(userGitconfigInfo, expected) {
+		t.Fatal("result does not match expected structure")
+	}
+}
+
+func TestUserGitconfigMap_WithEmptyLines(t *testing.T) {
+	d := `
+	user.name=Jane Doe
+	push.default=simple
+	merge.conflictstyle=diff3
+	pull.ff=only
+
+	core.repositoryformatversion=0
+	core.filemode=true
+	core.bare=false
+
+	`
+	expected := map[string]string{
+		"user.name":                    "Jane Doe",
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+	userGitconfigInfo := userGitconfigMap(d)
+
+	if len(userGitconfigInfo) != 7 {
+		t.Fatal("got map with unexpected number of keys")
+	}
+	if !reflect.DeepEqual(userGitconfigInfo, expected) {
+		t.Fatal("result does not match expected structure")
+	}
+}
+
+func TestUserGitconfigMap_WithNoKeys(t *testing.T) {
+	d := `
+	`
+	expected := make(map[string]string)
+
+	userGitconfigInfo := userGitconfigMap(d)
+	if len(userGitconfigInfo) != 0 {
+		t.Fatal("expected map with no keys")
+	}
+	if !reflect.DeepEqual(userGitconfigInfo, expected) {
+		t.Fatal("result does not match expected structure")
+	}
+}
+
+func TestGetCommitAuthor_BothNameAndEmail(t *testing.T) {
+	input := map[string]string{
+		"user.name":                    "Jane Doe",
+		"user.email":                   "jd@j.d",
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+	expected := "Jane Doe <jd@j.d>"
+	author := getCommitAuthor(input)
+	if author != expected {
+		t.Fatal("author did not match expected value")
+	}
+}
+
+func TestGetCommitAuthor_OnlyName(t *testing.T) {
+	input := map[string]string{
+		"user.name":                    "Jane Doe",
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+	expected := "Jane Doe"
+	author := getCommitAuthor(input)
+	if author != expected {
+		t.Fatal("author did not match expected value")
+	}
+}
+
+func TestGetCommitAuthor_OnlyEmail(t *testing.T) {
+	input := map[string]string{
+		"user.email":                   "jd@j.d",
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+	expected := "jd@j.d"
+	author := getCommitAuthor(input)
+	if author != expected {
+		t.Fatal("author did not match expected value")
+	}
+}
+
+func TestGetCommitAuthor_NoNameNoEmail(t *testing.T) {
+	input := map[string]string{
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+	expected := ""
+	author := getCommitAuthor(input)
+	if author != expected {
+		t.Fatal("author did not match expected value")
+	}
+}
+
+func TestGetCommitAuthor_NameAndEmptyEmail(t *testing.T) {
+	input := map[string]string{
+		"user.name":                    "Jane Doe",
+		"user.email":                   "",
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+	expected := "Jane Doe"
+	author := getCommitAuthor(input)
+	if author != expected {
+		t.Fatal("author did not match expected value")
+	}
+}
+
+func TestGetCommitAuthor_EmailAndEmptyName(t *testing.T) {
+	input := map[string]string{
+		"user.name":                    "",
+		"user.email":                   "jd@j.d",
+		"push.default":                 "simple",
+		"merge.conflictstyle":          "diff3",
+		"pull.ff":                      "only",
+		"core.repositoryformatversion": "0",
+		"core.filemode":                "true",
+		"core.bare":                    "false",
+	}
+	expected := "jd@j.d"
+	author := getCommitAuthor(input)
+	if author != expected {
+		t.Fatal("author did not match expected value")
+	}
+}

cmd/fluxd/main.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/client-go/1.5/rest"
 
 	"context"
+
 	"github.com/weaveworks/flux"
 	"github.com/weaveworks/flux/cluster"
 	"github.com/weaveworks/flux/cluster/kubernetes"
@@ -71,12 +72,13 @@ func main() {
 		kubernetesKubectl = fs.String("kubernetes-kubectl", "", "Optional, explicit path to kubectl tool")
 		versionFlag       = fs.Bool("version", false, "Get version number")
 		// Git repo & key etc.
-		gitURL    = fs.String("git-url", "", "URL of git repo with Kubernetes manifests; e.g., git@github.com:weaveworks/flux-example")
-		gitBranch = fs.String("git-branch", "master", "branch of git repo to use for Kubernetes manifests")
-		gitPath   = fs.String("git-path", "", "path within git repo to locate Kubernetes manifests (relative path)")
-		gitUser   = fs.String("git-user", "Weave Flux", "username to use as git committer")
-		gitEmail  = fs.String("git-email", "support@weave.works", "email to use as git committer")
-		gitLabel  = fs.String("git-label", "", "label to keep track of sync progress; overrides both --git-sync-tag and --git-notes-ref")
+		gitURL       = fs.String("git-url", "", "URL of git repo with Kubernetes manifests; e.g., git@github.com:weaveworks/flux-example")
+		gitBranch    = fs.String("git-branch", "master", "branch of git repo to use for Kubernetes manifests")
+		gitPath      = fs.String("git-path", "", "path within git repo to locate Kubernetes manifests (relative path)")
+		gitUser      = fs.String("git-user", "Weave Flux", "username to use as git committer")
+		gitEmail     = fs.String("git-email", "support@weave.works", "email to use as git committer")
+		gitSetAuthor = fs.Bool("git-set-author", false, "If set, the author of git commits will reflect the user who initiated the commit and will differ from the git committer.")
+		gitLabel     = fs.String("git-label", "", "label to keep track of sync progress; overrides both --git-sync-tag and --git-notes-ref")
 		// Old git config; still used if --git-label is not supplied, but --git-label is preferred.
 		gitSyncTag  = fs.String("git-sync-tag", defaultGitSyncTag, "tag to use to mark sync progress for this cluster")
 		gitNotesRef = fs.String("git-notes-ref", defaultGitNotesRef, "ref to use for keeping commit annotations in git notes")
@@ -360,6 +362,7 @@ func main() {
 			NotesRef:  *gitNotesRef,
 			UserName:  *gitUser,
 			UserEmail: *gitEmail,
+			SetAuthor: *gitSetAuthor,
 		}
 
 		for checkout == nil {
@@ -382,7 +385,8 @@ func main() {
 					"user", *gitUser,
 					"email", *gitEmail,
 					"sync-tag", *gitSyncTag,
-					"notes-ref", *gitNotesRef)
+					"notes-ref", *gitNotesRef,
+					"set-author", *gitSetAuthor)
 				checkout = working
 			}
 		}

daemon/daemon.go

@@ -10,6 +10,7 @@ import (
 	"github.com/pkg/errors"
 
 	"context"
+
 	"github.com/weaveworks/flux"
 	"github.com/weaveworks/flux/cluster"
 	//	fluxerr "github.com/weaveworks/flux/errors"
@@ -252,7 +253,12 @@ func (d *Daemon) updatePolicy(spec update.Spec, updates policy.Updates) DaemonJo
 			return metadata, nil
 		}
 
-		if err := working.CommitAndPush(ctx, policyCommitMessage(updates, spec.Cause), &git.Note{JobID: jobID, Spec: spec}); err != nil {
+		commitAuthor := ""
+		if d.Checkout.Config.SetAuthor {
+			commitAuthor = spec.Cause.User
+		}
+		commitAction := &git.CommitAction{Author: commitAuthor, Message: policyCommitMessage(updates, spec.Cause)}
+		if err := working.CommitAndPush(ctx, commitAction, &git.Note{JobID: jobID, Spec: spec}); err != nil {
 			// On the chance pushing failed because it was not
 			// possible to fast-forward, ask for a sync so the
 			// next attempt is more likely to succeed.
@@ -286,7 +292,12 @@ func (d *Daemon) release(spec update.Spec, c release.Changes) DaemonJobFunc {
 			if commitMsg == "" {
 				commitMsg = c.CommitMessage()
 			}
-			if err := working.CommitAndPush(ctx, commitMsg, &git.Note{JobID: jobID, Spec: spec, Result: result}); err != nil {
+			commitAuthor := ""
+			if d.Checkout.Config.SetAuthor {
+				commitAuthor = spec.Cause.User
+			}
+			commitAction := &git.CommitAction{Author: commitAuthor, Message: commitMsg}
+			if err := working.CommitAndPush(ctx, commitAction, &git.Note{JobID: jobID, Spec: spec, Result: result}); err != nil {
 				// On the chance pushing failed because it was not
 				// possible to fast-forward, ask for a sync so the
 				// next attempt is more likely to succeed.
@@ -315,7 +326,7 @@ func (d *Daemon) SyncNotify() error {
 	return nil
 }
 
-// Ask the daemon how far it's got committing things; in particular, is the job
+// JobStatus - Ask the daemon how far it's got committing things; in particular, is the job
 // queued? running? committed? If it is done, the commit ref is returned.
 func (d *Daemon) JobStatus(jobID job.ID) (job.Status, error) {
 	ctx, cancel := context.WithTimeout(context.Background(), defaultHandlerTimeout)

daemon/loop_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/go-kit/kit/log"
 
 	"context"
+
 	"github.com/weaveworks/flux"
 	"github.com/weaveworks/flux/cluster"
 	"github.com/weaveworks/flux/cluster/kubernetes"
@@ -206,7 +207,9 @@ func TestDoSync_WithNewCommit(t *testing.T) {
 	}); err != nil {
 		t.Fatal(err)
 	}
-	if err := d.Checkout.CommitAndPush(context.Background(), "test commit", nil); err != nil {
+
+	commitAction := &git.CommitAction{Author: "", Message: "test commit"}
+	if err := d.Checkout.CommitAndPush(context.Background(), commitAction, nil); err != nil {
 		t.Fatal(err)
 	}
 	newRevision, err := d.Checkout.HeadRevision(context.Background())