Merge branch 'ashmckenzie/make-console-messages-consistent' into 'master'

Make console messages consistent

See merge request gitlab-org/gitlab-shell!334

13 files changed, 310 insertions, 31 deletions

diff --git a/cmd/gitlab-shell-authorized-keys-check/main.go b/cmd/gitlab-shell-authorized-keys-check/main.go
index 6b52181..8cc0bc8 100644
--- a/cmd/gitlab-shell-authorized-keys-check/main.go
+++ b/cmd/gitlab-shell-authorized-keys-check/main.go
@@ -7,6 +7,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/console"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/executable"
 )
 
@@ -38,7 +39,7 @@ func main() {
 	}
 
 	if err = cmd.Execute(); err != nil {
-		fmt.Fprintf(readWriter.ErrOut, "%v\n", err)
+		console.DisplayWarningMessage(err.Error(), readWriter.ErrOut)
 		os.Exit(1)
 	}
 }
diff --git a/cmd/gitlab-shell-authorized-principals-check/main.go b/cmd/gitlab-shell-authorized-principals-check/main.go
index 645ccf0..328e11f 100644
--- a/cmd/gitlab-shell-authorized-principals-check/main.go
+++ b/cmd/gitlab-shell-authorized-principals-check/main.go
@@ -7,6 +7,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/console"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/executable"
 )
 
@@ -38,7 +39,7 @@ func main() {
 	}
 
 	if err = cmd.Execute(); err != nil {
-		fmt.Fprintf(readWriter.ErrOut, "%v\n", err)
+		console.DisplayWarningMessage(err.Error(), readWriter.ErrOut)
 		os.Exit(1)
 	}
 }
diff --git a/cmd/gitlab-shell/main.go b/cmd/gitlab-shell/main.go
index 148c652..7751e4d 100644
--- a/cmd/gitlab-shell/main.go
+++ b/cmd/gitlab-shell/main.go
@@ -7,6 +7,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/console"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/executable"
 )
 
@@ -38,7 +39,7 @@ func main() {
 	}
 
 	if err = cmd.Execute(); err != nil {
-		fmt.Fprintf(readWriter.ErrOut, "%v\n", err)
+		console.DisplayWarningMessage(err.Error(), readWriter.ErrOut)
 		os.Exit(1)
 	}
 }
