Remove leaking log from deploy

This commit removing lekaing logs from deploy API in SDK. It also
returns typed errors from API resoponse and allows as to check for some
custom errors which functions like `IsNotFound` or `IsUnauthorized`.

This also updates DeployFuntion API to return a typed response and `http.Response`.

Signed-off-by: Vivek Singh <[email protected]>

Author: viveksyngh

commands/deploy.go

@@ -276,9 +276,15 @@ Error: %s`, fprocessErr.Error())
 			if msg := checkTLSInsecure(services.Provider.GatewayURL, deploySpec.TLSInsecure); len(msg) > 0 {
 				fmt.Println(msg)
 			}
-			statusCode := proxyClient.DeployFunction(ctx, deploySpec)
-			if badStatusCode(statusCode) {
-				failedStatusCodes[k] = statusCode
+			dRes, res, err := proxyClient.DeployFunction(ctx, deploySpec)
+
+			if err == nil {
+				fmt.Println(dRes.Message)
+				fmt.Printf("URL: %s\n\n", dRes.URL)
+			}
+
+			if badStatusCode(res.StatusCode) {
+				failedStatusCodes[k] = res.StatusCode
 			}
 		}
 	} else {
@@ -375,9 +381,14 @@ func deployImage(
 		fmt.Println(msg)
 	}
 
-	statusCode = client.DeployFunction(ctx, deploySpec)
+	dRes, res, err := client.DeployFunction(ctx, deploySpec)
+	if err != nil {
+		return res.StatusCode, err
+	}
 
-	return statusCode, nil
+	fmt.Println(dRes.Message)
+	fmt.Printf("URL: %s\n\n", dRes.URL)
+	return res.StatusCode, nil
 }
 
 func mergeSlice(values []string, overlay []string) []string {

commands/describe.go

@@ -82,9 +82,9 @@ func runDescribe(cmd *cobra.Command, args []string) error {
 	}
 
 	//To get correct value for invocation count from /system/functions endpoint
-	functionList, err := cliClient.ListFunctions(ctx, functionNamespace)
+	functionList, _, err := cliClient.ListFunctions(ctx, functionNamespace)
 	if err != nil {
-		return err
+		return actionableErrorMessage(err)
 	}
 
 	var invocationCount int

commands/errors.go

@@ -4,7 +4,10 @@
 package commands
 
 import (
+	"fmt"
 	"strings"
+
+	"github.com/openfaas/faas-cli/proxy"
 )
 
 const (
@@ -24,3 +27,13 @@ func checkTLSInsecure(gateway string, tlsInsecure bool) string {
 	}
 	return ""
 }
+
+//actionableErrorMessage print actionable error message based on APIError check
+func actionableErrorMessage(err error) error {
+	if proxy.IsUnknown(err) {
+		return fmt.Errorf("server returned unexpected status response: %s", err.Error())
+	} else if proxy.IsUnauthorized(err) {
+		return fmt.Errorf("unauthorized access, run \"faas-cli login\" to setup authentication for this server")
+	}
+	return err
+}

commands/list.go

@@ -73,9 +73,9 @@ func runList(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	functions, err := proxyClient.ListFunctions(context.Background(), functionNamespace)
+	functions, _, err := proxyClient.ListFunctions(context.Background(), functionNamespace)
 	if err != nil {
-		return err
+		return actionableErrorMessage(err)
 	}
 
 	if sortOrder == "name" {

proxy/client.go

@@ -1,9 +1,12 @@
 package proxy
 
 import (
+	"bytes"
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net/http"
 	"net/http/httputil"
 	"net/url"
@@ -129,3 +132,30 @@ func addQueryParams(u string, params map[string]string) (string, error) {
 func (c *Client) AddCheckRedirect(checkRedirect func(*http.Request, []*http.Request) error) {
 	c.httpClient.CheckRedirect = checkRedirect
 }
+
+// parseResponse function parses HTTP response body into a typed struct
+// or copies to io.Writer object. If it fails to decode response body
+// then it re-construct it so that it can be read later
+func parseResponse(res *http.Response, v interface{}) error {
+	var err error
+	defer res.Body.Close()
+
+	switch v := v.(type) {
+	case nil:
+	case io.Writer:
+		_, err = io.Copy(v, res.Body)
+	default:
+		data, err := ioutil.ReadAll(res.Body)
+		if err == io.EOF {
+			err = nil // ignore EOF errors caused by empty response body
+		}
+
+		decErr := json.Unmarshal(data, v)
+		if decErr != nil {
+			err = decErr
+			// In case of JSON decode error, re-construct response body
+			res.Body = ioutil.NopCloser(bytes.NewBuffer(data))
+		}
+	}
+	return err
+}

proxy/deploy.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"net/http"
 	"time"
 
@@ -57,29 +56,48 @@ func generateFuncStr(spec *DeployFunctionSpec) string {
 	return spec.FunctionName
 }
 
+type DeployResponse struct {
+	Message       string
+	RollingUpdate bool
+	URL           string
+}
+
 // DeployFunction first tries to deploy a function and if it exists will then attempt
 // a rolling update. Warnings are suppressed for the second API call (if required.)
-func (c *Client) DeployFunction(context context.Context, spec *DeployFunctionSpec) int {
+func (c *Client) DeployFunction(context context.Context, spec *DeployFunctionSpec) (*DeployResponse, *http.Response, error) {
 
 	rollingUpdateInfo := fmt.Sprintf("Function %s already exists, attempting rolling-update.", spec.FunctionName)
-	statusCode, deployOutput := c.deploy(context, spec, spec.Update)
+	res, err := c.deploy(context, spec, spec.Update)
+
+	if err != nil && IsUnknown(err) {
+		return nil, res, err
+	}
+
+	var deployResponse DeployResponse
+	if err == nil {
+		deployResponse.Message = fmt.Sprintf("Deployed. %s.", res.Status)
+		deployResponse.URL = fmt.Sprintf("%s/function/%s", c.GatewayURL.String(), generateFuncStr(spec))
+	}
 
-	if spec.Update == true && statusCode == http.StatusNotFound {
+	if spec.Update == true && IsNotFound(err) {
 		// Re-run the function with update=false
+		res, err = c.deploy(context, spec, false)
+		if err == nil {
+			deployResponse.Message = fmt.Sprintf("Deployed. %s.", res.Status)
+			deployResponse.URL = fmt.Sprintf("%s/function/%s", c.GatewayURL.String(), generateFuncStr(spec))
+		}
+
+	} else if res.StatusCode == http.StatusOK {
+		deployResponse.Message += rollingUpdateInfo
+		deployResponse.RollingUpdate = true
 
-		statusCode, deployOutput = c.deploy(context, spec, false)
-	} else if statusCode == http.StatusOK {
-		fmt.Println(rollingUpdateInfo)
 	}
-	fmt.Println()
-	fmt.Println(deployOutput)
-	return statusCode
+
+	return &deployResponse, res, err
 }
 
 // deploy a function to an OpenFaaS gateway over REST
-func (c *Client) deploy(context context.Context, spec *DeployFunctionSpec, update bool) (int, string) {
-
-	var deployOutput string
+func (c *Client) deploy(context context.Context, spec *DeployFunctionSpec, update bool) (*http.Response, error) {
 	// Need to alter Gateway to allow nil/empty string as fprocess, to avoid this repetition.
 	var fprocessTemplate string
 	if len(spec.FProcess) > 0 {
@@ -146,37 +164,20 @@ func (c *Client) deploy(context context.Context, spec *DeployFunctionSpec, updat
 	request, err = c.newRequest(method, "/system/functions", reader)
 
 	if err != nil {
-		deployOutput += fmt.Sprintln(err)
-		return http.StatusInternalServerError, deployOutput
+		return nil, err
 	}
 
 	res, err := c.doRequest(context, request)
 
 	if err != nil {
-		deployOutput += fmt.Sprintln("Is OpenFaaS deployed? Do you need to specify the --gateway flag?")
-		deployOutput += fmt.Sprintln(err)
-		return http.StatusInternalServerError, deployOutput
-	}
-
-	if res.Body != nil {
-		defer res.Body.Close()
+		errMessage := fmt.Sprintln("Is OpenFaaS deployed? Do you need to specify the --gateway flag?")
+		errMessage += fmt.Sprintln(err)
+		return res, NewUnknown(errMessage, 0)
 	}
 
-	switch res.StatusCode {
-	case http.StatusOK, http.StatusCreated, http.StatusAccepted:
-		deployOutput += fmt.Sprintf("Deployed. %s.\n", res.Status)
-
-		deployedURL := fmt.Sprintf("URL: %s/function/%s", c.GatewayURL.String(), generateFuncStr(spec))
-		deployOutput += fmt.Sprintln(deployedURL)
-	case http.StatusUnauthorized:
-		deployOutput += fmt.Sprintln("unauthorized access, run \"faas-cli login\" to setup authentication for this server")
-
-	default:
-		bytesOut, err := ioutil.ReadAll(res.Body)
-		if err == nil {
-			deployOutput += fmt.Sprintf("Unexpected status: %d, message: %s\n", res.StatusCode, string(bytesOut))
-		}
+	err = checkForAPIError(res)
+	if err != nil {
+		return res, err
 	}
-
-	return res.StatusCode, deployOutput
+	return res, nil
 }

proxy/deploy_test.go

@@ -21,7 +21,8 @@ type deployProxyTest struct {
 	mockServerResponses []int
 	replace             bool
 	update              bool
-	expectedOutput      string
+	expectedMessage     string
+	statusCode          int
 }
 
 func runDeployProxyTest(t *testing.T, deployTest deployProxyTest) {
@@ -34,32 +35,34 @@ func runDeployProxyTest(t *testing.T, deployTest deployProxyTest) {
 	cliAuth := NewTestAuth(nil)
 	proxyClient, _ := NewClient(cliAuth, s.URL, nil, &defaultCommandTimeout)
 
-	stdout := test.CaptureStdout(func() {
-		proxyClient.DeployFunction(context.TODO(), &DeployFunctionSpec{
-			"fprocess",
-			"function",
-			"image",
-			"dXNlcjpwYXNzd29yZA==",
-			"language",
-			deployTest.replace,
-			nil,
-			"network",
-			[]string{},
-			deployTest.update,
-			[]string{},
-			map[string]string{},
-			map[string]string{},
-			FunctionResourceRequest{},
-			false,
-			tlsNoVerify,
-			"",
-			"",
-		})
+	dRes, httpRes, _ := proxyClient.DeployFunction(context.TODO(), &DeployFunctionSpec{
+		"fprocess",
+		"function",
+		"image",
+		"dXNlcjpwYXNzd29yZA==",
+		"language",
+		deployTest.replace,
+		nil,
+		"network",
+		[]string{},
+		deployTest.update,
+		[]string{},
+		map[string]string{},
+		map[string]string{},
+		FunctionResourceRequest{},
+		false,
+		tlsNoVerify,
+		"",
+		"",
 	})
 
-	r := regexp.MustCompile(deployTest.expectedOutput)
-	if !r.MatchString(stdout) {
-		t.Fatalf("Output not matched: %s", stdout)
+	if httpRes.StatusCode != deployTest.statusCode {
+		t.Fatalf("StatuCode did not match. expected: %d, got: %d", deployTest.statusCode, httpRes.StatusCode)
+	}
+
+	r := regexp.MustCompile(deployTest.expectedMessage)
+	if !r.MatchString(dRes.Message) {
+		t.Fatalf("Output not matched: %s", dRes.Message)
 	}
 }
 
@@ -70,21 +73,24 @@ func Test_RunDeployProxyTests(t *testing.T) {
 			mockServerResponses: []int{http.StatusOK, http.StatusOK},
 			replace:             true,
 			update:              false,
-			expectedOutput:      `(?m:Deployed)`,
+			statusCode:          http.StatusOK,
+			expectedMessage:     `(?m:Deployed)`,
 		},
 		{
 			title:               "404_Deploy",
 			mockServerResponses: []int{http.StatusOK, http.StatusNotFound},
 			replace:             true,
 			update:              false,
-			expectedOutput:      `(?m:Unexpected status: 404)`,
+			statusCode:          http.StatusNotFound,
+			expectedMessage:     "",
 		},
 		{
 			title:               "UpdateFailedDeployed",
 			mockServerResponses: []int{http.StatusNotFound, http.StatusOK},
 			replace:             false,
 			update:              true,
-			expectedOutput:      `(?m:Deployed)`,
+			statusCode:          http.StatusOK,
+			expectedMessage:     `(?m:Deployed)`,
 		},
 	}
 	for _, tst := range deployProxyTests {