From 259a03cf49bc95c5a3e18df59fb225b3ab3a8da0 Mon Sep 17 00:00:00 2001 From: Tamara Kaufler Date: Wed, 13 Sep 2017 15:37:42 +0100 Subject: [PATCH] Response to code review comments: refactors getting author logic --- cmd/fluxctl/args.go | 42 ++++----- cmd/fluxctl/args_test.go | 189 +++++++++++++++++++++++---------------- git/operations.go | 6 -- 3 files changed, 128 insertions(+), 109 deletions(-) diff --git a/cmd/fluxctl/args.go b/cmd/fluxctl/args.go index 9ba9a15a55..52678874f1 100644 --- a/cmd/fluxctl/args.go +++ b/cmd/fluxctl/args.go @@ -19,36 +19,27 @@ func parseServiceOption(s string) (update.ServiceSpec, error) { } func AddCauseFlags(cmd *cobra.Command, opts *update.Cause) { - username := getCommitAuthor() + 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 initiating the update") } -func getCommitAuthor() string { - authorInfo := getUserGitconfig() - if userName, ok := authorInfo["user.name"]; ok { - switch userName { - case "": - return getUserEmail(authorInfo) - default: - if userEmail, ok := authorInfo["user.email"]; ok { - if userEmail == "" { - return userName - } - return fmt.Sprintf("%s <%s>", userName, userEmail) - } - } - } - return getUserEmail(authorInfo) -} +func getCommitAuthor(authorInfo map[string]string) string { + userName := authorInfo["user.name"] + userEmail := authorInfo["user.email"] -func getUserEmail(gitconfig map[string]string) string { - if userEmail, ok := gitconfig["user.email"]; ok { + switch userName { + case "": + if userEmail != "" { + return userEmail + } + default: if userEmail == "" { - return "" + return userName } - return userEmail + return fmt.Sprintf("%s <%s>", userName, userEmail) } return "" } @@ -71,12 +62,13 @@ 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]) - if p == "user.name" || p == "user.email" { - c[p] = v - } + c[p] = v } return c } diff --git a/cmd/fluxctl/args_test.go b/cmd/fluxctl/args_test.go index 159b146fb1..93a6735365 100644 --- a/cmd/fluxctl/args_test.go +++ b/cmd/fluxctl/args_test.go @@ -12,125 +12,158 @@ func TestUserGitconfigMap_EmptyString(t *testing.T) { t.Fatal("expected no content") } } -func TestUserGitconfigMap_StringWithoutKeys(t *testing.T) { - d := ` - push.default=simple +func TestUserGitconfigMap(t *testing.T) { + d := `push.default=simple merge.conflictstyle=diff3 pull.ff=only core.repositoryformatversion=0 core.filemode=true - core.bare=false - ` + core.bare=false` + userGitconfigInfo := userGitconfigMap(d) - if len(userGitconfigInfo) != 0 { + if len(userGitconfigInfo) != 6 { t.Fatal("expected no content") } } -func TestUserGitconfigMap_BothKeys(t *testing.T) { +func TestUserGitconfigMap_WithEmptyLines(t *testing.T) { d := ` - user.email=jd@w.ws user.name=Jane Doe push.default=simple merge.conflictstyle=diff3 pull.ff=only - core.repositoryformatversion=0 - core.filemode=true - core.bare=false - ` - expected := make(map[string]string) - expected["user.email"] = "jd@w.ws" - expected["user.name"] = "Jane Doe" - userGitconfigInfo := userGitconfigMap(d) - - if len(userGitconfigInfo) != 2 { - t.Fatal("expected map with 2 keys") - } - if !reflect.DeepEqual(userGitconfigInfo, expected) { - t.Fatal("result does not match expected structure") - } -} -func TestUserGitconfigMap_OnlyNameKey(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 := make(map[string]string) - expected["user.name"] = "Jane Doe" + 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) != 1 { + if len(userGitconfigInfo) != 7 { t.Fatal("expected map with 1 key") } if !reflect.DeepEqual(userGitconfigInfo, expected) { t.Fatal("result does not match expected structure") } } -func TestUserGitconfigMap_OnlyNameKeyAndEmpty(t *testing.T) { +func TestUserGitconfigMap_WithNoKeys(t *testing.T) { d := ` - user.name= - push.default=simple - merge.conflictstyle=diff3 - pull.ff=only - core.repositoryformatversion=0 - core.filemode=true - core.bare=false ` expected := make(map[string]string) - expected["user.name"] = "" userGitconfigInfo := userGitconfigMap(d) - if len(userGitconfigInfo) != 1 { + if len(userGitconfigInfo) != 0 { t.Fatal("expected map with one key") } if !reflect.DeepEqual(userGitconfigInfo, expected) { t.Fatal("result does not match expected structure") } } -func TestUserGitconfigMap_OnlyEmailKey(t *testing.T) { - d := ` - user.email=jd@a.b - push.default=simple - merge.conflictstyle=diff3 - pull.ff=only - core.repositoryformatversion=0 - core.filemode=true - core.bare=false - ` - expected := make(map[string]string) - expected["user.email"] = "jd@a.b" - userGitconfigInfo := userGitconfigMap(d) - - if len(userGitconfigInfo) != 1 { - t.Fatal("expected map with 1 key") +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", } - if !reflect.DeepEqual(userGitconfigInfo, expected) { - t.Fatal("result does not match expected structure") + expected := "Jane Doe " + 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 TestUserGitconfigMap_OnlyEmailKeyAndEmpty(t *testing.T) { - d := ` - user.email= - push.default=simple - merge.conflictstyle=diff3 - pull.ff=only - core.repositoryformatversion=0 - core.filemode=true - core.bare=false - ` - expected := make(map[string]string) - expected["user.email"] = "" - userGitconfigInfo := userGitconfigMap(d) - if len(userGitconfigInfo) != 1 { - t.Fatal("expected map with one key") +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", } - if !reflect.DeepEqual(userGitconfigInfo, expected) { - t.Fatal("result does not match expected structure") + 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") } } diff --git a/git/operations.go b/git/operations.go index 75446463be..a0fe943437 100644 --- a/git/operations.go +++ b/git/operations.go @@ -43,12 +43,6 @@ func clone(ctx context.Context, workingDir string, keyRing ssh.KeyRing, repoURL, } func commit(ctx context.Context, workingDir string, commitAction *CommitAction) error { - // The user in the update.Cause.User must be a valid github user, otherwise the commit fails - // Hardcoding of the user to ignore is not ideal. If there is only one to ignore, ie test@test, - // then this is just about ok. Otherwise there could be config file with stored users to ignore etc. - // Currently the committer is hardcoded to flux-bot for user requested actions. - // TODO (potentially): allow the users to set the committer, ie have an additional - // update.Cause.Committer field. commitAuthor := commitAction.Author if commitAuthor != "" { if err := execGitCmd(ctx,