Merge pull request #115 from fluxcd/better-push-message

Better error messages from `git push`

Author: squaremo

controllers/git_error_test.go

@@ -0,0 +1,102 @@
+/*
+Copyright 2020, 2021 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestLibgit2ErrorTidy(t *testing.T) {
+	// this is what GitLab sends if the deploy key doesn't have write access
+	gitlabMessage := `remote: 
+remote: ========================================================================
+remote: 
+remote: This deploy key does not have write access to this project.
+remote: 
+remote: ========================================================================
+remote: 
+`
+	expectedReformat := "remote: This deploy key does not have write access to this project."
+
+	err := errors.New(gitlabMessage)
+	err = libgit2PushError(err)
+	reformattedMessage := err.Error()
+	if reformattedMessage != expectedReformat {
+		t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage)
+	}
+}
+
+func TestLibgit2Multiline(t *testing.T) {
+	// this is a hypothetical error message, in which the useful
+	// content spans more than one line
+	multilineMessage := `remote: 
+remote: ========================================================================
+remote: 
+remote: This deploy key does not have write access to this project.
+remote: You will need to create a new deploy key.
+remote: 
+remote: ========================================================================
+remote: 
+`
+	expectedReformat := "remote: This deploy key does not have write access to this project. You will need to create a new deploy key."
+
+	err := errors.New(multilineMessage)
+	err = libgit2PushError(err)
+	reformattedMessage := err.Error()
+	if reformattedMessage != expectedReformat {
+		t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage)
+	}
+}
+
+func TestLibgit2ErrorUnchanged(t *testing.T) {
+	// this is (roughly) what GitHub sends if the deploy key doesn't have write access
+	regularMessage := `remote: ERROR: deploy key does not have permissions`
+	expectedReformat := regularMessage
+	err := errors.New(regularMessage)
+	err = libgit2PushError(err)
+	reformattedMessage := err.Error()
+	if reformattedMessage != expectedReformat {
+		t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage)
+	}
+}
+
+func TestGoGitErrorReplace(t *testing.T) {
+	// this is what go-git uses as the error message is if the remote
+	// sends a blank first line
+	unknownMessage := `unknown error: remote: `
+	err := errors.New(unknownMessage)
+	err = gogitPushError(err)
+	reformattedMessage := err.Error()
+	if reformattedMessage == unknownMessage {
+		t.Errorf("expected rewritten error, got %q", reformattedMessage)
+	}
+}
+
+func TestGoGitErrorUnchanged(t *testing.T) {
+	// this is (roughly) what GitHub sends if the deploy key doesn't
+	// have write access; go-git passes this on verbatim
+	regularMessage := `remote: ERROR: deploy key does not have write access`
+	expectedReformat := regularMessage
+	err := errors.New(regularMessage)
+	err = gogitPushError(err)
+	reformattedMessage := err.Error()
+	// test that it's been rewritten, without checking the exact content
+	if len(reformattedMessage) > len(expectedReformat) {
+		t.Errorf("expected %q, got %q", expectedReformat, reformattedMessage)
+	}
+}

controllers/imageupdateautomation_controller.go

@@ -409,22 +409,74 @@ func push(ctx context.Context, path string, repo *gogit.Repository, branch strin
 }
 
 func pushGoGit(ctx context.Context, repo *gogit.Repository, access repoAccess) error {
-	return repo.PushContext(ctx, &gogit.PushOptions{
+	err := repo.PushContext(ctx, &gogit.PushOptions{
 		Auth: access.auth.AuthMethod,
 	})
+	return gogitPushError(err)
+}
+
+func gogitPushError(err error) error {
+	if err == nil {
+		return nil
+	}
+	switch strings.TrimSpace(err.Error()) {
+	case "unknown error: remote:":
+		// this unhelpful error arises because go-git takes the first
+		// line of the output on stderr, and for some git providers
+		// (GitLab, at least) the output has a blank line at the
+		// start. The rest of stderr is thrown away, so we can't get
+		// the actual error; but at least we know what was being
+		// attempted, and the likely cause.
+		return fmt.Errorf("push rejected; check git secret has write access")
+	default:
+		return err
+	}
 }
 
 func pushLibgit2(repo *libgit2.Repository, access repoAccess, branch string) error {
 	origin, err := repo.Remotes.Lookup(originRemote)
 	if err != nil {
 		return err
 	}
-	return origin.Push([]string{fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)}, &libgit2.PushOptions{
+	err = origin.Push([]string{fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch)}, &libgit2.PushOptions{
 		RemoteCallbacks: libgit2.RemoteCallbacks{
 			CertificateCheckCallback: access.auth.CertCallback,
 			CredentialsCallback:      access.auth.CredCallback,
 		},
 	})
+	return libgit2PushError(err)
+}
+
+func libgit2PushError(err error) error {
+	if err == nil {
+		return err
+	}
+	// libgit2 returns the whole output from stderr, and we only need
+	// the message. GitLab likes to return a banner, so as an
+	// heuristic, strip any lines that are just "remote:" and spaces
+	// or fencing.
+	msg := err.Error()
+	lines := strings.Split(msg, "\n")
+	if len(lines) == 1 {
+		return err
+	}
+	var b strings.Builder
+	// the following removes the prefix "remote:" from each line; to
+	// retain a bit of fidelity to the original error, start with it.
+	b.WriteString("remote: ")
+
+	var appending bool
+	for _, line := range lines {
+		m := strings.TrimPrefix(line, "remote:")
+		if m = strings.Trim(m, " \t="); m != "" {
+			if appending {
+				b.WriteString(" ")
+			}
+			b.WriteString(m)
+			appending = true
+		}
+	}
+	return errors.New(b.String())
 }
 
 // --- events, metrics