From dc5ae8107af28a22ea930922f7387226c8d55479 Mon Sep 17 00:00:00 2001 From: Moji Date: Mon, 14 Oct 2024 11:29:27 +0330 Subject: [PATCH 1/2] chore: pass a custom logger from user for the builder factory (#571) * chore: pass a custom logger from user for the builder factory * fix: imagebuilder empty check * chore: applied feedback --- pkg/container/docker.go | 84 +++++++++++++++++++++++++++-------------- pkg/container/errors.go | 4 ++ pkg/instance/build.go | 16 +++++++- 3 files changed, 73 insertions(+), 31 deletions(-) diff --git a/pkg/container/docker.go b/pkg/container/docker.go index ec35d50e..c63cbc7c 100644 --- a/pkg/container/docker.go +++ b/pkg/container/docker.go @@ -22,20 +22,34 @@ type BuilderFactory struct { dockerFileInstructions []string buildContext string args []builder.ArgInterface + logger *logrus.Logger +} + +type BuilderFactoryOptions struct { + ImageName string + BuildContext string + ImageBuilder builder.Builder + Args []builder.ArgInterface + Logger *logrus.Logger } // NewBuilderFactory creates a new instance of BuilderFactory. -func NewBuilderFactory(imageName, buildContext string, imageBuilder builder.Builder, args []builder.ArgInterface) (*BuilderFactory, error) { - if err := os.MkdirAll(buildContext, 0755); err != nil { +func NewBuilderFactory(opts BuilderFactoryOptions) (*BuilderFactory, error) { + if err := verifyOptions(opts); err != nil { + return nil, err + } + + if err := os.MkdirAll(opts.BuildContext, 0755); err != nil { return nil, ErrFailedToCreateContextDir.Wrap(err) } return &BuilderFactory{ - imageNameFrom: imageName, - dockerFileInstructions: []string{"FROM " + imageName}, - buildContext: buildContext, - imageBuilder: imageBuilder, - args: args, + imageNameFrom: opts.ImageName, + dockerFileInstructions: []string{"FROM " + opts.ImageName}, + buildContext: opts.BuildContext, + imageBuilder: opts.ImageBuilder, + args: opts.Args, + logger: opts.Logger, }, nil } @@ -73,7 +87,7 @@ func (f *BuilderFactory) Changed() bool { // The image is identified by the provided name. func (f *BuilderFactory) PushBuilderImage(ctx context.Context, imageName string) error { if !f.Changed() { - logrus.Debugf("No changes made to image %s, skipping push", f.imageNameFrom) + f.logger.Debugf("No changes made to image %s, skipping push", f.imageNameFrom) return nil } @@ -94,6 +108,10 @@ func (f *BuilderFactory) PushBuilderImage(ctx context.Context, imageName string) return ErrFailedToWriteDockerfile.Wrap(err) } + if f.imageBuilder == nil { + return ErrImageBuilderNotSet + } + logs, err := f.imageBuilder.Build(ctx, &builder.BuilderOptions{ ImageName: f.imageNameTo, Destination: f.imageNameTo, // in docker the image name and destination are the same @@ -101,15 +119,7 @@ func (f *BuilderFactory) PushBuilderImage(ctx context.Context, imageName string) Args: f.args, }) - qStatus := logrus.TextFormatter{}.DisableQuote - logrus.SetFormatter(&logrus.TextFormatter{ - DisableQuote: true, - }) - logrus.Debug("build logs: ", logs) - logrus.SetFormatter(&logrus.TextFormatter{ - DisableQuote: qStatus, - }) - + f.logDebugWithQuotesDisabled("build logs: ", logs) return err } @@ -129,7 +139,11 @@ func (f *BuilderFactory) BuildImageFromGitRepo(ctx context.Context, gitCtx build return ErrFailedToGetDefaultCacheOptions.Wrap(err) } - logrus.Debugf("Building image %s from git repo %s", imageName, gitCtx.Repo) + f.logger.Debugf("Building image %s from git repo %s", imageName, gitCtx.Repo) + + if f.imageBuilder == nil { + return ErrImageBuilderNotSet + } logs, err := f.imageBuilder.Build(ctx, &builder.BuilderOptions{ ImageName: imageName, @@ -139,16 +153,7 @@ func (f *BuilderFactory) BuildImageFromGitRepo(ctx context.Context, gitCtx build Args: f.args, }) - qStatus := logrus.TextFormatter{}.DisableQuote - logrus.SetFormatter(&logrus.TextFormatter{ - DisableQuote: true, - }) - - logrus.Debug("build logs: ", logs) - - logrus.SetFormatter(&logrus.TextFormatter{ - DisableQuote: qStatus, - }) + f.logDebugWithQuotesDisabled("build logs: ", logs) return err } @@ -184,7 +189,28 @@ func (f *BuilderFactory) GenerateImageHash() (string, error) { return "", ErrHashingBuildContext.Wrap(err) } - logrus.Debug("Generated image hash: ", fmt.Sprintf("%x", hasher.Sum(nil))) + f.logger.Debug("Generated image hash: ", fmt.Sprintf("%x", hasher.Sum(nil))) return fmt.Sprintf("%x", hasher.Sum(nil)), nil } + +func verifyOptions(opts BuilderFactoryOptions) error { + if opts.ImageName == "" { + return ErrImageNameEmpty + } + if opts.BuildContext == "" { + return ErrBuildContextEmpty + } + if opts.Logger == nil { + return ErrLoggerEmpty + } + return nil +} + +func (f *BuilderFactory) logDebugWithQuotesDisabled(args ...interface{}) { + lf := f.logger.Formatter.(*logrus.TextFormatter) + qStatus := lf.DisableQuote + lf.DisableQuote = true + f.logger.Debug(args...) + lf.DisableQuote = qStatus +} diff --git a/pkg/container/errors.go b/pkg/container/errors.go index ad866c3b..bfeec0a5 100644 --- a/pkg/container/errors.go +++ b/pkg/container/errors.go @@ -25,4 +25,8 @@ var ( ErrReadingFile = errors.New("ReadingFile", "error reading file: %s") ErrHashingFile = errors.New("HashingFile", "error hashing file %s") ErrHashingBuildContext = errors.New("HashingBuildContext", "error hashing build context") + ErrImageNameEmpty = errors.New("ImageNameEmpty", "image name is empty") + ErrBuildContextEmpty = errors.New("BuildContextEmpty", "build context is empty") + ErrImageBuilderNotSet = errors.New("ImageBuilderNotSet", "image builder is not set") + ErrLoggerEmpty = errors.New("LoggerEmpty", "logger is empty") ) diff --git a/pkg/instance/build.go b/pkg/instance/build.go index ac9c96e7..aa1bb549 100644 --- a/pkg/instance/build.go +++ b/pkg/instance/build.go @@ -54,7 +54,13 @@ func (b *build) SetImage(ctx context.Context, image string, args ...builder.ArgI } // Use the builder to build a new image - factory, err := container.NewBuilderFactory(image, b.getBuildDir(), b.instance.ImageBuilder, args) + factory, err := container.NewBuilderFactory(container.BuilderFactoryOptions{ + ImageName: image, + BuildContext: b.getBuildDir(), + ImageBuilder: b.instance.ImageBuilder, + Args: args, + Logger: b.instance.Logger, + }) if err != nil { return ErrCreatingBuilder.Wrap(err) } @@ -80,7 +86,13 @@ func (b *build) SetGitRepo(ctx context.Context, gitContext builder.GitContext, a return ErrGettingImageName.Wrap(err) } - factory, err := container.NewBuilderFactory(imageName, b.getBuildDir(), b.instance.ImageBuilder, args) + factory, err := container.NewBuilderFactory(container.BuilderFactoryOptions{ + ImageName: imageName, + BuildContext: b.getBuildDir(), + ImageBuilder: b.instance.ImageBuilder, + Args: args, + Logger: b.instance.Logger, + }) if err != nil { return ErrCreatingBuilder.Wrap(err) } From f86e86ca65c758ceaef4cf72d78d1352a2756089 Mon Sep 17 00:00:00 2001 From: Moji Date: Mon, 14 Oct 2024 15:58:22 +0330 Subject: [PATCH 2/2] chore: add a check for Instance state when cloning (#570) * chore: add a check for Instance state when cloning * chore: update the func comment * fix: a failing unit test --- pkg/instance/errors.go | 1 + pkg/instance/instance.go | 6 +++++- pkg/sidecars/tshark/tshark_test.go | 13 +++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/pkg/instance/errors.go b/pkg/instance/errors.go index 497c09bc..6c1e42e4 100644 --- a/pkg/instance/errors.go +++ b/pkg/instance/errors.go @@ -220,4 +220,5 @@ var ( ErrAddingHostToProxyNotAllowed = errors.New("AddingHostToProxyNotAllowed", "adding host to proxy is only allowed in state 'Started' and 'Preparing'. Current state is '%s'") ErrInstanceNameAlreadyExists = errors.New("InstanceNameAlreadyExists", "instance name '%s' already exists") ErrSettingSidecarName = errors.New("SettingSidecarName", "error setting sidecar name with prefix '%s' for instance '%s'") + ErrCannotCloneInstance = errors.New("CannotCloneInstance", "cannot clone instance '%s' in state '%s'") ) diff --git a/pkg/instance/instance.go b/pkg/instance/instance.go index 878c7136..d9aa769a 100644 --- a/pkg/instance/instance.go +++ b/pkg/instance/instance.go @@ -137,10 +137,14 @@ func (i *Instance) CloneWithSuffix(suffix string) (*Instance, error) { } // CloneWithName creates a clone of the instance with a given name -// This function can only be called in the state 'Committed' +// This function can only be called in the state 'Committed' or 'Stopped' // When cloning an instance that is a sidecar, the clone will be not a sidecar // When cloning an instance with sidecars, the sidecars will be cloned as well func (i *Instance) CloneWithName(name string) (*Instance, error) { + if !i.IsInState(StateCommitted, StateStopped) { + return nil, ErrCannotCloneInstance.WithParams(i.name, i.state) + } + clonedSidecars, err := i.sidecars.clone(name) if err != nil { return nil, err diff --git a/pkg/sidecars/tshark/tshark_test.go b/pkg/sidecars/tshark/tshark_test.go index e91eadcc..47031fbe 100644 --- a/pkg/sidecars/tshark/tshark_test.go +++ b/pkg/sidecars/tshark/tshark_test.go @@ -169,7 +169,16 @@ func TestTsharkValidateConfig(t *testing.T) { } func TestTsharkClone(t *testing.T) { - testInstance, err := instance.New("testInstance", &system.SystemDependencies{}) + testInstance, err := instance.New("testInstance", + &system.SystemDependencies{ + Logger: logrus.New(), + }) + require.NoError(t, err) + + err = testInstance.Build().SetImage(context.Background(), "testImage") + require.NoError(t, err) + + err = testInstance.Build().Commit(context.Background()) require.NoError(t, err) tshark := &Tshark{ @@ -184,8 +193,8 @@ func TestTsharkClone(t *testing.T) { UploadInterval: time.Minute * 5, instance: testInstance, } - clonePrefixName := "test-clone-prefix" + clone, err := tshark.Clone(clonePrefixName) require.NoError(t, err)