Merge pull request #20 from sambaiz/feature/check_commmand_exitcode

improve logging

Makefile

@@ -54,4 +54,4 @@ mock:
 	mockgen -package mock -source functions/operation/cdk/cdk.go -destination functions/operation/cdk/mock/cdk_mock.go
 	mockgen -package mock -source functions/operation/config/config.go -destination functions/operation/config/mock/config_mock.go
 	mockgen -package mock -source functions/operation/git/git.go -destination functions/operation/git/mock/git_mock.go
-	mockgen -package mock -source functions/operation/platform/client.go -destination functions/operation/platform/mock/client_mock.go
+	mockgen -package mock -source functions/operation/platform/client.go -destination functions/operation/platform/mock/client_mock.go

functions/operation/cdk/cdk.go

@@ -1,7 +1,6 @@
 package cdk
 
 import (
-	"errors"
 	"fmt"
 	"os"
 	"os/exec"
@@ -31,9 +30,9 @@ func (*Client) Setup(repoPath string) error {
 	}
 	cmd := exec.Command("npm", "install")
 	cmd.Dir = repoPath
-	out, _ := cmd.CombinedOutput()
-	if cmd.ProcessState.ExitCode() != 0 {
-		return fmt.Errorf("npm install failed: %s", string(out))
+	out, err := cmd.CombinedOutput()
+	if err != nil || cmd.ProcessState.ExitCode() != 0 {
+		return fmt.Errorf("npm install failed: %s %v", string(out), err)
 	}
 	return nil
 }
@@ -47,37 +46,33 @@ func (*Client) List(repoPath string, contexts map[string]string) ([]string, erro
 	cmd := exec.Command("npm", args...)
 	cmd.Dir = repoPath
 	out, err := cmd.CombinedOutput()
-	if err != nil {
-		return nil, err
-	}
-	if cmd.ProcessState.ExitCode() != 0 {
-		return nil, fmt.Errorf("cdk list failed: %s", string(out))
+	if err != nil || cmd.ProcessState.ExitCode() != 0 {
+		return nil, fmt.Errorf("cdk list failed: %s %v", string(out), err)
 	}
 	lists := strings.Split(strings.Trim(string(out), "\n"), "\n")[3:]
 	return lists, nil
 }
 
-// Diff stack and returns (diff, hasDiff)
+// Diff stack and returns (diff, hasDiff, error)
 func (*Client) Diff(repoPath string, stacks string, contexts map[string]string) (string, bool, error) {
 	args := []string{"run", "cdk", "--", "diff", stacks}
 	for k, v := range contexts {
 		args = append(args, "-c", fmt.Sprintf("%s=%s", k, v))
 	}
 	cmd := exec.Command("npm", args...)
 	cmd.Dir = repoPath
-	out, _ := cmd.CombinedOutput()
+	out, err := cmd.CombinedOutput()
+	// If the error code is 0, there is no diff, if it is 1, there is diff, otherwise it is an error
+	if cmd.ProcessState.ExitCode() != 0 && cmd.ProcessState.ExitCode() != 1 {
+		return "failed!", true, fmt.Errorf("cdk diff failed: %s %v", string(out), err)
+	}
 	lines := []string{}
 	for _, line := range strings.Split(strings.Trim(string(out), "\n"), "\n")[3:] {
 		if !strings.HasPrefix(line, "npm ERR!") {
 			lines = append(lines, line)
 		}
 	}
-	var err error
-	// If the error code is 0, there is no diff, if it is 1, there is diff, otherwise it is an error
-	if cmd.ProcessState.ExitCode() != 0 && cmd.ProcessState.ExitCode() != 1 {
-		err = errors.New("cdk diff failed")
-	}
-	return strings.Trim(strings.Join(lines, "\n"), "\n"), cmd.ProcessState.ExitCode() != 0, err
+	return strings.Trim(strings.Join(lines, "\n"), "\n"), cmd.ProcessState.ExitCode() != 0, nil
 }
 
 // Deploy stack
@@ -88,16 +83,15 @@ func (*Client) Deploy(repoPath string, stacks string, contexts map[string]string
 	}
 	cmd := exec.Command("npm", args...)
 	cmd.Dir = repoPath
-	out, _ := cmd.CombinedOutput()
+	out, err := cmd.CombinedOutput()
+	if err != nil || cmd.ProcessState.ExitCode() != 0 {
+		return "failed!", fmt.Errorf("cdk deploy failed: %s %v", string(out), err)
+	}
 	lines := []string{}
 	for _, line := range strings.Split(strings.Trim(string(out), "\n"), "\n")[3:] {
 		if !strings.HasPrefix(line, "npm ERR!") {
 			lines = append(lines, line)
 		}
 	}
-	var err error
-	if cmd.ProcessState.ExitCode() != 0 {
-		err = errors.New("cdk deploy failed")
-	}
 	return strings.Trim(strings.Join(lines, "\n"), "\n"), err
 }

functions/operation/command/command.go

@@ -7,7 +7,9 @@ import (
 	"github.com/sambaiz/cdkbot/functions/operation/config"
 	"github.com/sambaiz/cdkbot/functions/operation/constant"
 	"github.com/sambaiz/cdkbot/functions/operation/git"
+	"github.com/sambaiz/cdkbot/functions/operation/logger"
 	"github.com/sambaiz/cdkbot/functions/operation/platform"
+	"go.uber.org/zap"
 	"os/exec"
 	"regexp"
 	"strings"
@@ -35,15 +37,17 @@ type Runner struct {
 	git      git.Clienter
 	config   config.Readerer
 	cdk      cdk.Clienter
+	logger   logger.Loggerer
 }
 
-// NewRunner Runenrn
-func NewRunner(client platform.Clienter, cloneURL string) *Runner {
+// NewRunner creates Runner
+func NewRunner(client platform.Clienter, cloneURL string, logger logger.Loggerer) *Runner {
 	return &Runner{
 		platform: client,
 		git:      git.NewClient(cloneURL),
 		config:   new(config.Reader),
 		cdk:      new(cdk.Client),
+		logger:   logger,
 	}
 }
 
@@ -70,20 +74,26 @@ func (r *Runner) updateStatus(
 		return err
 	}
 	state, err := f()
-	r.platform.RemoveLabel(ctx, constant.LabelRunning)
+	if err := r.platform.RemoveLabel(ctx, constant.LabelRunning); err != nil {
+		r.logger.Error("remove label error", zap.Error(err))
+	}
 	if err != nil {
-		r.platform.SetStatus(
+		if err := r.platform.SetStatus(
 			ctx,
 			constant.StateError,
 			err.Error(),
-		)
+		); err != nil {
+			r.logger.Error("set status error", zap.Error(err))
+		}
 		return err
 	}
-	r.platform.SetStatus(
+	if err := r.platform.SetStatus(
 		ctx,
 		state.state,
 		state.description,
-	)
+	); err != nil {
+		return err
+	}
 	return nil
 }
 
@@ -132,8 +142,8 @@ func (r *Runner) setup(ctx context.Context, cloneHead bool) (string, *config.Con
 		command := strings.Split(preCommand, " ")
 		cmd := exec.Command(command[0], command[1:]...)
 		cmd.Dir = cdkPath
-		if out, _ := cmd.CombinedOutput(); cmd.ProcessState.ExitCode() != 0 {
-			return "", nil, nil, nil, fmt.Errorf("preCommand %s failed: %s", preCommand, string(out))
+		if out, err := cmd.CombinedOutput(); err != nil || cmd.ProcessState.ExitCode() != 0 {
+			return "", nil, nil, nil, fmt.Errorf("preCommand %s failed: %s %v", preCommand, string(out), err)
 		}
 	}
 	return cdkPath, cfg, &target, pr, nil

functions/operation/command/command_test.go

@@ -3,6 +3,7 @@ package command
 import (
 	"context"
 	"fmt"
+	"github.com/sambaiz/cdkbot/functions/operation/logger"
 	"github.com/sambaiz/cdkbot/functions/operation/platform"
 	"strings"
 	"testing"
@@ -77,6 +78,7 @@ func TestRunner_setup(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 			cdkPath, retCfg, retTarget, outpr, err := runner.setup(ctx, cloneHead)
 			assert.Equal(t, fmt.Sprintf("%s/%s", clonePath, cfg.CDKRoot), cdkPath)

functions/operation/command/deploy_test.go

@@ -3,6 +3,7 @@ package command
 import (
 	"context"
 	"fmt"
+	"github.com/sambaiz/cdkbot/functions/operation/logger"
 	"github.com/sambaiz/cdkbot/functions/operation/platform"
 	"strings"
 	"testing"
@@ -207,6 +208,7 @@ func TestRunner_Deploy(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 
@@ -236,6 +238,7 @@ func TestRunner_Deploy(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 
@@ -259,6 +262,7 @@ func TestRunner_Deploy(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 
@@ -273,6 +277,7 @@ func TestRunner_Deploy(t *testing.T) {
 			git:      gitClient,
 			config:   configClient,
 			cdk:      cdkClient,
+			logger:   logger.MockLogger{},
 		}
 	}
 

functions/operation/command/diff_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"github.com/sambaiz/cdkbot/functions/operation/constant"
+	"github.com/sambaiz/cdkbot/functions/operation/logger"
 	"github.com/sambaiz/cdkbot/functions/operation/platform"
 	"testing"
 
@@ -147,6 +148,7 @@ func TestRunner_Diff(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 
@@ -161,6 +163,7 @@ func TestRunner_Diff(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 		platformClient.EXPECT().RemoveLabel(ctx, constant.LabelOutdatedDiff).Return(nil)
@@ -170,6 +173,7 @@ func TestRunner_Diff(t *testing.T) {
 			git:      gitClient,
 			config:   configClient,
 			cdk:      cdkClient,
+			logger:   logger.MockLogger{},
 		}
 	}
 

functions/operation/command/rollback_test.go

@@ -3,6 +3,7 @@ package command
 import (
 	"context"
 	"fmt"
+	"github.com/sambaiz/cdkbot/functions/operation/logger"
 	"github.com/sambaiz/cdkbot/functions/operation/platform"
 	"strings"
 	"testing"
@@ -235,6 +236,7 @@ func TestRunner_Rollback(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 		if !cfg.IsUserAllowedDeploy(userName) {
@@ -243,6 +245,7 @@ func TestRunner_Rollback(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 
@@ -263,6 +266,7 @@ func TestRunner_Rollback(t *testing.T) {
 				git:      gitClient,
 				config:   configClient,
 				cdk:      cdkClient,
+				logger:   logger.MockLogger{},
 			}
 		}
 		if !resultHasDiff {
@@ -274,6 +278,7 @@ func TestRunner_Rollback(t *testing.T) {
 			git:      gitClient,
 			config:   configClient,
 			cdk:      cdkClient,
+			logger:   logger.MockLogger{},
 		}
 	}
 

functions/operation/git/git.go

@@ -33,8 +33,8 @@ func (c *Client) Clone(path string, hash *string) error {
 		return err
 	}
 	cmd := exec.Command("git", "clone", c.cloneURL, path)
-	if out, _ := cmd.CombinedOutput(); cmd.ProcessState.ExitCode() != 0 {
-		return fmt.Errorf("git clone failed: %s", string(out))
+	if out, err := cmd.CombinedOutput(); err != nil || cmd.ProcessState.ExitCode() != 0 {
+		return fmt.Errorf("git clone failed: %s %v", string(out), err)
 	}
 	if hash != nil {
 		cmd := exec.Command("git", "checkout", *hash)
@@ -50,8 +50,8 @@ func (c *Client) Clone(path string, hash *string) error {
 func (c *Client) Merge(path, branch string) error {
 	cmd := exec.Command("git", "merge", branch, "-m", `"cdkbot merged"`)
 	cmd.Dir = path
-	if out, _ := cmd.CombinedOutput(); cmd.ProcessState.ExitCode() != 0 {
-		return fmt.Errorf("git merge failed: %s", string(out))
+	if out, err := cmd.CombinedOutput(); err != nil || cmd.ProcessState.ExitCode() != 0 {
+		return fmt.Errorf("git merge failed: %s %v", string(out), err)
 	}
 	return nil
 }
@@ -60,8 +60,8 @@ func (c *Client) Merge(path, branch string) error {
 func (c *Client) Checkout(path, fileName, branch string) error {
 	cmd := exec.Command("git", "checkout", branch, fileName)
 	cmd.Dir = path
-	if out, _ := cmd.CombinedOutput(); cmd.ProcessState.ExitCode() != 0 {
-		return fmt.Errorf("git checkout %s of %s failed: %s", fileName, branch, string(out))
+	if out, err := cmd.CombinedOutput(); err != nil || cmd.ProcessState.ExitCode() != 0 {
+		return fmt.Errorf("git checkout %s of %s failed: %s %v", fileName, branch, string(out), err)
 	}
 	return nil
 }

functions/operation/logger/log.go

@@ -0,0 +1,42 @@
+package logger
+
+import "go.uber.org/zap"
+
+// Loggerer is interface of Logger
+type Loggerer interface {
+	Info(msg string, fields ...zap.Field)
+	Error(msg string, fields ...zap.Field)
+}
+
+// Logger logs
+type Logger struct {
+	logger *zap.Logger
+}
+
+// New Logger
+func New() *Logger {
+	logger, err := zap.NewProduction()
+	if err != nil {
+		panic(err)
+	}
+	return &Logger{
+		logger: logger,
+	}
+}
+
+// Info log
+func (l *Logger) Info(msg string, fields ...zap.Field) {
+	l.logger.Info(msg, fields...)
+}
+
+// Error log
+func (l *Logger) Error(msg string, fields ...zap.Field) {
+	l.logger.Error(msg, fields...)
+}
+
+// MockLogger is logger for test
+type MockLogger struct {}
+// Info log
+func (l MockLogger) Info(msg string, fields ...zap.Field) {}
+// Error log
+func (l MockLogger) Error(msg string, fields ...zap.Field) {}