diff --git a/internal/command/lfsauthenticate/lfsauthenticate_test.go b/internal/command/lfsauthenticate/lfsauthenticate_test.go
index f2ccc20..22e151a 100644
--- a/internal/command/lfsauthenticate/lfsauthenticate_test.go
+++ b/internal/command/lfsauthenticate/lfsauthenticate_test.go
@@ -31,12 +31,12 @@ func TestFailedRequests(t *testing.T) {
 		{
 			desc:           "With missing arguments",
 			arguments:      &commandargs.Shell{},
-			expectedOutput: "> GitLab: Disallowed command",
+			expectedOutput: "Disallowed command",
 		},
 		{
 			desc:           "With disallowed command",
 			arguments:      &commandargs.Shell{GitlabKeyId: "1", SshArgs: []string{"git-lfs-authenticate", "group/repo", "unknown"}},
-			expectedOutput: "> GitLab: Disallowed command",
+			expectedOutput: "Disallowed command",
 		},
 		{
 			desc:           "With disallowed user",
diff --git a/internal/command/receivepack/customaction.go b/internal/command/receivepack/customaction.go
index c94ae4c..7575ee9 100644
--- a/internal/command/receivepack/customaction.go
+++ b/internal/command/receivepack/customaction.go
@@ -3,12 +3,13 @@ package receivepack
 import (
 	"bytes"
 	"errors"
-	"fmt"
+
 	"io"
 	"io/ioutil"
 	"net/http"
 	"strings"
 
+	"gitlab.com/gitlab-org/gitlab-shell/internal/console"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/accessverifier"
 )
@@ -32,19 +33,11 @@ func (c *Command) processCustomAction(response *accessverifier.Response) error {
 		return errors.New("Custom action error: Empty API endpoints")
 	}
 
-	c.displayInfoMessage(data.InfoMessage)
+	console.DisplayInfoMessages(strings.Split(data.InfoMessage, "\n"), c.ReadWriter.ErrOut)
 
 	return c.processApiEndpoints(response)
 }
 
-func (c *Command) displayInfoMessage(infoMessage string) {
-	messages := strings.Split(infoMessage, "\n")
-
-	for _, msg := range messages {
-		fmt.Fprintf(c.ReadWriter.ErrOut, "> GitLab: %v\n", msg)
-	}
-}
-
 func (c *Command) processApiEndpoints(response *accessverifier.Response) error {
 	client, err := gitlabnet.GetClient(c.Config)
 
diff --git a/internal/command/receivepack/customaction_test.go b/internal/command/receivepack/customaction_test.go
index 2a4a718..11e7dce 100644
--- a/internal/command/receivepack/customaction_test.go
+++ b/internal/command/receivepack/customaction_test.go
@@ -100,6 +100,6 @@ func TestCustomReceivePack(t *testing.T) {
 
 	// expect printing of info message, "custom" string from the first request
 	// and "output" string from the second request
-	require.Equal(t, "> GitLab: info_message\n> GitLab: one more message\n", errBuf.String())
+	require.Equal(t, "remote: \nremote: info_message\nremote: one more message\nremote: \n", errBuf.String())
 	require.Equal(t, "customoutput", outBuf.String())
 }
diff --git a/internal/command/shared/accessverifier/accessverifier.go b/internal/command/shared/accessverifier/accessverifier.go
index 3aaf98d..5d2d709 100644
--- a/internal/command/shared/accessverifier/accessverifier.go
+++ b/internal/command/shared/accessverifier/accessverifier.go
@@ -2,11 +2,11 @@ package accessverifier
 
 import (
 	"errors"
-	"fmt"
 
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/internal/console"
 	"gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/accessverifier"
 )
 
@@ -39,7 +39,5 @@ func (c *Command) Verify(action commandargs.CommandType, repo string) (*Response
 }
 
 func (c *Command) displayConsoleMessages(messages []string) {
-	for _, msg := range messages {
-		fmt.Fprintf(c.ReadWriter.ErrOut, "> GitLab: %v\n", msg)
-	}
+	console.DisplayInfoMessages(messages, c.ReadWriter.ErrOut)
 }
diff --git a/internal/command/shared/accessverifier/accessverifier_test.go b/internal/command/shared/accessverifier/accessverifier_test.go
index 39c2a66..cfcf4a8 100644
--- a/internal/command/shared/accessverifier/accessverifier_test.go
+++ b/internal/command/shared/accessverifier/accessverifier_test.go
@@ -77,6 +77,6 @@ func TestConsoleMessages(t *testing.T) {
 	cmd.Args = &commandargs.Shell{GitlabKeyId: "1"}
 	cmd.Verify(action, repo)
 
-	require.Equal(t, "> GitLab: console\n> GitLab: message\n", errBuf.String())
+	require.Equal(t, "remote: \nremote: console\nremote: message\nremote: \n", errBuf.String())
 	require.Empty(t, outBuf.String())
 }
diff --git a/internal/command/shared/disallowedcommand/disallowedcommand.go b/internal/command/shared/disallowedcommand/disallowedcommand.go
index 3c98bcc..794944f 100644
--- a/internal/command/shared/disallowedcommand/disallowedcommand.go
+++ b/internal/command/shared/disallowedcommand/disallowedcommand.go
@@ -3,5 +3,5 @@ package disallowedcommand
 import "errors"
 
 var (
-	Error = errors.New("> GitLab: Disallowed command")
+	Error = errors.New("Disallowed command")
 )
diff --git a/internal/console/console.go b/internal/console/console.go
new file mode 100644
index 0000000..72f80cc
--- /dev/null
+++ b/internal/console/console.go
@@ -0,0 +1,73 @@
+package console
+
+import (
+	"fmt"
+	"io"
+	"strings"
+)
+
+func DisplayWarningMessage(message string, out io.Writer) {
+	DisplayWarningMessages([]string{message}, out)
+}
+
+func DisplayInfoMessage(message string, out io.Writer) {
+	DisplayInfoMessages([]string{message}, out)
+}
+
+func DisplayWarningMessages(messages []string, out io.Writer) {
+	DisplayMessages(messages, out, true)
+}
+
+func DisplayInfoMessages(messages []string, out io.Writer) {
+	DisplayMessages(messages, out, false)
+}
+
+func DisplayMessages(messages []string, out io.Writer, displayDivider bool) {
+	if noMessages(messages) {
+		return
+	}
+
+	displayBlankLineOrDivider(out, displayDivider)
+
+	for _, msg := range messages {
+		fmt.Fprintf(out, formatLine(msg))
+	}
+
+	displayBlankLineOrDivider(out, displayDivider)
+}
+
+func noMessages(messages []string) bool {
+	if len(messages) == 0 {
+		return true
+	}
+
+	for _, msg := range messages {
+		if len(strings.TrimSpace(msg)) > 0 {
+			return false
+		}
+	}
+
+	return true
+}
+
+func formatLine(message string) string {
+	return fmt.Sprintf("remote: %v\n", message)
+}
+
+func displayBlankLineOrDivider(out io.Writer, displayDivider bool) {
+	if displayDivider {
+		fmt.Fprintf(out, divider())
+	} else {
+		fmt.Fprintf(out, blankLine())
+	}
+}
+
+func blankLine() string {
+	return formatLine("")
+}
+
+func divider() string {
+	ruler := strings.Repeat("=", 72)
+
+	return fmt.Sprintf("%v%v%v", blankLine(), formatLine(ruler), blankLine())
+}
diff --git a/internal/console/console_test.go b/internal/console/console_test.go
new file mode 100644
index 0000000..bcd8444
--- /dev/null
+++ b/internal/console/console_test.go
@@ -0,0 +1,201 @@
+package console
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestDisplayWarningMessage(t *testing.T) {
+	tests := []struct {
+		name    string
+		message string
+		wantOut string
+	}{
+		{
+			name:    "empty",
+			message: "",
+			wantOut: "",
+		},
+		{
+			name:    "basically empty",
+			message: " ",
+			wantOut: "",
+		},
+		{
+			name:    "something",
+			message: "something",
+			wantOut: `remote: 
+remote: ========================================================================
+remote: 
+remote: something
+remote: 
+remote: ========================================================================
+remote: 
+`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			out := &bytes.Buffer{}
+			DisplayWarningMessage(tt.message, out)
+
+			require.Equal(t, tt.wantOut, out.String())
+		})
+	}
+}
+
+func TestDisplayWarningMessages(t *testing.T) {
+	tests := []struct {
+		name     string
+		messages []string
+		wantOut  string
+	}{
+		{
+			name:     "empty",
+			messages: []string{""},
+			wantOut:  "",
+		},
+		{
+			name:     "basically empty",
+			messages: []string{" "},
+			wantOut:  "",
+		},
+		{
+			name:     "something",
+			messages: []string{"something", "here"},
+			wantOut: `remote: 
+remote: ========================================================================
+remote: 
+remote: something
+remote: here
+remote: 
+remote: ========================================================================
+remote: 
+`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			out := &bytes.Buffer{}
+			DisplayWarningMessages(tt.messages, out)
+
+			require.Equal(t, tt.wantOut, out.String())
+		})
+	}
+}
+
+func TestDisplayInfoMessage(t *testing.T) {
+	tests := []struct {
+		name    string
+		message string
+		wantOut string
+	}{
+		{
+			name:    "empty",
+			message: "",
+			wantOut: "",
+		},
+		{
+			name:    "basically empty",
+			message: " ",
+			wantOut: "",
+		},
+		{
+			name:    "something",
+			message: "something",
+			wantOut: `remote: 
+remote: something
+remote: 
+`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			out := &bytes.Buffer{}
+			DisplayInfoMessage(tt.message, out)
+
+			require.Equal(t, tt.wantOut, out.String())
+		})
+	}
+}
+
+func TestDisplayInfoMessages(t *testing.T) {
+	tests := []struct {
+		name     string
+		messages []string
+		wantOut  string
+	}{
+		{
+			name:     "empty",
+			messages: []string{""},
+			wantOut:  "",
+		},
+		{
+			name:     "basically empty",
+			messages: []string{" "},
+			wantOut:  "",
+		},
+		{
+			name:     "something",
+			messages: []string{"something", "here"},
+			wantOut:  "remote: \nremote: something\nremote: here\nremote: \n",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			out := &bytes.Buffer{}
+			DisplayInfoMessages(tt.messages, out)
+
+			require.Equal(t, tt.wantOut, out.String())
+		})
+	}
+}
+
+func Test_noMessages(t *testing.T) {
+	tests := []struct {
+		name     string
+		messages []string
+		want     bool
+	}{
+		{
+			name:     "empty",
+			messages: []string{""},
+			want:     true,
+		},
+		{
+			name:     "basically empty",
+			messages: []string{" "},
+			want:     true,
+		},
+		{
+			name:     "something",
+			messages: []string{"something", "here"},
+			want:     false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.Equal(t, tt.want, noMessages(tt.messages))
+		})
+	}
+}
+
+func Test_formatLine(t *testing.T) {
+	require.Equal(t, "remote: something\n", formatLine("something"))
+}
+
+func Test_divider(t *testing.T) {
+	want := `remote: 
+remote: ========================================================================
+remote: 
+`
+
+	require.Equal(t, want, divider())
+}
diff --git a/spec/gitlab_shell_custom_git_receive_pack_spec.rb b/spec/gitlab_shell_custom_git_receive_pack_spec.rb
index 0a57903..4b0fec4 100644
--- a/spec/gitlab_shell_custom_git_receive_pack_spec.rb
+++ b/spec/gitlab_shell_custom_git_receive_pack_spec.rb
@@ -8,6 +8,7 @@ describe 'Custom bin/gitlab-shell git-receive-pack' do
   include_context 'gitlab shell'
 
   let(:env) { {'SSH_CONNECTION' => 'fake', 'SSH_ORIGINAL_COMMAND' => 'git-receive-pack group/repo' } }
