Wrap Stderr & Stdout in a reporter struct

The reporter struct can be used for passing around and reporting to
the io.Writer of choice.

8 files changed, 96 insertions, 37 deletions

diff --git a/go/cmd/gitlab-shell/main.go b/go/cmd/gitlab-shell/main.go
index 07623b4..2ed319d 100644
--- a/go/cmd/gitlab-shell/main.go
+++ b/go/cmd/gitlab-shell/main.go
@@ -7,25 +7,28 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback"
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/reporting"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/config"
 )
 
 var (
-	binDir  string
-	rootDir string
+	binDir   string
+	rootDir  string
+	reporter *reporting.Reporter
 )
 
 func init() {
 	binDir = filepath.Dir(os.Args[0])
 	rootDir = filepath.Dir(binDir)
+	reporter = &reporting.Reporter{Out: os.Stdout, ErrOut: os.Stderr}
 }
 
 // rubyExec will never return. It either replaces the current process with a
 // Ruby interpreter, or outputs an error and kills the process.
 func execRuby() {
 	cmd := &fallback.Command{}
-	if err := cmd.Execute(); err != nil {
-		fmt.Fprintf(os.Stderr, "Failed to exec: %v\n", err)
+	if err := cmd.Execute(reporter); err != nil {
+		fmt.Fprintf(reporter.ErrOut, "Failed to exec: %v\n", err)
 		os.Exit(1)
 	}
 }
@@ -35,7 +38,7 @@ func main() {
 	// warning as this isn't something we can sustain indefinitely
 	config, err := config.NewFromDir(rootDir)
 	if err != nil {
-		fmt.Fprintln(os.Stderr, "Failed to read config, falling back to gitlab-shell-ruby")
+		fmt.Fprintln(reporter.ErrOut, "Failed to read config, falling back to gitlab-shell-ruby")
 		execRuby()
 	}
 
@@ -43,14 +46,14 @@ func main() {
 	if err != nil {
 		// For now this could happen if `SSH_CONNECTION` is not set on
 		// the environment
-		fmt.Fprintf(os.Stderr, "%v\n", err)
+		fmt.Fprintf(reporter.ErrOut, "%v\n", err)
 		os.Exit(1)
 	}
 
 	// The command will write to STDOUT on execution or replace the current
 	// process in case of the `fallback.Command`
-	if err = cmd.Execute(); err != nil {
-		fmt.Fprintf(os.Stderr, "%v\n", err)
+	if err = cmd.Execute(reporter); err != nil {
+		fmt.Fprintf(reporter.ErrOut, "%v\n", err)
 		os.Exit(1)
 	}
 }
diff --git a/go/internal/command/command.go b/go/internal/command/command.go
index cb2acdc..d4649de 100644
--- a/go/internal/command/command.go
+++ b/go/internal/command/command.go
@@ -4,11 +4,12 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/discover"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback"
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/reporting"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/config"
 )
 
 type Command interface {
-	Execute() error
+	Execute(*reporting.Reporter) error
 }
 
 func New(arguments []string, config *config.Config) (Command, error) {
diff --git a/go/internal/command/discover/discover.go b/go/internal/command/discover/discover.go
index ab04cbd..8ad2868 100644
--- a/go/internal/command/discover/discover.go
+++ b/go/internal/command/discover/discover.go
@@ -2,10 +2,9 @@ package discover
 
 import (
 	"fmt"
-	"io"
-	"os"
 
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs"
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/reporting"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/config"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet/discover"
 )
@@ -15,20 +14,16 @@ type Command struct {
 	Args   *commandargs.CommandArgs
 }
 
-var (
-	output io.Writer = os.Stdout
-)
-
-func (c *Command) Execute() error {
+func (c *Command) Execute(reporter *reporting.Reporter) error {
 	response, err := c.getUserInfo()
 	if err != nil {
 		return fmt.Errorf("Failed to get username: %v", err)
 	}
 
 	if response.IsAnonymous() {
-		fmt.Fprintf(output, "Welcome to GitLab, Anonymous!\n")
+		fmt.Fprintf(reporter.Out, "Welcome to GitLab, Anonymous!\n")
 	} else {
-		fmt.Fprintf(output, "Welcome to GitLab, @%s!\n", response.Username)
+		fmt.Fprintf(reporter.Out, "Welcome to GitLab, @%s!\n", response.Username)
 	}
 
 	return nil
diff --git a/go/internal/command/discover/discover_test.go b/go/internal/command/discover/discover_test.go
index 752e76e..ec6f931 100644
--- a/go/internal/command/discover/discover_test.go
+++ b/go/internal/command/discover/discover_test.go
@@ -11,6 +11,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs"
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/reporting"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/config"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet/testserver"
 )
@@ -78,11 +79,10 @@ func TestExecute(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			buffer := &bytes.Buffer{}
-			output = buffer
 			cmd := &Command{Config: testConfig, Args: tc.arguments}
+			buffer := &bytes.Buffer{}
 
-			err := cmd.Execute()
+			err := cmd.Execute(&reporting.Reporter{Out: buffer})
 
 			assert.NoError(t, err)
 			assert.Equal(t, tc.expectedOutput, buffer.String())
@@ -120,11 +120,12 @@ func TestFailingExecute(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			cmd := &Command{Config: testConfig, Args: tc.arguments}
+			buffer := &bytes.Buffer{}
 
-			err := cmd.Execute()
+			err := cmd.Execute(&reporting.Reporter{Out: buffer})
 
+			assert.Empty(t, buffer.String())
 			assert.EqualError(t, err, tc.expectedError)
 		})
 	}
-
 }
diff --git a/go/internal/command/fallback/fallback.go b/go/internal/command/fallback/fallback.go
index a136657..a2c73ed 100644
--- a/go/internal/command/fallback/fallback.go
+++ b/go/internal/command/fallback/fallback.go
@@ -4,6 +4,8 @@ import (
 	"os"
 	"path/filepath"
 	"syscall"
+
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/command/reporting"
 )
 
 type Command struct{}
@@ -12,7 +14,7 @@ var (
 	binDir = filepath.Dir(os.Args[0])
 )
 
-func (c *Command) Execute() error {
+func (c *Command) Execute(_ *reporting.Reporter) error {
 	rubyCmd := filepath.Join(binDir, "gitlab-shell-ruby")
 	execErr := syscall.Exec(rubyCmd, os.Args, os.Environ())
 	return execErr
diff --git a/go/internal/command/reporting/reporter.go b/go/internal/command/reporting/reporter.go
new file mode 100644
index 0000000..74bca59
--- /dev/null
+++ b/go/internal/command/reporting/reporter.go
@@ -0,0 +1,8 @@
+package reporting
+
+import "io"
+
+type Reporter struct {
+	Out    io.Writer
+	ErrOut io.Writer
+}
diff --git a/go/internal/gitlabnet/discover/client.go b/go/internal/gitlabnet/discover/client.go
index 4e65d25..8df78fb 100644
--- a/go/internal/gitlabnet/discover/client.go
+++ b/go/internal/gitlabnet/discover/client.go
@@ -45,7 +45,6 @@ func (c *Client) GetByUsername(username string) (*Response, error) {
 }
 
 func (c *Client) parseResponse(resp *http.Response) (*Response, error) {
-	defer resp.Body.Close()
 	parsedResponse := &Response{}
 
 	if err := json.NewDecoder(resp.Body).Decode(parsedResponse); err != nil {
@@ -53,7 +52,6 @@ func (c *Client) parseResponse(resp *http.Response) (*Response, error) {
 	} else {
 		return parsedResponse, nil
 	}
-
 }
 
 func (c *Client) getResponse(params url.Values) (*Response, error) {
@@ -64,7 +62,13 @@ func (c *Client) getResponse(params url.Values) (*Response, error) {
 		return nil, err
 	}
 
-	return c.parseResponse(response)
+	defer response.Body.Close()
+	parsedResponse, err := c.parseResponse(response)
+	if err != nil {
+		return nil, fmt.Errorf("Parsing failed")
+	}
+
+	return parsedResponse, nil
 }
 
 func (r *Response) IsAnonymous() bool {
diff --git a/go/internal/gitlabnet/discover/client_test.go b/go/internal/gitlabnet/discover/client_test.go
index 6c87d07..e88cedd 100644
--- a/go/internal/gitlabnet/discover/client_test.go
+++ b/go/internal/gitlabnet/discover/client_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/config"
+	"gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet"
 	"gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet/testserver"
 
 	"github.com/stretchr/testify/assert"
@@ -25,24 +26,32 @@ func init() {
 			Path: "/api/v4/internal/discover",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
 				if r.URL.Query().Get("key_id") == "1" {
-					body := map[string]interface{}{
-						"id":       2,
-						"username": "alex-doe",
-						"name":     "Alex Doe",
+					body := &Response{
+						UserId:   2,
+						Username: "alex-doe",
+						Name:     "Alex Doe",
 					}
 					json.NewEncoder(w).Encode(body)
 				} else if r.URL.Query().Get("username") == "jane-doe" {
-					body := map[string]interface{}{
-						"id":       1,
-						"username": "jane-doe",
-						"name":     "Jane Doe",
+					body := &Response{
+						UserId:   1,
+						Username: "jane-doe",
+						Name:     "Jane Doe",
 					}
 					json.NewEncoder(w).Encode(body)
+				} else if r.URL.Query().Get("username") == "broken_message" {
+					w.WriteHeader(http.StatusForbidden)
+					body := &gitlabnet.ErrorResponse{
+						Message: "Not allowed!",
+					}
+					json.NewEncoder(w).Encode(body)
+				} else if r.URL.Query().Get("username") == "broken_json" {
+					w.Write([]byte("{ \"message\": \"broken json!\""))
+				} else if r.URL.Query().Get("username") == "broken_empty" {
+					w.WriteHeader(http.StatusForbidden)
 				} else {
 					fmt.Fprint(w, "null")
-
 				}
-
 			},
 		},
 	}
@@ -75,6 +84,42 @@ func TestMissingUser(t *testing.T) {
 	assert.True(t, result.IsAnonymous())
 }
 
+func TestErrorResponses(t *testing.T) {
+	client, cleanup := setup(t)
+	defer cleanup()
+
+	testCases := []struct {
+		desc          string
+		fakeUsername  string
+		expectedError string
+	}{
+		{
+			desc:          "A response with an error message",
+			fakeUsername:  "broken_message",
+			expectedError: "Not allowed!",
+		},
+		{
+			desc:          "A response with bad JSON",
+			fakeUsername:  "broken_json",
+			expectedError: "Parsing failed",
+		},
+		{
+			desc:          "An error response without message",
+			fakeUsername:  "broken_empty",
+			expectedError: "Internal API error (403)",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			resp, err := client.GetByUsername(tc.fakeUsername)
+
+			assert.EqualError(t, err, tc.expectedError)
+			assert.Nil(t, resp)
+		})
+	}
+}
+
 func setup(t *testing.T) (*Client, func()) {
 	cleanup, err := testserver.StartSocketHttpServer(requests)
 	require.NoError(t, err)