From 7ccdc440e1ea13f53c454d177af6211628791fbc Mon Sep 17 00:00:00 2001 From: Kyle Quest Date: Tue, 24 Sep 2024 20:33:03 -0700 Subject: [PATCH] imagebuild cleanup and fixes Signed-off-by: Kyle Quest --- pkg/app/master/command/cliprompt.go | 9 ++ pkg/app/master/command/imagebuild/cli.go | 25 +++++- pkg/app/master/command/imagebuild/flags.go | 2 +- pkg/app/master/command/imagebuild/prompt.go | 2 + .../docker/dockercrtclient/dockercrtclient.go | 19 ++-- .../podman/podmancrtclient/podmancrtclient.go | 38 ++++---- pkg/imagebuilder/standardbuilder/engine.go | 90 +------------------ 7 files changed, 66 insertions(+), 119 deletions(-) diff --git a/pkg/app/master/command/cliprompt.go b/pkg/app/master/command/cliprompt.go index 92659e35..5f3ff392 100644 --- a/pkg/app/master/command/cliprompt.go +++ b/pkg/app/master/command/cliprompt.go @@ -26,6 +26,7 @@ import ( type InteractiveApp struct { appPrompt *prompt.Prompt fpCompleter completer.FilePathCompleter + dpCompleter completer.FilePathCompleter app *cli.App dclient *dockerapi.Client crtConnection string @@ -38,6 +39,10 @@ func NewInteractiveApp(app *cli.App, gparams *GenericParams) *InteractiveApp { fpCompleter: completer.FilePathCompleter{ IgnoreCase: true, }, + dpCompleter: completer.FilePathCompleter{ + IgnoreCase: true, + Filter: func(fi os.FileInfo) bool { return fi.IsDir() }, + }, } ia.appPrompt = prompt.New( @@ -507,6 +512,10 @@ func CompleteFile(ia *InteractiveApp, token string, params prompt.Document) []pr return ia.fpCompleter.Complete(params) } +func CompleteDir(ia *InteractiveApp, token string, params prompt.Document) []prompt.Suggest { + return ia.dpCompleter.Complete(params) +} + // Runtime var runtimeValues = []prompt.Suggest{ diff --git a/pkg/app/master/command/imagebuild/cli.go b/pkg/app/master/command/imagebuild/cli.go index f8888cae..6a1880ce 100644 --- a/pkg/app/master/command/imagebuild/cli.go +++ b/pkg/app/master/command/imagebuild/cli.go @@ -3,8 +3,10 @@ package imagebuild import ( "fmt" "os" + "path/filepath" "strings" + log "github.com/sirupsen/logrus" "github.com/urfave/cli/v2" "github.com/mintoolkit/mint/pkg/app" @@ -43,8 +45,11 @@ var CLI = &cli.Command{ Usage: Usage, Flags: ImageBuildFlags, Action: func(ctx *cli.Context) error { + logger := log.WithFields(log.Fields{"app": command.AppName, "cmd": Name, "op": "cli.Action"}) + gcvalues, ok := command.CLIContextGet(ctx.Context, command.GlobalParams).(*command.GenericParams) if !ok || gcvalues == nil { + logger.Error("no gcvalues") return command.ErrNoGlobalParams } @@ -64,6 +69,7 @@ var CLI = &cli.Command{ ContextDir: ctx.String(FlagContextDir), Runtime: ctx.String(FlagRuntimeLoad), Architecture: ctx.String(FlagArchitecture), + Labels: map[string]string{}, } cboBuildArgs := command.ParseKVParams(ctx.StringSlice(FlagBuildArg)) @@ -79,6 +85,7 @@ var CLI = &cli.Command{ engineProps, found := BuildEngines[cparams.Engine] if !found { + logger.Errorf("engine not found - %s", cparams.Engine) return command.ErrBadParamValue } @@ -86,12 +93,23 @@ var CLI = &cli.Command{ cparams.Dockerfile = DefaultDockerfilePath } - if !fsutil.Exists(cparams.Dockerfile) { + if !fsutil.DirExists(cparams.ContextDir) { + logger.Errorf("context dir not found - %s", cparams.ContextDir) return command.ErrBadParamValue } - if !fsutil.DirExists(cparams.ContextDir) { - return command.ErrBadParamValue + switch cparams.Engine { + case BuildkitBuildEngine, DepotBuildEngine: + if !fsutil.Exists(cparams.Dockerfile) { + logger.Errorf("Dockerfile not found - '%s'", cparams.Dockerfile) + return command.ErrBadParamValue + } + default: + fullDockerfilePath := filepath.Join(cparams.ContextDir, cparams.Dockerfile) + if !fsutil.Exists(fullDockerfilePath) { + logger.Errorf("Dockerfile not found - '%s' ('%s')", cparams.Dockerfile, fullDockerfilePath) + return command.ErrBadParamValue + } } if cparams.Architecture == "" { @@ -99,6 +117,7 @@ var CLI = &cli.Command{ } if !IsArchValue(cparams.Architecture) { + logger.Errorf("architecture not supported - %s", cparams.Architecture) return command.ErrBadParamValue } diff --git a/pkg/app/master/command/imagebuild/flags.go b/pkg/app/master/command/imagebuild/flags.go index 1dc20361..c4d39239 100644 --- a/pkg/app/master/command/imagebuild/flags.go +++ b/pkg/app/master/command/imagebuild/flags.go @@ -31,7 +31,7 @@ const ( FlagImageArchiveFileUsage = "local file path for the image tar archive file" FlagDockerfile = "dockerfile" - FlagDockerfileUsage = "local Dockerfile path" + FlagDockerfileUsage = "local Dockerfile path (for buildkit and depot) or a relative to the build context directory (for docker or podman)" FlagContextDir = "context-dir" FlagContextDirUsage = "local build context directory" diff --git a/pkg/app/master/command/imagebuild/prompt.go b/pkg/app/master/command/imagebuild/prompt.go index 8a96d625..2dbc9352 100644 --- a/pkg/app/master/command/imagebuild/prompt.go +++ b/pkg/app/master/command/imagebuild/prompt.go @@ -54,11 +54,13 @@ var CommandFlagSuggestions = &command.FlagSuggestions{ {Text: command.FullFlagName(FlagDockerfile), Description: FlagDockerfileUsage}, {Text: command.FullFlagName(FlagContextDir), Description: FlagContextDirUsage}, {Text: command.FullFlagName(FlagBuildArg), Description: FlagBuildArgUsage}, + {Text: command.FullFlagName(FlagLabel), Description: FlagLabelUsage}, {Text: command.FullFlagName(FlagArchitecture), Description: FlagArchitectureUsage}, }, Values: map[string]command.CompleteValue{ command.FullFlagName(FlagEngine): completeBuildEngine, command.FullFlagName(FlagRuntimeLoad): completeRuntimeLoad, command.FullFlagName(FlagArchitecture): completeArchitecture, + command.FullFlagName(FlagContextDir): command.CompleteDir, }, } diff --git a/pkg/crt/docker/dockercrtclient/dockercrtclient.go b/pkg/crt/docker/dockercrtclient/dockercrtclient.go index 426ac6db..b1284e22 100644 --- a/pkg/crt/docker/dockercrtclient/dockercrtclient.go +++ b/pkg/crt/docker/dockercrtclient/dockercrtclient.go @@ -63,6 +63,10 @@ func (ref *Instance) BuildImage(options imagebuilder.DockerfileBuildOptions) err options.Labels = labels } + if options.BuildContext == "" { + options.BuildContext = "." + } + //not using options.CacheTo in this image builder... buildOptions := docker.BuildImageOptions{ Dockerfile: options.Dockerfile, @@ -90,21 +94,16 @@ func (ref *Instance) BuildImage(options imagebuilder.DockerfileBuildOptions) err } else { if exists := fsutil.DirExists(options.BuildContext); exists { buildOptions.ContextDir = options.BuildContext + //Dockerfile path is expected to be relative to build context + fullDockerfileName := filepath.Join(buildOptions.ContextDir, buildOptions.Dockerfile) + if !fsutil.Exists(fullDockerfileName) || !fsutil.IsRegularFile(fullDockerfileName) { + return fmt.Errorf("invalid dockerfile reference (%s) - %s", buildOptions.Dockerfile, fullDockerfileName) + } } else { return imagebuilder.ErrInvalidContextDir } } - if !fsutil.Exists(buildOptions.Dockerfile) || !fsutil.IsRegularFile(buildOptions.Dockerfile) { - //a slightly hacky behavior using the build context directory if the dockerfile flag doesn't include a usable path - fullDockerfileName := filepath.Join(buildOptions.ContextDir, buildOptions.Dockerfile) - if !fsutil.Exists(fullDockerfileName) || !fsutil.IsRegularFile(fullDockerfileName) { - return fmt.Errorf("invalid dockerfile reference - %s", fullDockerfileName) - } - - buildOptions.Dockerfile = fullDockerfileName - } - if options.OutputStream != nil { buildOptions.OutputStream = options.OutputStream } else if ref.showBuildLogs { diff --git a/pkg/crt/podman/podmancrtclient/podmancrtclient.go b/pkg/crt/podman/podmancrtclient/podmancrtclient.go index 850a9a8d..97b34ef2 100644 --- a/pkg/crt/podman/podmancrtclient/podmancrtclient.go +++ b/pkg/crt/podman/podmancrtclient/podmancrtclient.go @@ -77,6 +77,10 @@ func (ref *Instance) BuildImage(options imagebuilder.DockerfileBuildOptions) err } } + if options.BuildContext == "" { + options.BuildContext = "." + } + var contextDir string if strings.HasPrefix(options.BuildContext, "http://") || strings.HasPrefix(options.BuildContext, "https://") { @@ -86,20 +90,16 @@ func (ref *Instance) BuildImage(options imagebuilder.DockerfileBuildOptions) err } else { if exists := fsutil.DirExists(options.BuildContext); exists { contextDir = options.BuildContext + //Dockerfile path is expected to be relative to build context + fullDockerfileName := filepath.Join(contextDir, options.Dockerfile) + if !fsutil.Exists(fullDockerfileName) || !fsutil.IsRegularFile(fullDockerfileName) { + return fmt.Errorf("invalid dockerfile reference (%s) - %s", options.Dockerfile, fullDockerfileName) + } } else { return imagebuilder.ErrInvalidContextDir } } - fullDockerfileName := options.Dockerfile - if !fsutil.Exists(fullDockerfileName) || !fsutil.IsRegularFile(fullDockerfileName) { - //a slightly hacky behavior using the build context directory if the dockerfile flag doesn't include a usable path - fullDockerfileName = filepath.Join(contextDir, fullDockerfileName) - if !fsutil.Exists(fullDockerfileName) || !fsutil.IsRegularFile(fullDockerfileName) { - return fmt.Errorf("invalid dockerfile reference - %s", fullDockerfileName) - } - } - buildOptions := images.BuildOptions{ BuildOptions: buildahDefine.BuildOptions{ Output: options.ImagePath, @@ -110,15 +110,18 @@ func (ref *Instance) BuildImage(options imagebuilder.DockerfileBuildOptions) err RemoveIntermediateCtrs: true, PullPolicy: buildahDefine.PullIfMissing, OutputFormat: buildahDefine.Dockerv2ImageManifest, //buildah.OCIv1ImageManifest - CommonBuildOpts: &buildahDefine.CommonBuildOptions{ - AddHost: strings.Split(options.ExtraHosts, ","), + CommonBuildOpts: &buildahDefine.CommonBuildOptions{ + //AddHost: strings.Split(options.ExtraHosts, ","), }, - - //ConfigureNetwork: tbd <- options.NetworkMode + ConfigureNetwork: buildahDefine.NetworkDefault, //tbd <- options.NetworkMode //CacheFrom: tbd <- options.CacheFrom //CacheTo: tbd <- options.CacheFrom }, - ContainerFiles: []string{fullDockerfileName}, + ContainerFiles: []string{options.Dockerfile}, + } + + if options.ExtraHosts != "" { + buildOptions.CommonBuildOpts.AddHost = strings.Split(options.ExtraHosts, ",") } if len(options.Platforms) > 0 { @@ -149,8 +152,11 @@ func (ref *Instance) BuildImage(options imagebuilder.DockerfileBuildOptions) err } } - for _, nv := range options.BuildArgs { - buildOptions.Args[nv.Name] = nv.Value + if len(options.BuildArgs) > 0 { + buildOptions.Args = map[string]string{} + for _, nv := range options.BuildArgs { + buildOptions.Args[nv.Name] = nv.Value + } } if options.OutputStream != nil { diff --git a/pkg/imagebuilder/standardbuilder/engine.go b/pkg/imagebuilder/standardbuilder/engine.go index 9cd2f804..efb572cb 100644 --- a/pkg/imagebuilder/standardbuilder/engine.go +++ b/pkg/imagebuilder/standardbuilder/engine.go @@ -1,16 +1,8 @@ package standardbuilder import ( - //"bytes" - //"fmt" - //"path/filepath" - //"strings" - - //docker "github.com/fsouza/go-dockerclient" - "github.com/mintoolkit/mint/pkg/crt" "github.com/mintoolkit/mint/pkg/imagebuilder" - //"github.com/mintoolkit/mint/pkg/util/fsutil" ) const ( @@ -52,89 +44,9 @@ func (ref *Engine) Name() string { } func (ref *Engine) BuildLog() string { - return ref.pclient.BuildOutputLog() //ref.buildLog.String() + return ref.pclient.BuildOutputLog() } func (ref *Engine) Build(options imagebuilder.DockerfileBuildOptions) error { return ref.pclient.BuildImage(options) - /* - if len(options.Labels) > 0 { - //Docker has a limit on how long the labels can be - labels := map[string]string{} - for k, v := range options.Labels { - lineLen := len(k) + len(v) + 7 - if lineLen > 65535 { - //TODO: improve JSON data splitting - valueLen := len(v) - parts := valueLen / 50000 - parts++ - offset := 0 - for i := 0; i < parts && offset < valueLen; i++ { - chunkSize := 50000 - if (offset + chunkSize) > valueLen { - chunkSize = valueLen - offset - } - value := v[offset:(offset + chunkSize)] - offset += chunkSize - key := fmt.Sprintf("%s.%d", k, i) - labels[key] = value - } - } else { - labels[k] = v - } - } - options.Labels = labels - } - - //not using options.CacheTo in this image builder... - buildOptions := docker.BuildImageOptions{ - Dockerfile: options.Dockerfile, - Target: options.Target, - Name: options.ImagePath, - - NetworkMode: options.NetworkMode, - ExtraHosts: options.ExtraHosts, - CacheFrom: options.CacheFrom, - Labels: options.Labels, - RmTmpContainer: true, - } - - if len(options.Platforms) > 0 { - buildOptions.Platform = strings.Join(options.Platforms, ",") - } - - for _, nv := range options.BuildArgs { - buildOptions.BuildArgs = append(buildOptions.BuildArgs, docker.BuildArg{Name: nv.Name, Value: nv.Value}) - } - - if strings.HasPrefix(options.BuildContext, "http://") || - strings.HasPrefix(options.BuildContext, "https://") { - buildOptions.Remote = options.BuildContext - } else { - if exists := fsutil.DirExists(options.BuildContext); exists { - buildOptions.ContextDir = options.BuildContext - } else { - return imagebuilder.ErrInvalidContextDir - } - } - - if !fsutil.Exists(buildOptions.Dockerfile) || !fsutil.IsRegularFile(buildOptions.Dockerfile) { - //a slightly hacky behavior using the build context directory if the dockerfile flag doesn't include a usable path - fullDockerfileName := filepath.Join(buildOptions.ContextDir, buildOptions.Dockerfile) - if !fsutil.Exists(fullDockerfileName) || !fsutil.IsRegularFile(fullDockerfileName) { - return fmt.Errorf("invalid dockerfile reference - %s", fullDockerfileName) - } - - buildOptions.Dockerfile = fullDockerfileName - } - - if options.OutputStream != nil { - buildOptions.OutputStream = options.OutputStream - } else if ref.showBuildLogs { - ref.buildLog.Reset() - buildOptions.OutputStream = &ref.buildLog - } - - return ref.pclient.BuildImage(buildOptions) - */ }