+  let(:divider) { "remote: ========================================================================\n" }
 
   before(:context) do
     write_config("gitlab_url" => "http+unix://#{CGI.escape(tmp_socket_path)}")
@@ -65,21 +66,24 @@ describe 'Custom bin/gitlab-shell git-receive-pack' do
   end
 
   describe 'dialog for performing a custom action' do
-    let(:inaccessible_error) { "Internal API error (403)\n" }
-
     context 'when API calls perform successfully' do
+      let(:remote_blank_line) { "remote: \n" }
       def verify_successful_call!(cmd)
         Open3.popen3(env, cmd) do |stdin, stdout, stderr|
-          expect(stderr.gets).to eq("> GitLab: console\n")
-          expect(stderr.gets).to eq("> GitLab: message\n")
+          expect(stderr.gets).to eq(remote_blank_line)
+          expect(stderr.gets).to eq("remote: console\n")
+          expect(stderr.gets).to eq("remote: message\n")
+          expect(stderr.gets).to eq(remote_blank_line)
 
-          expect(stderr.gets).to eq("> GitLab: info_message\n")
-          expect(stderr.gets).to eq("> GitLab: another_message\n")
-          expect(stdout.gets(6)).to eq("custom")
+          expect(stderr.gets).to eq(remote_blank_line)
+          expect(stderr.gets).to eq("remote: info_message\n")
+          expect(stderr.gets).to eq("remote: another_message\n")
+          expect(stderr.gets).to eq(remote_blank_line)
 
           stdin.puts("input")
           stdin.close
 
