Logger improvements

colorlogger/colorLogger.go

@@ -6,38 +6,44 @@ import (
 	"log"
 )
 
-type colorLogger struct {
-	color    bool
-	logger   *log.Logger
-	logLevel int
-}
-
 const (
 	NONE  = -1
 	DEBUG = 0
+
+	debug             = "debug"
+	plainPrefixFormat = "%s:"
+	colorPrefixFormat = "\033[32m%s:\033[0m"
 )
 
-var logLevelsNames = []string{"debug"}
+type colorLogger struct {
+	color    bool
+	logger   *log.Logger
+	logLevel int
+}
 
-func ConstructLogger(logLevel int, color bool, writer io.Writer) *colorLogger {
-	logger := log.New(writer, "", 0)
-	loggerPlus := colorLogger{color, logger, logLevel}
-	return &loggerPlus
+type Logger interface {
+	Printf(format string, args ...interface{})
 }
 
-func (g *colorLogger) Logf(logLevel int, msg string) {
-	if g.logLevel >= logLevel && g.logLevel != NONE {
-		if g.color {
-			g.logger.Printf("\033[32m%s:\033[0m %s", logLevelsNames[logLevel], msg)
-		} else {
-			g.logger.Printf("%s: %s", logLevelsNames[logLevel], msg)
+func New(logLevel int, color bool, writer io.Writer) Logger {
+	return &colorLogger{
+		color:    color,
+		logger:   log.New(writer, "", 0),
+		logLevel: logLevel,
+	}
+}
 
-		}
+func (cl *colorLogger) Printf(format string, a ...interface{}) {
+	if cl.logLevel >= DEBUG && cl.logLevel != NONE {
+		cl.logger.Printf("%s %s", cl.prefix(), fmt.Sprintf(format, a...))
 	}
 }
 
-// This function is here so that existing Debugf outputs will still work.
-// Once we figure out how to properly deal with logging, this can be revisited
-func (g *colorLogger) Debugf(format string, a ...interface{}) {
-	g.Logf(DEBUG, fmt.Sprintf(format, a...))
+func (cl *colorLogger) prefix() string {
+	prefixFormat := plainPrefixFormat
+	if cl.color {
+		prefixFormat = colorPrefixFormat
+	}
+
+	return fmt.Sprintf(prefixFormat, debug)
 }

colorlogger/colorlogger_test.go

@@ -10,50 +10,37 @@ import (
 )
 
 var _ = Describe("Stdout", func() {
-
 	It("write debug output when log level is debug", func() {
 		buf := bytes.Buffer{}
 
 		Expect(colorlogger.DEBUG).To(Equal(0))
-		logger := colorlogger.ConstructLogger(colorlogger.DEBUG, false, &buf)
+		logger := colorlogger.New(colorlogger.DEBUG, false, &buf)
 
 		message := "This is a test"
 
-		logger.Logf(colorlogger.DEBUG, message)
+		logger.Printf(message)
 		Expect(buf.String()).To(Equal("debug: " + message + "\n"))
 	})
 
 	It("write no debug output when log level is NONE", func() {
 		buf := bytes.Buffer{}
 
-		logger := colorlogger.ConstructLogger(colorlogger.NONE, false, &buf)
-
-		message := "This is a test"
-
-		logger.Logf(colorlogger.DEBUG, message)
-		Expect(buf.String()).To(BeEmpty())
-	})
-
-	It("write no none output when log level is NONE", func() {
-		buf := bytes.Buffer{}
-
-		logger := colorlogger.ConstructLogger(colorlogger.NONE, false, &buf)
+		logger := colorlogger.New(colorlogger.NONE, false, &buf)
 
 		message := "This is a test"
 
-		logger.Logf(colorlogger.NONE, message)
+		logger.Printf(message)
 		Expect(buf.String()).To(BeEmpty())
 	})
 
 	It("write colored debug output when log level is DEBUG and color is true", func() {
 		buf := bytes.Buffer{}
 
-		logger := colorlogger.ConstructLogger(colorlogger.DEBUG, true, &buf)
+		logger := colorlogger.New(colorlogger.DEBUG, true, &buf)
 
 		message := "This is a test"
 
-		logger.Logf(colorlogger.DEBUG, message)
+		logger.Printf(message)
 		Expect(buf.String()).To(Equal("\033[32mdebug:\033[0m " + message + "\n"))
 	})
-
 })

commandparser/package_stemcell.go

@@ -23,7 +23,7 @@ type OSAndVersionGetter interface {
 
 //counterfeiter:generate . PackagerFactory
 type PackagerFactory interface {
-	Packager(sourceConfig config.SourceConfig, outputConfig config.OutputConfig, logLevel int, color bool) (Packager, error)
+	Packager(sourceConfig config.SourceConfig, outputConfig config.OutputConfig, logger colorlogger.Logger) (Packager, error)
 }
 
 //counterfeiter:generate . Packager
@@ -129,7 +129,8 @@ func (p *PackageCmd) Execute(_ context.Context, f *flag.FlagSet, _ ...interface{
 		return subcommands.ExitFailure
 	}
 
-	packager, err := p.packagerFactory.Packager(p.sourceConfig, p.outputConfig, logLevel, p.GlobalFlags.Color)
+	logger := colorlogger.New(logLevel, p.GlobalFlags.Color, os.Stderr)
+	packager, err := p.packagerFactory.Packager(p.sourceConfig, p.outputConfig, logger)
 	if err != nil {
 		p.packagerMessenger.CannotCreatePackager(err)
 		return subcommands.ExitFailure

commandparser/package_stemcell_test.go

@@ -62,7 +62,7 @@ var _ = Describe("package_stemcell", func() {
 				Expect(exitStatus).To(Equal(subcommands.ExitSuccess))
 
 				Expect(packagerFactory.PackagerCallCount()).To(Equal(1))
-				actualSourceConfig, _, _, _ := packagerFactory.PackagerArgsForCall(0)
+				actualSourceConfig, _, _ := packagerFactory.PackagerArgsForCall(0)
 				Expect(actualSourceConfig.Vmdk).To(Equal("some_vmdk_file"))
 			})
 
@@ -82,7 +82,7 @@ var _ = Describe("package_stemcell", func() {
 				Expect(exitStatus).To(Equal(subcommands.ExitSuccess))
 
 				Expect(packagerFactory.PackagerCallCount()).To(Equal(1))
-				actualSourceConfig, _, _, _ := packagerFactory.PackagerArgsForCall(0)
+				actualSourceConfig, _, _ := packagerFactory.PackagerArgsForCall(0)
 				Expect(actualSourceConfig.URL).To(Equal("https://vcenter.test"))
 				Expect(actualSourceConfig.Username).To(Equal("test-user"))
 				Expect(actualSourceConfig.Password).To(Equal("verysecure"))
@@ -100,7 +100,7 @@ var _ = Describe("package_stemcell", func() {
 				Expect(exitStatus).To(Equal(subcommands.ExitSuccess))
 
 				Expect(packagerFactory.PackagerCallCount()).To(Equal(1))
-				_, actualOutputConfig, _, _ := packagerFactory.PackagerArgsForCall(0)
+				_, actualOutputConfig, _ := packagerFactory.PackagerArgsForCall(0)
 				Expect(actualOutputConfig.OutputDir).To(Equal("some_output_dir"))
 			})
 
@@ -114,7 +114,7 @@ var _ = Describe("package_stemcell", func() {
 				Expect(exitStatus).To(Equal(subcommands.ExitSuccess))
 
 				Expect(packagerFactory.PackagerCallCount()).To(Equal(1))
-				_, actualOutputConfig, _, _ := packagerFactory.PackagerArgsForCall(0)
+				_, actualOutputConfig, _ := packagerFactory.PackagerArgsForCall(0)
 				Expect(actualOutputConfig.OutputDir).To(Equal("some_output_dir"))
 				Expect(actualOutputConfig.StemcellVersion).To(Equal("2019.2"))
 				Expect(actualOutputConfig.Os).To(Equal("2019"))
@@ -132,7 +132,7 @@ var _ = Describe("package_stemcell", func() {
 				Expect(exitStatus).To(Equal(subcommands.ExitSuccess))
 
 				Expect(packagerFactory.PackagerCallCount()).To(Equal(1))
-				_, actualOutputConfig, _, _ := packagerFactory.PackagerArgsForCall(0)
+				_, actualOutputConfig, _ := packagerFactory.PackagerArgsForCall(0)
 				Expect(actualOutputConfig.StemcellVersion).To(Equal("1803.27.36"))
 
 				Expect(oSAndVersionGetter.GetVersionWithPatchNumberCallCount()).To(Equal(1))

package_stemcell/factory/packager_factory.go

@@ -2,7 +2,6 @@ package factory
 
 import (
 	"errors"
-	"os"
 	"strings"
 
 	"github.com/cloudfoundry/stembuild/colorlogger"
@@ -16,24 +15,37 @@ import (
 
 type PackagerFactory struct{}
 
-func (f *PackagerFactory) Packager(sourceConfig config.SourceConfig, outputConfig config.OutputConfig, logLevel int, color bool) (commandparser.Packager, error) {
+func (f *PackagerFactory) Packager(sourceConfig config.SourceConfig, outputConfig config.OutputConfig, logger colorlogger.Logger) (commandparser.Packager, error) {
 	source, err := sourceConfig.GetSource()
 	if err != nil {
 		return nil, err
 	}
+
 	switch source {
 	case config.VCENTER:
-		runner := &iaas_cli.GovcRunner{}
-		client := iaas_clients.NewVcenterClient(sourceConfig.Username, sourceConfig.Password, sourceConfig.URL, sourceConfig.CaCertFile, runner)
-		vCenterPackager := &packagers.VCenterPackager{SourceConfig: sourceConfig, OutputConfig: outputConfig, Client: client}
-		return vCenterPackager, nil
+		client :=
+			iaas_clients.NewVcenterClient(
+				sourceConfig.Username,
+				sourceConfig.Password,
+				sourceConfig.URL,
+				sourceConfig.CaCertFile,
+				&iaas_cli.GovcRunner{},
+			)
+
+		return &packagers.VCenterPackager{
+			SourceConfig: sourceConfig,
+			OutputConfig: outputConfig,
+			Client:       client,
+			Logger:       logger,
+		}, nil
 	case config.VMDK:
-		options := package_parameters.VmdkPackageParameters{}
-		logger := colorlogger.ConstructLogger(logLevel, color, os.Stderr)
+		options :=
+			package_parameters.VmdkPackageParameters{}
+
 		vmdkPackager := &packagers.VmdkPackager{
 			Stop:         make(chan struct{}),
-			Debugf:       logger.Debugf,
 			BuildOptions: options,
+			Logger:       logger,
 		}
 
 		vmdkPackager.BuildOptions.VMDKFile = sourceConfig.Vmdk

package_stemcell/factory/packager_factory_test.go

@@ -4,6 +4,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
+	"github.com/cloudfoundry/stembuild/colorlogger"
 	"github.com/cloudfoundry/stembuild/package_stemcell/config"
 	"github.com/cloudfoundry/stembuild/package_stemcell/factory"
 	"github.com/cloudfoundry/stembuild/package_stemcell/packagers"
@@ -18,9 +19,11 @@ var _ = Describe("Factory", func() {
 	}
 
 	var packagerFactory *factory.PackagerFactory
+	var logger colorlogger.Logger
 
 	BeforeEach(func() {
 		packagerFactory = &factory.PackagerFactory{}
+		logger = colorlogger.New(0, false, GinkgoWriter)
 	})
 
 	Describe("GetPackager", func() {
@@ -30,7 +33,7 @@ var _ = Describe("Factory", func() {
 					Vmdk: "path/to/a/vmdk",
 				}
 
-				actualPackager, err := packagerFactory.Packager(sourceConfig, outputConfig, 0, false)
+				actualPackager, err := packagerFactory.Packager(sourceConfig, outputConfig, logger)
 				Expect(err).NotTo(HaveOccurred())
 
 				Expect(actualPackager).To(BeAssignableToTypeOf(&packagers.VmdkPackager{}))
@@ -47,7 +50,7 @@ var _ = Describe("Factory", func() {
 					VmInventoryPath: "some-vm-inventory-path",
 				}
 
-				actualPackager, err := packagerFactory.Packager(sourceConfig, outputConfig, 0, false)
+				actualPackager, err := packagerFactory.Packager(sourceConfig, outputConfig, logger)
 				Expect(err).NotTo(HaveOccurred())
 
 				Expect(actualPackager).To(BeAssignableToTypeOf(&packagers.VCenterPackager{}))
@@ -62,7 +65,7 @@ var _ = Describe("Factory", func() {
 					VmInventoryPath: "some-vm",
 				}
 
-				packager, err := packagerFactory.Packager(sourceConfig, outputConfig, 0, false)
+				packager, err := packagerFactory.Packager(sourceConfig, outputConfig, logger)
 				Expect(err).To(HaveOccurred())
 				Expect(err.Error()).To(Equal("configuration provided for VMDK & vCenter sources"))
 				Expect(packager).To(BeNil())
@@ -77,7 +80,7 @@ var _ = Describe("Factory", func() {
 					URL:             "some-url",
 				}
 
-				packager, err := packagerFactory.Packager(sourceConfig, outputConfig, 0, false)
+				packager, err := packagerFactory.Packager(sourceConfig, outputConfig, logger)
 				Expect(err).To(HaveOccurred())
 				Expect(err.Error()).To(Equal("missing vCenter configurations"))
 				Expect(packager).To(BeNil())
@@ -88,7 +91,7 @@ var _ = Describe("Factory", func() {
 			It("returns an error", func() {
 				sourceConfig := config.SourceConfig{}
 
-				packager, err := packagerFactory.Packager(sourceConfig, outputConfig, 0, false)
+				packager, err := packagerFactory.Packager(sourceConfig, outputConfig, logger)
 				Expect(err).To(HaveOccurred())
 				Expect(err.Error()).To(Equal("no configuration was provided"))
 				Expect(packager).To(BeNil())

package_stemcell/packagers/vcenter_packager.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"regexp"
 
+	"github.com/cloudfoundry/stembuild/colorlogger"
 	"github.com/cloudfoundry/stembuild/filesystem"
 	"github.com/cloudfoundry/stembuild/package_stemcell/config"
 )
@@ -27,6 +28,7 @@ type VCenterPackager struct {
 	SourceConfig config.SourceConfig
 	OutputConfig config.OutputConfig
 	Client       IaasClient
+	Logger       colorlogger.Logger
 }
 
 func (v VCenterPackager) Package() error {