From e853f8b46b35dfa7adf663291bf8d79a95fe9ab8 Mon Sep 17 00:00:00 2001 From: aram price Date: Tue, 23 Jul 2024 15:21:37 -0700 Subject: [PATCH 1/5] winrm_remotemanager wraps errors instead of copying err string --- remotemanager/winrm_remotemanager.go | 13 +++++++------ remotemanager/winrm_remotemanager_test.go | 17 +++++++++-------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/remotemanager/winrm_remotemanager.go b/remotemanager/winrm_remotemanager.go index 1b0c01fd..5beb9298 100644 --- a/remotemanager/winrm_remotemanager.go +++ b/remotemanager/winrm_remotemanager.go @@ -38,13 +38,14 @@ func NewWinRM(host string, username string, password string, clientFactory WinRM } func (w *WinRM) CanReachVM() error { - conn, err := net.DialTimeout("tcp", fmt.Sprintf("%s:%d", w.host, WinRmPort), time.Duration(time.Second*60)) + conn, err := net.DialTimeout("tcp", fmt.Sprintf("%s:%d", w.host, WinRmPort), time.Second*60) if err != nil { - return fmt.Errorf("host %s is unreachable. Please ensure WinRM is enabled and the IP is correct: %s", w.host, err) + return fmt.Errorf("host %s is unreachable; lease ensure WinRM is enabled and the IP is correct: %w", w.host, err) } + err = conn.Close() if err != nil { - return fmt.Errorf("could not close connection to host %s: %s", w.host, err) + return fmt.Errorf("could not close connection to host %s: %w", w.host, err) } return nil @@ -54,17 +55,17 @@ func (w *WinRM) CanLoginVM() error { winrmClient, err := w.clientFactory.Build(WinRmTimeout) if err != nil { - return fmt.Errorf("failed to create winrm client: %s", err) + return fmt.Errorf("failed to create winrm client: %w", err) } s, err := winrmClient.CreateShell() if err != nil { - return fmt.Errorf("failed to create winrm shell: %s", err) + return fmt.Errorf("failed to create winrm shell: %w", err) } err = s.Close() if err != nil { - return fmt.Errorf("failed to close winrm shell: %s", err) + return fmt.Errorf("failed to close winrm shell: %w", err) } return nil diff --git a/remotemanager/winrm_remotemanager_test.go b/remotemanager/winrm_remotemanager_test.go index fb2ba8e5..a75fda75 100644 --- a/remotemanager/winrm_remotemanager_test.go +++ b/remotemanager/winrm_remotemanager_test.go @@ -178,33 +178,34 @@ var _ = Describe("WinRM RemoteManager", func() { remotemanager := remotemanager.NewWinRM("some-host", "some-user", "some-pass", winRMClientFactory) err := remotemanager.CanLoginVM() - Expect(err).NotTo(HaveOccurred()) }) It("returns error if winrmclient cannot be created", func() { winRMClientFactory := new(remotemanagerfakes.FakeWinRMClientFactoryI) - buildError := errors.New("unable to build a client") - winRMClientFactory.BuildReturns(nil, buildError) + buildErr := errors.New("unable to build a client") + winRMClientFactory.BuildReturns(nil, buildErr) remotemanager := remotemanager.NewWinRM("some-host", "some-user", "some-pass", winRMClientFactory) - err := remotemanager.CanLoginVM() + err := remotemanager.CanLoginVM() Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError(fmt.Errorf("failed to create winrm client: %s", buildError))) + Expect(err).To(MatchError(fmt.Errorf("failed to create winrm client: %w", buildErr))) }) + It("returns error if shell cannot be created", func() { winRMClientFactory := new(remotemanagerfakes.FakeWinRMClientFactoryI) winRMClient := new(remotemanagerfakes.FakeWinRMClient) winRMClientFactory.BuildReturns(winRMClient, nil) - winRMClient.CreateShellReturns(nil, errors.New("some shell creation error")) + shellErr := errors.New("some shell creation error") + winRMClient.CreateShellReturns(nil, shellErr) + remotemanager := remotemanager.NewWinRM("some-host", "some-user", "some-pass", winRMClientFactory) err := remotemanager.CanLoginVM() - Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError(fmt.Errorf("failed to create winrm shell: some shell creation error"))) + Expect(err).To(MatchError(fmt.Errorf("failed to create winrm shell: %w", shellErr))) }) }) }) From 4479c6d91791b3afb069f088d1093b4e250e23e2 Mon Sep 17 00:00:00 2001 From: aram price Date: Tue, 23 Jul 2024 16:12:05 -0700 Subject: [PATCH 2/5] colorlogger package looks more like log.Logger --- colorlogger/colorLogger.go | 50 +++++++++++--------- colorlogger/colorlogger_test.go | 25 +++------- package_stemcell/factory/packager_factory.go | 4 +- 3 files changed, 36 insertions(+), 43 deletions(-) diff --git a/colorlogger/colorLogger.go b/colorlogger/colorLogger.go index aa8c184b..59bb23ce 100644 --- a/colorlogger/colorLogger.go +++ b/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) } diff --git a/colorlogger/colorlogger_test.go b/colorlogger/colorlogger_test.go index 6b4c02e5..bebb798e 100644 --- a/colorlogger/colorlogger_test.go +++ b/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")) }) - }) diff --git a/package_stemcell/factory/packager_factory.go b/package_stemcell/factory/packager_factory.go index a4220c79..4113b443 100644 --- a/package_stemcell/factory/packager_factory.go +++ b/package_stemcell/factory/packager_factory.go @@ -29,10 +29,10 @@ func (f *PackagerFactory) Packager(sourceConfig config.SourceConfig, outputConfi return vCenterPackager, nil case config.VMDK: options := package_parameters.VmdkPackageParameters{} - logger := colorlogger.ConstructLogger(logLevel, color, os.Stderr) + logger := colorlogger.New(logLevel, color, os.Stderr) vmdkPackager := &packagers.VmdkPackager{ Stop: make(chan struct{}), - Debugf: logger.Debugf, + Debugf: logger.Printf, BuildOptions: options, } From 7436bafc9f484d4c8dbceaa6b402c44bb5f1109d Mon Sep 17 00:00:00 2001 From: aram price Date: Tue, 23 Jul 2024 16:25:32 -0700 Subject: [PATCH 3/5] package stemcells command constructs loger ... instead of having the packager_factory costruct it --- .../fake_packager_factory.go | 23 +++++++++---------- commandparser/package_stemcell.go | 5 ++-- commandparser/package_stemcell_test.go | 10 ++++---- package_stemcell/factory/packager_factory.go | 5 ++-- .../factory/packager_factory_test.go | 13 +++++++---- 5 files changed, 29 insertions(+), 27 deletions(-) diff --git a/commandparser/commandparserfakes/fake_packager_factory.go b/commandparser/commandparserfakes/fake_packager_factory.go index 6c337c3a..fdeffff6 100644 --- a/commandparser/commandparserfakes/fake_packager_factory.go +++ b/commandparser/commandparserfakes/fake_packager_factory.go @@ -4,18 +4,18 @@ package commandparserfakes import ( "sync" + "github.com/cloudfoundry/stembuild/colorlogger" "github.com/cloudfoundry/stembuild/commandparser" "github.com/cloudfoundry/stembuild/package_stemcell/config" ) type FakePackagerFactory struct { - PackagerStub func(config.SourceConfig, config.OutputConfig, int, bool) (commandparser.Packager, error) + PackagerStub func(config.SourceConfig, config.OutputConfig, colorlogger.Logger) (commandparser.Packager, error) packagerMutex sync.RWMutex packagerArgsForCall []struct { arg1 config.SourceConfig arg2 config.OutputConfig - arg3 int - arg4 bool + arg3 colorlogger.Logger } packagerReturns struct { result1 commandparser.Packager @@ -29,21 +29,20 @@ type FakePackagerFactory struct { invocationsMutex sync.RWMutex } -func (fake *FakePackagerFactory) Packager(arg1 config.SourceConfig, arg2 config.OutputConfig, arg3 int, arg4 bool) (commandparser.Packager, error) { +func (fake *FakePackagerFactory) Packager(arg1 config.SourceConfig, arg2 config.OutputConfig, arg3 colorlogger.Logger) (commandparser.Packager, error) { fake.packagerMutex.Lock() ret, specificReturn := fake.packagerReturnsOnCall[len(fake.packagerArgsForCall)] fake.packagerArgsForCall = append(fake.packagerArgsForCall, struct { arg1 config.SourceConfig arg2 config.OutputConfig - arg3 int - arg4 bool - }{arg1, arg2, arg3, arg4}) + arg3 colorlogger.Logger + }{arg1, arg2, arg3}) stub := fake.PackagerStub fakeReturns := fake.packagerReturns - fake.recordInvocation("Packager", []interface{}{arg1, arg2, arg3, arg4}) + fake.recordInvocation("Packager", []interface{}{arg1, arg2, arg3}) fake.packagerMutex.Unlock() if stub != nil { - return stub(arg1, arg2, arg3, arg4) + return stub(arg1, arg2, arg3) } if specificReturn { return ret.result1, ret.result2 @@ -57,17 +56,17 @@ func (fake *FakePackagerFactory) PackagerCallCount() int { return len(fake.packagerArgsForCall) } -func (fake *FakePackagerFactory) PackagerCalls(stub func(config.SourceConfig, config.OutputConfig, int, bool) (commandparser.Packager, error)) { +func (fake *FakePackagerFactory) PackagerCalls(stub func(config.SourceConfig, config.OutputConfig, colorlogger.Logger) (commandparser.Packager, error)) { fake.packagerMutex.Lock() defer fake.packagerMutex.Unlock() fake.PackagerStub = stub } -func (fake *FakePackagerFactory) PackagerArgsForCall(i int) (config.SourceConfig, config.OutputConfig, int, bool) { +func (fake *FakePackagerFactory) PackagerArgsForCall(i int) (config.SourceConfig, config.OutputConfig, colorlogger.Logger) { fake.packagerMutex.RLock() defer fake.packagerMutex.RUnlock() argsForCall := fake.packagerArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 } func (fake *FakePackagerFactory) PackagerReturns(result1 commandparser.Packager, result2 error) { diff --git a/commandparser/package_stemcell.go b/commandparser/package_stemcell.go index c28589d7..2b9967fd 100644 --- a/commandparser/package_stemcell.go +++ b/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 diff --git a/commandparser/package_stemcell_test.go b/commandparser/package_stemcell_test.go index 76caca55..2a4791f2 100644 --- a/commandparser/package_stemcell_test.go +++ b/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)) diff --git a/package_stemcell/factory/packager_factory.go b/package_stemcell/factory/packager_factory.go index 4113b443..5ac10dd2 100644 --- a/package_stemcell/factory/packager_factory.go +++ b/package_stemcell/factory/packager_factory.go @@ -2,7 +2,6 @@ package factory import ( "errors" - "os" "strings" "github.com/cloudfoundry/stembuild/colorlogger" @@ -16,7 +15,7 @@ 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 @@ -29,7 +28,7 @@ func (f *PackagerFactory) Packager(sourceConfig config.SourceConfig, outputConfi return vCenterPackager, nil case config.VMDK: options := package_parameters.VmdkPackageParameters{} - logger := colorlogger.New(logLevel, color, os.Stderr) + vmdkPackager := &packagers.VmdkPackager{ Stop: make(chan struct{}), Debugf: logger.Printf, diff --git a/package_stemcell/factory/packager_factory_test.go b/package_stemcell/factory/packager_factory_test.go index f04b0707..447a548b 100644 --- a/package_stemcell/factory/packager_factory_test.go +++ b/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()) From 0e3cf2ea61d6e5ddb6efd40141171bc959684d90 Mon Sep 17 00:00:00 2001 From: aram price Date: Tue, 23 Jul 2024 16:50:54 -0700 Subject: [PATCH 4/5] VmdkPackager uses `.logger.Printf()` instead of bespoke `.Debugf()` --- package_stemcell/factory/packager_factory.go | 6 ++- package_stemcell/packagers/vmdk_packager.go | 37 ++++++++++--------- .../packagers/vmdk_packager_test.go | 3 +- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/package_stemcell/factory/packager_factory.go b/package_stemcell/factory/packager_factory.go index 5ac10dd2..a7df1187 100644 --- a/package_stemcell/factory/packager_factory.go +++ b/package_stemcell/factory/packager_factory.go @@ -20,6 +20,7 @@ func (f *PackagerFactory) Packager(sourceConfig config.SourceConfig, outputConfi if err != nil { return nil, err } + switch source { case config.VCENTER: runner := &iaas_cli.GovcRunner{} @@ -27,12 +28,13 @@ func (f *PackagerFactory) Packager(sourceConfig config.SourceConfig, outputConfi vCenterPackager := &packagers.VCenterPackager{SourceConfig: sourceConfig, OutputConfig: outputConfig, Client: client} return vCenterPackager, nil case config.VMDK: - options := package_parameters.VmdkPackageParameters{} + options := + package_parameters.VmdkPackageParameters{} vmdkPackager := &packagers.VmdkPackager{ Stop: make(chan struct{}), - Debugf: logger.Printf, BuildOptions: options, + Logger: logger, } vmdkPackager.BuildOptions.VMDKFile = sourceConfig.Vmdk diff --git a/package_stemcell/packagers/vmdk_packager.go b/package_stemcell/packagers/vmdk_packager.go index d2085dff..948afcc1 100644 --- a/package_stemcell/packagers/vmdk_packager.go +++ b/package_stemcell/packagers/vmdk_packager.go @@ -14,6 +14,7 @@ import ( "path/filepath" "time" + "github.com/cloudfoundry/stembuild/colorlogger" "github.com/cloudfoundry/stembuild/filesystem" "github.com/cloudfoundry/stembuild/package_stemcell/ovftool" "github.com/cloudfoundry/stembuild/package_stemcell/package_parameters" @@ -29,8 +30,8 @@ type VmdkPackager struct { Sha1sum string tmpdir string Stop chan struct{} - Debugf func(format string, a ...interface{}) BuildOptions package_parameters.VmdkPackageParameters + Logger colorlogger.Logger } var ErrInterrupt = errors.New("interrupt") @@ -74,7 +75,7 @@ func (c *VmdkPackager) Reader(r io.Reader) *CancelReader { } func (c *VmdkPackager) StopConfig() { - c.Debugf("stopping config") + c.Logger.Printf("stopping config") defer c.Cleanup() // make sure this runs! close(c.Stop) } @@ -85,13 +86,13 @@ func (c *VmdkPackager) Cleanup() { } // check if directory exists to make Cleanup idempotent if _, err := os.Stat(c.tmpdir); err == nil { - c.Debugf("deleting temp directory: %s", c.tmpdir) + c.Logger.Printf("deleting temp directory: %s", c.tmpdir) os.RemoveAll(c.tmpdir) } } func (c *VmdkPackager) AddTarFile(tr *tar.Writer, name string) error { - c.Debugf("adding file (%s) to tar archive", name) + c.Logger.Printf("adding file (%s) to tar archive", name) f, err := os.Open(name) if err != nil { return err @@ -117,7 +118,7 @@ func (c *VmdkPackager) AddTarFile(tr *tar.Writer, name string) error { func (c *VmdkPackager) TempDir() (string, error) { if c.tmpdir != "" { if _, err := os.Stat(c.tmpdir); err != nil { - c.Debugf("unable to stat temp dir (%s) was it deleted?", c.tmpdir) + c.Logger.Printf("unable to stat temp dir (%s) was it deleted?", c.tmpdir) return "", fmt.Errorf("opening temp directory: %s", c.tmpdir) } return c.tmpdir, nil @@ -127,12 +128,12 @@ func (c *VmdkPackager) TempDir() (string, error) { return "", fmt.Errorf("creating temp directory: %s", err) } c.tmpdir = name - c.Debugf("created temp directory: %s", name) + c.Logger.Printf("created temp directory: %s", name) return c.tmpdir, nil } func (c *VmdkPackager) CreateStemcell() error { - c.Debugf("creating stemcell") + c.Logger.Printf("creating stemcell") // programming errors - panic! if c.Manifest == "" { @@ -153,7 +154,7 @@ func (c *VmdkPackager) CreateStemcell() error { return err } defer stemcell.Close() - c.Debugf("created temp stemcell: %s", c.Stemcell) + c.Logger.Printf("created temp stemcell: %s", c.Stemcell) errorf := func(format string, a ...interface{}) error { stemcell.Close() @@ -165,12 +166,12 @@ func (c *VmdkPackager) CreateStemcell() error { w := gzip.NewWriter(c.Writer(stemcell)) tr := tar.NewWriter(w) - c.Debugf("adding image file to stemcell tarball: %s", c.Image) + c.Logger.Printf("adding image file to stemcell tarball: %s", c.Image) if err := c.AddTarFile(tr, c.Image); err != nil { return errorf("creating stemcell: %s", err) } - c.Debugf("adding manifest file to stemcell tarball: %s", c.Manifest) + c.Logger.Printf("adding manifest file to stemcell tarball: %s", c.Manifest) if err := c.AddTarFile(tr, c.Manifest); err != nil { return errorf("creating stemcell: %s", err) } @@ -183,7 +184,7 @@ func (c *VmdkPackager) CreateStemcell() error { return errorf("creating stemcell: %s", err) } - c.Debugf("created stemcell in: %s", time.Since(t)) + c.Logger.Printf("created stemcell in: %s", time.Since(t)) return nil } @@ -209,7 +210,7 @@ func (c *VmdkPackager) ConvertVMX2OVA(vmx, ova string) error { if err := cmd.Start(); err != nil { return fmt.Errorf("ovftool: %s", err) } - c.Debugf("converting vmx to ova with cmd: %s %s", cmd.Path, cmd.Args[1:]) + c.Logger.Printf("converting vmx to ova with cmd: %s %s", cmd.Path, cmd.Args[1:]) // Wait for process exit or interupt errCh := make(chan error, 1) @@ -218,7 +219,7 @@ func (c *VmdkPackager) ConvertVMX2OVA(vmx, ova string) error { select { case <-c.Stop: if cmd.Process != nil { - c.Debugf("received stop signall killing ovftool process") + c.Logger.Printf("received stop signall killing ovftool process") cmd.Process.Kill() //nolint:errcheck } return ErrInterrupt @@ -234,7 +235,7 @@ func (c *VmdkPackager) ConvertVMX2OVA(vmx, ova string) error { // CreateImage converts a vmdk to a gzip compressed image file and records the // sha1 sum of the resulting image. func (c *VmdkPackager) CreateImage() error { - c.Debugf("Creating [image] from [vmdk]: %s", c.BuildOptions.VMDKFile) + c.Logger.Printf("Creating [image] from [vmdk]: %s", c.BuildOptions.VMDKFile) tmpdir, err := c.TempDir() if err != nil { @@ -290,7 +291,7 @@ func (c *VmdkPackager) CreateImage() error { } c.Sha1sum = fmt.Sprintf("%x", h.Sum(nil)) - c.Debugf("Sha1 of image (%s): %s", c.Image, c.Sha1sum) + c.Logger.Printf("Sha1 of image (%s): %s", c.Image, c.Sha1sum) return nil } @@ -314,7 +315,7 @@ func (c *VmdkPackager) ConvertVMDK() (string, error) { } stemcellPath := filepath.Join(c.BuildOptions.OutputDir, filepath.Base(c.Stemcell)) - c.Debugf("moving stemcell (%s) to: %s", c.Stemcell, stemcellPath) + c.Logger.Printf("moving stemcell (%s) to: %s", c.Stemcell, stemcellPath) if err := os.Rename(c.Stemcell, stemcellPath); err != nil { return "", err @@ -327,7 +328,7 @@ func (c *VmdkPackager) catchInterruptSignal() { signal.Notify(ch, os.Interrupt) stopping := false for sig := range ch { - c.Debugf("received signal: %s", sig) + c.Logger.Printf("received signal: %s", sig) if stopping { fmt.Fprintf(os.Stderr, "received second (%s) signal - exiting now\n", sig) c.Cleanup() // remove temp dir @@ -352,7 +353,7 @@ func (c *VmdkPackager) Package() error { return err } - c.Debugf("created stemcell (%s) in: %s", stemcellPath, time.Since(start)) + c.Logger.Printf("created stemcell (%s) in: %s", stemcellPath, time.Since(start)) fmt.Printf("created stemcell: %s", stemcellPath) c.Cleanup() diff --git a/package_stemcell/packagers/vmdk_packager_test.go b/package_stemcell/packagers/vmdk_packager_test.go index a09be07d..806db368 100644 --- a/package_stemcell/packagers/vmdk_packager_test.go +++ b/package_stemcell/packagers/vmdk_packager_test.go @@ -12,6 +12,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/cloudfoundry/stembuild/colorlogger" mockfilesystem "github.com/cloudfoundry/stembuild/filesystem/mock" "github.com/cloudfoundry/stembuild/package_stemcell/package_parameters" "github.com/cloudfoundry/stembuild/package_stemcell/packagers" @@ -30,8 +31,8 @@ var _ = Describe("VmdkPackager", func() { vmdkPackager = packagers.VmdkPackager{ Stop: make(chan struct{}), - Debugf: func(format string, a ...interface{}) {}, BuildOptions: stembuildConfig, + Logger: colorlogger.New(0, false, GinkgoWriter), } }) From 310e42c78ea190d40dfabbbdc38b2b4b21888c94 Mon Sep 17 00:00:00 2001 From: aram price Date: Tue, 23 Jul 2024 16:52:02 -0700 Subject: [PATCH 5/5] VCenterPackager has `Logger` available - packager_factory passes logger when constructing --- package_stemcell/factory/packager_factory.go | 19 +++++++++++++++---- .../packagers/vcenter_packager.go | 2 ++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/package_stemcell/factory/packager_factory.go b/package_stemcell/factory/packager_factory.go index a7df1187..83095c4b 100644 --- a/package_stemcell/factory/packager_factory.go +++ b/package_stemcell/factory/packager_factory.go @@ -23,10 +23,21 @@ func (f *PackagerFactory) Packager(sourceConfig config.SourceConfig, outputConfi 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{} diff --git a/package_stemcell/packagers/vcenter_packager.go b/package_stemcell/packagers/vcenter_packager.go index 9d564ea1..feb73c17 100644 --- a/package_stemcell/packagers/vcenter_packager.go +++ b/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 {