+          expect(stdout.gets(6)).to eq("custom")
           expect(stdout.flush.read).to eq("input\n")
         end
       end
@@ -106,7 +110,13 @@ describe 'Custom bin/gitlab-shell git-receive-pack' do
 
       it 'custom action is not performed' do
         Open3.popen2e(env, cmd) do |stdin, stdout|
-          expect(stdout.gets).to eq(inaccessible_error)
+          expect(stdout.gets).to eq("remote: \n")
+          expect(stdout.gets).to eq(divider)
+          expect(stdout.gets).to eq("remote: \n")
+          expect(stdout.gets).to eq("remote: Internal API error (403)\n")
+          expect(stdout.gets).to eq("remote: \n")
+          expect(stdout.gets).to eq(divider)
+          expect(stdout.gets).to eq("remote: \n")
         end
       end
     end
diff --git a/spec/gitlab_shell_lfs_authentication_spec.rb b/spec/gitlab_shell_lfs_authentication_spec.rb
index d27f50a..437005c 100644
--- a/spec/gitlab_shell_lfs_authentication_spec.rb
+++ b/spec/gitlab_shell_lfs_authentication_spec.rb
@@ -117,9 +117,10 @@ describe 'bin/gitlab-shell git-lfs-authentication' do
       let(:env) { {'SSH_CONNECTION' => 'fake', 'SSH_ORIGINAL_COMMAND' => 'git-lfs-authenticate project/repo unknown' } }
 
       it 'the command is disallowed' do
+        divider = "remote: \nremote: ========================================================================\nremote: \n"
         _, stderr, status = Open3.capture3(env, cmd)
 
-        expect(stderr).to eq("> GitLab: Disallowed command\n")
+        expect(stderr).to eq("#{divider}remote: Disallowed command\n#{divider}")
         expect(status).not_to be_success
       end
     end