Merge pull request #2150 from crazy-max/builder-create

builder: move builder creation logic to builder package

builder/builder.go

@@ -2,7 +2,9 @@ package builder
 
 import (
 	"context"
+	"encoding/csv"
 	"encoding/json"
+	"net/url"
 	"os"
 	"sort"
 	"strings"
@@ -10,12 +12,18 @@ import (
 	"time"
 
 	"github.com/docker/buildx/driver"
+	k8sutil "github.com/docker/buildx/driver/kubernetes/util"
+	remoteutil "github.com/docker/buildx/driver/remote/util"
+	"github.com/docker/buildx/localstate"
 	"github.com/docker/buildx/store"
 	"github.com/docker/buildx/store/storeutil"
+	"github.com/docker/buildx/util/confutil"
 	"github.com/docker/buildx/util/dockerutil"
 	"github.com/docker/buildx/util/imagetools"
 	"github.com/docker/buildx/util/progress"
 	"github.com/docker/cli/cli/command"
+	dopts "github.com/docker/cli/opts"
+	"github.com/google/shlex"
 	"github.com/moby/buildkit/util/progress/progressui"
 	"github.com/pkg/errors"
 	"golang.org/x/sync/errgroup"
@@ -322,3 +330,315 @@ func GetBuilders(dockerCli command.Cli, txn *store.Txn) ([]*Builder, error) {
 
 	return builders, nil
 }
+
+type CreateOpts struct {
+	Name       string
+	Driver     string
+	NodeName   string
+	Platforms  []string
+	Flags      string
+	ConfigFile string
+	DriverOpts []string
+	Use        bool
+	Endpoint   string
+	Append     bool
+}
+
+func Create(ctx context.Context, txn *store.Txn, dockerCli command.Cli, opts CreateOpts) (*Builder, error) {
+	var err error
+
+	if opts.Name == "default" {
+		return nil, errors.Errorf("default is a reserved name and cannot be used to identify builder instance")
+	} else if opts.Append && opts.Name == "" {
+		return nil, errors.Errorf("append requires a builder name")
+	}
+
+	name := opts.Name
+	if name == "" {
+		name, err = store.GenerateName(txn)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if !opts.Append {
+		contexts, err := dockerCli.ContextStore().List()
+		if err != nil {
+			return nil, err
+		}
+		for _, c := range contexts {
+			if c.Name == name {
+				return nil, errors.Errorf("instance name %q already exists as context builder", name)
+			}
+		}
+	}
+
+	ng, err := txn.NodeGroupByName(name)
+	if err != nil {
+		if os.IsNotExist(errors.Cause(err)) {
+			if opts.Append && opts.Name != "" {
+				return nil, errors.Errorf("failed to find instance %q for append", opts.Name)
+			}
+		} else {
+			return nil, err
+		}
+	}
+
+	buildkitHost := os.Getenv("BUILDKIT_HOST")
+
+	driverName := opts.Driver
+	if driverName == "" {
+		if ng != nil {
+			driverName = ng.Driver
+		} else if opts.Endpoint == "" && buildkitHost != "" {
+			driverName = "remote"
+		} else {
+			f, err := driver.GetDefaultFactory(ctx, opts.Endpoint, dockerCli.Client(), true, nil)
+			if err != nil {
+				return nil, err
+			}
+			if f == nil {
+				return nil, errors.Errorf("no valid drivers found")
+			}
+			driverName = f.Name()
+		}
+	}
+
+	if ng != nil {
+		if opts.NodeName == "" && !opts.Append {
+			return nil, errors.Errorf("existing instance for %q but no append mode, specify the node name to make changes for existing instances", name)
+		}
+		if driverName != ng.Driver {
+			return nil, errors.Errorf("existing instance for %q but has mismatched driver %q", name, ng.Driver)
+		}
+	}
+
+	if _, err := driver.GetFactory(driverName, true); err != nil {
+		return nil, err
+	}
+
+	ngOriginal := ng
+	if ngOriginal != nil {
+		ngOriginal = ngOriginal.Copy()
+	}
+
+	if ng == nil {
+		ng = &store.NodeGroup{
+			Name:   name,
+			Driver: driverName,
+		}
+	}
+
+	var flags []string
+	if opts.Flags != "" {
+		flags, err = shlex.Split(opts.Flags)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to parse buildkit flags")
+		}
+	}
+
+	var ep string
+	var setEp bool
+	switch {
+	case driverName == "kubernetes":
+		if opts.Endpoint != "" {
+			return nil, errors.Errorf("kubernetes driver does not support endpoint args %q", opts.Endpoint)
+		}
+		// generate node name if not provided to avoid duplicated endpoint
+		// error: https://github.com/docker/setup-buildx-action/issues/215
+		nodeName := opts.NodeName
+		if nodeName == "" {
+			nodeName, err = k8sutil.GenerateNodeName(name, txn)
+			if err != nil {
+				return nil, err
+			}
+		}
+		// naming endpoint to make append works
+		ep = (&url.URL{
+			Scheme: driverName,
+			Path:   "/" + name,
+			RawQuery: (&url.Values{
+				"deployment": {nodeName},
+				"kubeconfig": {os.Getenv("KUBECONFIG")},
+			}).Encode(),
+		}).String()
+		setEp = false
+	case driverName == "remote":
+		if opts.Endpoint != "" {
+			ep = opts.Endpoint
+		} else if buildkitHost != "" {
+			ep = buildkitHost
+		} else {
+			return nil, errors.Errorf("no remote endpoint provided")
+		}
+		ep, err = validateBuildkitEndpoint(ep)
+		if err != nil {
+			return nil, err
+		}
+		setEp = true
+	case opts.Endpoint != "":
+		ep, err = validateEndpoint(dockerCli, opts.Endpoint)
+		if err != nil {
+			return nil, err
+		}
+		setEp = true
+	default:
+		if dockerCli.CurrentContext() == "default" && dockerCli.DockerEndpoint().TLSData != nil {
+			return nil, errors.Errorf("could not create a builder instance with TLS data loaded from environment. Please use `docker context create <context-name>` to create a context for current environment and then create a builder instance with context set to <context-name>")
+		}
+		ep, err = dockerutil.GetCurrentEndpoint(dockerCli)
+		if err != nil {
+			return nil, err
+		}
+		setEp = false
+	}
+
+	m, err := csvToMap(opts.DriverOpts)
+	if err != nil {
+		return nil, err
+	}
+
+	configFile := opts.ConfigFile
+	if configFile == "" {
+		// if buildkit config is not provided, check if the default one is
+		// available and use it
+		if f, ok := confutil.DefaultConfigFile(dockerCli); ok {
+			configFile = f
+		}
+	}
+
+	if err := ng.Update(opts.NodeName, ep, opts.Platforms, setEp, opts.Append, flags, configFile, m); err != nil {
+		return nil, err
+	}
+
+	if err := txn.Save(ng); err != nil {
+		return nil, err
+	}
+
+	b, err := New(dockerCli,
+		WithName(ng.Name),
+		WithStore(txn),
+		WithSkippedValidation(),
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	timeoutCtx, cancel := context.WithTimeout(ctx, 20*time.Second)
+	defer cancel()
+
+	nodes, err := b.LoadNodes(timeoutCtx, WithData())
+	if err != nil {
+		return nil, err
+	}
+
+	for _, node := range nodes {
+		if err := node.Err; err != nil {
+			err := errors.Errorf("failed to initialize builder %s (%s): %s", ng.Name, node.Name, err)
+			var err2 error
+			if ngOriginal == nil {
+				err2 = txn.Remove(ng.Name)
+			} else {
+				err2 = txn.Save(ngOriginal)
+			}
+			if err2 != nil {
+				return nil, errors.Errorf("could not rollback to previous state: %s", err2)
+			}
+			return nil, err
+		}
+	}
+
+	if opts.Use && ep != "" {
+		current, err := dockerutil.GetCurrentEndpoint(dockerCli)
+		if err != nil {
+			return nil, err
+		}
+		if err := txn.SetCurrent(current, ng.Name, false, false); err != nil {
+			return nil, err
+		}
+	}
+
+	return b, nil
+}
+
+type LeaveOpts struct {
+	Name     string
+	NodeName string
+}
+
+func Leave(ctx context.Context, txn *store.Txn, dockerCli command.Cli, opts LeaveOpts) error {
+	if opts.Name == "" {
+		return errors.Errorf("leave requires instance name")
+	}
+	if opts.NodeName == "" {
+		return errors.Errorf("leave requires node name")
+	}
+
+	ng, err := txn.NodeGroupByName(opts.Name)
+	if err != nil {
+		if os.IsNotExist(errors.Cause(err)) {
+			return errors.Errorf("failed to find instance %q for leave", opts.Name)
+		}
+		return err
+	}
+
+	if err := ng.Leave(opts.NodeName); err != nil {
+		return err
+	}
+
+	ls, err := localstate.New(confutil.ConfigDir(dockerCli))
+	if err != nil {
+		return err
+	}
+	if err := ls.RemoveBuilderNode(ng.Name, opts.NodeName); err != nil {
+		return err
+	}
+
+	return txn.Save(ng)
+}
+
+func csvToMap(in []string) (map[string]string, error) {
+	if len(in) == 0 {
+		return nil, nil
+	}
+	m := make(map[string]string, len(in))
+	for _, s := range in {
+		csvReader := csv.NewReader(strings.NewReader(s))
+		fields, err := csvReader.Read()
+		if err != nil {
+			return nil, err
+		}
+		for _, v := range fields {
+			p := strings.SplitN(v, "=", 2)
+			if len(p) != 2 {
+				return nil, errors.Errorf("invalid value %q, expecting k=v", v)
+			}
+			m[p[0]] = p[1]
+		}
+	}
+	return m, nil
+}
+
+// validateEndpoint validates that endpoint is either a context or a docker host
+func validateEndpoint(dockerCli command.Cli, ep string) (string, error) {
+	dem, err := dockerutil.GetDockerEndpoint(dockerCli, ep)
+	if err == nil && dem != nil {
+		if ep == "default" {
+			return dem.Host, nil
+		}
+		return ep, nil
+	}
+	h, err := dopts.ParseHost(true, ep)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to parse endpoint %s", ep)
+	}
+	return h, nil
+}
+
+// validateBuildkitEndpoint validates that endpoint is a valid buildkit host
+func validateBuildkitEndpoint(ep string) (string, error) {
+	if err := remoteutil.IsValidEndpoint(ep); err != nil {
+		return "", err
+	}
+	return ep, nil
+}

builder/builder_test.go

@@ -1,4 +1,4 @@
-package commands
+package builder
 
 import (
 	"testing"

commands/create.go

@@ -2,32 +2,16 @@ package commands
 
 import (
 	"bytes"
-	"context"
-	"encoding/csv"
 	"fmt"
-	"net/url"
-	"os"
-	"strings"
-	"time"
 
 	"github.com/docker/buildx/builder"
 	"github.com/docker/buildx/driver"
-	k8sutil "github.com/docker/buildx/driver/kubernetes/util"
-	remoteutil "github.com/docker/buildx/driver/remote/util"
-	"github.com/docker/buildx/localstate"
-	"github.com/docker/buildx/store"
 	"github.com/docker/buildx/store/storeutil"
 	"github.com/docker/buildx/util/cobrautil"
 	"github.com/docker/buildx/util/cobrautil/completion"
-	"github.com/docker/buildx/util/confutil"
-	"github.com/docker/buildx/util/dockerutil"
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
-	dopts "github.com/docker/cli/opts"
-	"github.com/google/shlex"
 	"github.com/moby/buildkit/util/appcontext"
-	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
 
@@ -49,25 +33,6 @@ type createOptions struct {
 func runCreate(dockerCli command.Cli, in createOptions, args []string) error {
 	ctx := appcontext.Context()
 
-	if in.name == "default" {
-		return errors.Errorf("default is a reserved name and cannot be used to identify builder instance")
-	}
-
-	if in.actionLeave {
-		if in.name == "" {
-			return errors.Errorf("leave requires instance name")
-		}
-		if in.nodeName == "" {
-			return errors.Errorf("leave requires node name but --node not set")
-		}
-	}
-
-	if in.actionAppend {
-		if in.name == "" {
-			logrus.Warnf("append used without name, creating a new instance instead")
-		}
-	}
-
 	txn, release, err := storeutil.GetStore(dockerCli)
 	if err != nil {
 		return err
@@ -75,232 +40,34 @@ func runCreate(dockerCli command.Cli, in createOptions, args []string) error {
 	// Ensure the file lock gets released no matter what happens.
 	defer release()
 
-	name := in.name
+	if in.actionLeave {
-	if name == "" {
+		return builder.Leave(ctx, txn, dockerCli, builder.LeaveOpts{
-		name, err = store.GenerateName(txn)
+			Name:     in.name,
-		if err != nil {
+			NodeName: in.nodeName,
-			return err
+		})
-		}
-	}
-
-	if !in.actionLeave && !in.actionAppend {
-		contexts, err := dockerCli.ContextStore().List()
-		if err != nil {
-			return err
-		}
-		for _, c := range contexts {
-			if c.Name == name {
-				logrus.Warnf("instance name %q already exists as context builder", name)
-				break
-			}
-		}
-	}
-
-	ng, err := txn.NodeGroupByName(name)
-	if err != nil {
-		if os.IsNotExist(errors.Cause(err)) {
-			if in.actionAppend && in.name != "" {
-				logrus.Warnf("failed to find %q for append, creating a new instance instead", in.name)
-			}
-			if in.actionLeave {
-				return errors.Errorf("failed to find instance %q for leave", in.name)
-			}
-		} else {
-			return err
-		}
-	}
-
-	buildkitHost := os.Getenv("BUILDKIT_HOST")
-
-	driverName := in.driver
-	if driverName == "" {
-		if ng != nil {
-			driverName = ng.Driver
-		} else if len(args) == 0 && buildkitHost != "" {
-			driverName = "remote"
-		} else {
-			var arg string
-			if len(args) > 0 {
-				arg = args[0]
-			}
-			f, err := driver.GetDefaultFactory(ctx, arg, dockerCli.Client(), true, nil)
-			if err != nil {
-				return err
-			}
-			if f == nil {
-				return errors.Errorf("no valid drivers found")
-			}
-			driverName = f.Name()
-		}
-	}
-
-	if ng != nil {
-		if in.nodeName == "" && !in.actionAppend {
-			return errors.Errorf("existing instance for %q but no append mode, specify --node to make changes for existing instances", name)
-		}
-		if driverName != ng.Driver {
-			return errors.Errorf("existing instance for %q but has mismatched driver %q", name, ng.Driver)
-		}
-	}
-
-	if _, err := driver.GetFactory(driverName, true); err != nil {
-		return err
-	}
-
-	ngOriginal := ng
-	if ngOriginal != nil {
-		ngOriginal = ngOriginal.Copy()
-	}
-
-	if ng == nil {
-		ng = &store.NodeGroup{
-			Name:   name,
-			Driver: driverName,
-		}
-	}
-
-	var flags []string
-	if in.flags != "" {
-		flags, err = shlex.Split(in.flags)
-		if err != nil {
-			return errors.Wrap(err, "failed to parse buildkit flags")
-		}
 	}
 
 	var ep string
-	var setEp bool
+	if len(args) > 0 {
-	if in.actionLeave {
+		ep = args[0]
-		if err := ng.Leave(in.nodeName); err != nil {
-			return err
-		}
-		ls, err := localstate.New(confutil.ConfigDir(dockerCli))
-		if err != nil {
-			return err
-		}
-		if err := ls.RemoveBuilderNode(ng.Name, in.nodeName); err != nil {
-			return err
-		}
-	} else {
-		switch {
-		case driverName == "kubernetes":
-			if len(args) > 0 {
-				logrus.Warnf("kubernetes driver does not support endpoint args %q", args[0])
-			}
-			// generate node name if not provided to avoid duplicated endpoint
-			// error: https://github.com/docker/setup-buildx-action/issues/215
-			nodeName := in.nodeName
-			if nodeName == "" {
-				nodeName, err = k8sutil.GenerateNodeName(name, txn)
-				if err != nil {
-					return err
-				}
-			}
-			// naming endpoint to make --append works
-			ep = (&url.URL{
-				Scheme: driverName,
-				Path:   "/" + name,
-				RawQuery: (&url.Values{
-					"deployment": {nodeName},
-					"kubeconfig": {os.Getenv("KUBECONFIG")},
-				}).Encode(),
-			}).String()
-			setEp = false
-		case driverName == "remote":
-			if len(args) > 0 {
-				ep = args[0]
-			} else if buildkitHost != "" {
-				ep = buildkitHost
-			} else {
-				return errors.Errorf("no remote endpoint provided")
-			}
-			ep, err = validateBuildkitEndpoint(ep)
-			if err != nil {
-				return err
-			}
-			setEp = true
-		case len(args) > 0:
-			ep, err = validateEndpoint(dockerCli, args[0])
-			if err != nil {
-				return err
-			}
-			setEp = true
-		default:
-			if dockerCli.CurrentContext() == "default" && dockerCli.DockerEndpoint().TLSData != nil {
-				return errors.Errorf("could not create a builder instance with TLS data loaded from environment. Please use `docker context create <context-name>` to create a context for current environment and then create a builder instance with `docker buildx create <context-name>`")
-			}
-			ep, err = dockerutil.GetCurrentEndpoint(dockerCli)
-			if err != nil {
-				return err
-			}
-			setEp = false
-		}
-
-		m, err := csvToMap(in.driverOpts)
-		if err != nil {
-			return err
-		}
-
-		if in.configFile == "" {
-			// if buildkit config is not provided, check if the default one is
-			// available and use it
-			if f, ok := confutil.DefaultConfigFile(dockerCli); ok {
-				logrus.Warnf("Using default BuildKit config in %s", f)
-				in.configFile = f
-			}
-		}
-
-		if err := ng.Update(in.nodeName, ep, in.platform, setEp, in.actionAppend, flags, in.configFile, m); err != nil {
-			return err
-		}
 	}
 
-	if err := txn.Save(ng); err != nil {
+	b, err := builder.Create(ctx, txn, dockerCli, builder.CreateOpts{
-		return err
+		Name:       in.name,
-	}
+		Driver:     in.driver,
-
+		NodeName:   in.nodeName,
-	b, err := builder.New(dockerCli,
+		Platforms:  in.platform,
-		builder.WithName(ng.Name),
+		Flags:      in.flags,
-		builder.WithStore(txn),
+		ConfigFile: in.configFile,
-		builder.WithSkippedValidation(),
+		DriverOpts: in.driverOpts,
-	)
+		Use:        in.use,
+		Endpoint:   ep,
+		Append:     in.actionAppend,
+	})
 	if err != nil {
 		return err
 	}
 
-	timeoutCtx, cancel := context.WithTimeout(ctx, 20*time.Second)
-	defer cancel()
-
-	nodes, err := b.LoadNodes(timeoutCtx, builder.WithData())
-	if err != nil {
-		return err
-	}
-
-	for _, node := range nodes {
-		if err := node.Err; err != nil {
-			err := errors.Errorf("failed to initialize builder %s (%s): %s", ng.Name, node.Name, err)
-			var err2 error
-			if ngOriginal == nil {
-				err2 = txn.Remove(ng.Name)
-			} else {
-				err2 = txn.Save(ngOriginal)
-			}
-			if err2 != nil {
-				logrus.Warnf("Could not rollback to previous state: %s", err2)
-			}
-			return err
-		}
-	}
-
-	if in.use && ep != "" {
-		current, err := dockerutil.GetCurrentEndpoint(dockerCli)
-		if err != nil {
-			return err
-		}
-		if err := txn.SetCurrent(current, ng.Name, false, false); err != nil {
-			return err
-		}
-	}
-
 	// The store is no longer used from this point.
 	// Release it so we aren't holding the file lock during the boot.
 	release()
@@ -311,7 +78,7 @@ func runCreate(dockerCli command.Cli, in createOptions, args []string) error {
 		}
 	}
 
-	fmt.Printf("%s\n", ng.Name)
+	fmt.Printf("%s\n", b.Name)
 	return nil
 }
 
@@ -356,49 +123,3 @@ func createCmd(dockerCli command.Cli) *cobra.Command {
 
 	return cmd
 }
-
-func csvToMap(in []string) (map[string]string, error) {
-	if len(in) == 0 {
-		return nil, nil
-	}
-	m := make(map[string]string, len(in))
-	for _, s := range in {
-		csvReader := csv.NewReader(strings.NewReader(s))
-		fields, err := csvReader.Read()
-		if err != nil {
-			return nil, err
-		}
-		for _, v := range fields {
-			p := strings.SplitN(v, "=", 2)
-			if len(p) != 2 {
-				return nil, errors.Errorf("invalid value %q, expecting k=v", v)
-			}
-			m[p[0]] = p[1]
-		}
-	}
-	return m, nil
-}
-
-// validateEndpoint validates that endpoint is either a context or a docker host
-func validateEndpoint(dockerCli command.Cli, ep string) (string, error) {
-	dem, err := dockerutil.GetDockerEndpoint(dockerCli, ep)
-	if err == nil && dem != nil {
-		if ep == "default" {
-			return dem.Host, nil
-		}
-		return ep, nil
-	}
-	h, err := dopts.ParseHost(true, ep)
-	if err != nil {
-		return "", errors.Wrapf(err, "failed to parse endpoint %s", ep)
-	}
-	return h, nil
-}
-
-// validateBuildkitEndpoint validates that endpoint is a valid buildkit host
-func validateBuildkitEndpoint(ep string) (string, error) {
-	if err := remoteutil.IsValidEndpoint(ep); err != nil {
-		return "", err
-	}
-	return ep, nil
-}