Merge pull request #2829 from dotcloud/refactor_opts

Refactor opts

buildfile.go

@@ -246,7 +246,7 @@ func (b *buildFile) CmdVolume(args string) error {
 		volume = []string{args}
 	}
 	if b.config.Volumes == nil {
-		b.config.Volumes = PathOpts{}
+		b.config.Volumes = map[string]struct{}{}
 	}
 	for _, v := range volume {
 		b.config.Volumes[v] = struct{}{}

commands.go

@@ -23,7 +23,6 @@ import (
 	"os"
 	"os/signal"
 	"path"
-	"path/filepath"
 	"reflect"
 	"regexp"
 	"runtime"
@@ -1622,58 +1621,6 @@ func (cli *DockerCli) CmdSearch(args ...string) error {
 // Ports type - Used to parse multiple -p flags
 type ports []int
 
-// AttachOpts stores arguments to 'docker run -a', eg. which streams to attach to
-type AttachOpts map[string]bool
-
-func (opts AttachOpts) String() string { return fmt.Sprintf("%v", map[string]bool(opts)) }
-func (opts AttachOpts) Set(val string) error {
-	if val != "stdin" && val != "stdout" && val != "stderr" {
-		return fmt.Errorf("Unsupported stream name: %s", val)
-	}
-	opts[val] = true
-	return nil
-}
-
-// LinkOpts stores arguments to `docker run -link`
-type LinkOpts []string
-
-func (link *LinkOpts) String() string { return fmt.Sprintf("%v", []string(*link)) }
-func (link *LinkOpts) Set(val string) error {
-	if _, err := parseLink(val); err != nil {
-		return err
-	}
-	*link = append(*link, val)
-	return nil
-}
-
-// PathOpts stores a unique set of absolute paths
-type PathOpts map[string]struct{}
-
-func (opts PathOpts) String() string { return fmt.Sprintf("%v", map[string]struct{}(opts)) }
-func (opts PathOpts) Set(val string) error {
-	var containerPath string
-
-	if strings.Count(val, ":") > 2 {
-		return fmt.Errorf("bad format for volumes: %s", val)
-	}
-
-	if splited := strings.SplitN(val, ":", 2); len(splited) == 1 {
-		containerPath = splited[0]
-		val = filepath.Clean(splited[0])
-	} else {
-		containerPath = splited[1]
-		val = fmt.Sprintf("%s:%s", splited[0], filepath.Clean(splited[1]))
-	}
-
-	if !filepath.IsAbs(containerPath) {
-		utils.Debugf("%s is not an absolute path", containerPath)
-		return fmt.Errorf("%s is not an absolute path", containerPath)
-	}
-	opts[val] = struct{}{}
-
-	return nil
-}
-
 func (cli *DockerCli) CmdTag(args ...string) error {
 	cmd := cli.Subcmd("tag", "[OPTIONS] IMAGE REPOSITORY[:TAG]", "Tag an image into a repository")
 	force := cmd.Bool("f", false, "Force")
@@ -1719,16 +1666,16 @@ func ParseRun(args []string, capabilities *Capabilities) (*Config, *HostConfig,
 func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Config, *HostConfig, *flag.FlagSet, error) {
 	var (
 		// FIXME: use utils.ListOpts for attach and volumes?
-		flAttach  = AttachOpts{}
-		flVolumes = PathOpts{}
-		flLinks   = LinkOpts{}
+		flAttach  = NewListOpts(ValidateAttach)
+		flVolumes = NewListOpts(ValidatePath)
+		flLinks   = NewListOpts(ValidateLink)
+		flEnv     = NewListOpts(ValidateEnv)
 
-		flPublish     utils.ListOpts
-		flExpose      utils.ListOpts
-		flEnv         utils.ListOpts
-		flDns         utils.ListOpts
-		flVolumesFrom utils.ListOpts
-		flLxcOpts     utils.ListOpts
+		flPublish     ListOpts
+		flExpose      ListOpts
+		flDns         ListOpts
+		flVolumesFrom ListOpts
+		flLxcOpts     ListOpts
 
 		flAutoRemove      = cmd.Bool("rm", false, "Automatically remove the container when it exits (incompatible with -d)")
 		flDetach          = cmd.Bool("d", false, "Detached mode: Run container in the background, print new container id")
@@ -1750,13 +1697,13 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 		_ = cmd.String("name", "", "Assign a name to the container")
 	)
 
-	cmd.Var(flAttach, "a", "Attach to stdin, stdout or stderr.")
-	cmd.Var(flVolumes, "v", "Bind mount a volume (e.g. from the host: -v /host:/container, from docker: -v /container)")
+	cmd.Var(&flAttach, "a", "Attach to stdin, stdout or stderr.")
+	cmd.Var(&flVolumes, "v", "Bind mount a volume (e.g. from the host: -v /host:/container, from docker: -v /container)")
 	cmd.Var(&flLinks, "link", "Add link to another container (name:alias)")
+	cmd.Var(&flEnv, "e", "Set environment variables")
 
 	cmd.Var(&flPublish, "p", fmt.Sprintf("Publish a container's port to the host (format: %s) (use 'docker port' to see the actual mapping)", PortSpecTemplateFormat))
 	cmd.Var(&flExpose, "expose", "Expose a port from the container without publishing it to your host")
-	cmd.Var(&flEnv, "e", "Set environment variables")
 	cmd.Var(&flDns, "dns", "Set custom dns servers")
 	cmd.Var(&flVolumesFrom, "volumes-from", "Mount volumes from the specified container(s)")
 	cmd.Var(&flLxcOpts, "lxc-conf", "Add custom lxc options -lxc-conf=\"lxc.cgroup.cpuset.cpus = 0,1\"")
@@ -1771,7 +1718,7 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 	}
 
 	// Validate input params
-	if *flDetach && len(flAttach) > 0 {
+	if *flDetach && flAttach.Len() > 0 {
 		return nil, nil, cmd, ErrConflictAttachDetach
 	}
 	if *flWorkingDir != "" && !path.IsAbs(*flWorkingDir) {
@@ -1782,7 +1729,7 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 	}
 
 	// If neither -d or -a are set, attach to everything by default
-	if len(flAttach) == 0 && !*flDetach {
+	if flAttach.Len() == 0 && !*flDetach {
 		if !*flDetach {
 			flAttach.Set("stdout")
 			flAttach.Set("stderr")
@@ -1792,17 +1739,6 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 		}
 	}
 
-	var envs []string
-	for _, env := range flEnv {
-		arr := strings.Split(env, "=")
-		if len(arr) > 1 {
-			envs = append(envs, env)
-		} else {
-			v := os.Getenv(env)
-			envs = append(envs, env+"="+v)
-		}
-	}
-
 	var flMemory int64
 	if *flMemoryString != "" {
 		parsedMemory, err := utils.RAMInBytes(*flMemoryString)
@@ -1814,16 +1750,15 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 
 	var binds []string
 	// add any bind targets to the list of container volumes
-	for bind := range flVolumes {
-		arr := strings.Split(bind, ":")
-		if len(arr) > 1 {
+	for bind := range flVolumes.GetMap() {
+		if arr := strings.Split(bind, ":"); len(arr) > 1 {
 			if arr[0] == "/" {
 				return nil, nil, cmd, fmt.Errorf("Invalid bind mount: source can't be '/'")
 			}
 			dstDir := arr[1]
-			flVolumes[dstDir] = struct{}{}
+			flVolumes.Set(dstDir)
 			binds = append(binds, bind)
-			delete(flVolumes, bind)
+			flVolumes.Delete(bind)
 		}
 	}
 
@@ -1858,13 +1793,13 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 		domainname = parts[1]
 	}
 
-	ports, portBindings, err := parsePortSpecs(flPublish)
+	ports, portBindings, err := parsePortSpecs(flPublish.GetAll())
 	if err != nil {
 		return nil, nil, cmd, err
 	}
 
 	// Merge in exposed ports to the map of published ports
-	for _, e := range flExpose {
+	for _, e := range flExpose.GetAll() {
 		if strings.Contains(e, ":") {
 			return nil, nil, cmd, fmt.Errorf("Invalid port format for -expose: %s", e)
 		}
@@ -1885,15 +1820,15 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 		OpenStdin:       *flStdin,
 		Memory:          flMemory,
 		CpuShares:       *flCpuShares,
-		AttachStdin:     flAttach["stdin"],
-		AttachStdout:    flAttach["stdout"],
-		AttachStderr:    flAttach["stderr"],
-		Env:             envs,
+		AttachStdin:     flAttach.Get("stdin"),
+		AttachStdout:    flAttach.Get("stdout"),
+		AttachStderr:    flAttach.Get("stderr"),
+		Env:             flEnv.GetAll(),
 		Cmd:             runCmd,
-		Dns:             flDns,
+		Dns:             flDns.GetAll(),
 		Image:           image,
-		Volumes:         flVolumes,
-		VolumesFrom:     strings.Join(flVolumesFrom, ","),
+		Volumes:         flVolumes.GetMap(),
+		VolumesFrom:     strings.Join(flVolumesFrom.GetAll(), ","),
 		Entrypoint:      entrypoint,
 		WorkingDir:      *flWorkingDir,
 	}
@@ -1904,7 +1839,7 @@ func parseRun(cmd *flag.FlagSet, args []string, capabilities *Capabilities) (*Co
 		LxcConf:         lxcConf,
 		Privileged:      *flPrivileged,
 		PortBindings:    portBindings,
-		Links:           flLinks,
+		Links:           flLinks.GetAll(),
 		PublishAllPorts: *flPublishAll,
 	}
 

docker/docker.go

@@ -23,22 +23,24 @@ func main() {
 		sysinit.SysInit()
 		return
 	}
-	// FIXME: Switch d and D ? (to be more sshd like)
-	flVersion := flag.Bool("v", false, "Print version information and quit")
-	flDaemon := flag.Bool("d", false, "Enable daemon mode")
-	flDebug := flag.Bool("D", false, "Enable debug mode")
-	flAutoRestart := flag.Bool("r", true, "Restart previously running containers")
-	bridgeName := flag.String("b", "", "Attach containers to a pre-existing network bridge; use 'none' to disable container networking")
-	pidfile := flag.String("p", "/var/run/docker.pid", "Path to use for daemon PID file")
-	flRoot := flag.String("g", "/var/lib/docker", "Path to use as the root of the docker runtime")
-	flEnableCors := flag.Bool("api-enable-cors", false, "Enable CORS headers in the remote API")
-	flDns := flag.String("dns", "", "Force docker to use specific DNS servers")
-	flHosts := utils.ListOpts{fmt.Sprintf("unix://%s", docker.DEFAULTUNIXSOCKET)}
+
+	var (
+		flVersion            = flag.Bool("v", false, "Print version information and quit")
+		flDaemon             = flag.Bool("d", false, "Enable daemon mode")
+		flDebug              = flag.Bool("D", false, "Enable debug mode")
+		flAutoRestart        = flag.Bool("r", true, "Restart previously running containers")
+		bridgeName           = flag.String("b", "", "Attach containers to a pre-existing network bridge; use 'none' to disable container networking")
+		pidfile              = flag.String("p", "/var/run/docker.pid", "Path to use for daemon PID file")
+		flRoot               = flag.String("g", "/var/lib/docker", "Path to use as the root of the docker runtime")
+		flEnableCors         = flag.Bool("api-enable-cors", false, "Enable CORS headers in the remote API")
+		flDns                = flag.String("dns", "", "Force docker to use specific DNS servers")
+		flEnableIptables     = flag.Bool("iptables", true, "Disable docker's addition of iptables rules")
+		flDefaultIp          = flag.String("ip", "0.0.0.0", "Default IP address to use when binding container ports")
+		flInterContainerComm = flag.Bool("icc", true, "Enable inter-container communication")
+		flGraphDriver        = flag.String("s", "", "Force the docker runtime to use a specific storage driver")
+		flHosts              = docker.NewListOpts(docker.ValidateHost)
+	)
 	flag.Var(&flHosts, "H", "Multiple tcp://host:port or unix://path/to/socket to bind in daemon mode, single connection otherwise")
-	flEnableIptables := flag.Bool("iptables", true, "Disable docker's addition of iptables rules")
-	flDefaultIp := flag.String("ip", "0.0.0.0", "Default IP address to use when binding container ports")
-	flInterContainerComm := flag.Bool("icc", true, "Enable inter-container communication")
-	flGraphDriver := flag.String("s", "", "Force the docker runtime to use a specific storage driver")
 
 	flag.Parse()
 
@@ -46,16 +48,9 @@ func main() {
 		showVersion()
 		return
 	}
-	if len(flHosts) > 1 {
-		flHosts = flHosts[1:] //trick to display a nice default value in the usage
-	}
-	for i, flHost := range flHosts {
-		host, err := utils.ParseHost(docker.DEFAULTHTTPHOST, docker.DEFAULTHTTPPORT, flHost)
-		if err == nil {
-			flHosts[i] = host
-		} else {
-			log.Fatal(err)
-		}
+	if flHosts.Len() == 0 {
+		// If we do not have a host, default to unix socket
+		flHosts.Set(fmt.Sprintf("unix://%s", docker.DEFAULTUNIXSOCKET))
 	}
 
 	if *flDebug {
@@ -88,16 +83,16 @@ func main() {
 			log.Fatal(err)
 		}
 		// Serve api
-		job = eng.Job("serveapi", flHosts...)
+		job = eng.Job("serveapi", flHosts.GetAll()...)
 		job.SetenvBool("Logging", true)
 		if err := job.Run(); err != nil {
 			log.Fatal(err)
 		}
 	} else {
-		if len(flHosts) > 1 {
+		if flHosts.Len() > 1 {
 			log.Fatal("Please specify only one -H")
 		}
-		protoAddrParts := strings.SplitN(flHosts[0], "://", 2)
+		protoAddrParts := strings.SplitN(flHosts.GetAll()[0], "://", 2)
 		if err := docker.ParseCommands(protoAddrParts[0], protoAddrParts[1], flag.Args()...); err != nil {
 			if sterr, ok := err.(*utils.StatusError); ok {
 				if sterr.Status != "" {

opts.go

@@ -0,0 +1,136 @@
+package docker
+
+import (
+	"fmt"
+	"github.com/dotcloud/docker/utils"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+// ListOpts type
+type ListOpts struct {
+	values    []string
+	validator ValidatorFctType
+}
+
+func NewListOpts(validator ValidatorFctType) ListOpts {
+	return ListOpts{
+		validator: validator,
+	}
+}
+
+func (opts *ListOpts) String() string {
+	return fmt.Sprintf("%v", []string(opts.values))
+}
+
+// Set validates if needed the input value and add it to the
+// internal slice.
+func (opts *ListOpts) Set(value string) error {
+	if opts.validator != nil {
+		v, err := opts.validator(value)
+		if err != nil {
+			return err
+		}
+		value = v
+	}
+	opts.values = append(opts.values, value)
+	return nil
+}
+
+// Delete remove the given element from the slice.
+func (opts *ListOpts) Delete(key string) {
+	for i, k := range opts.values {
+		if k == key {
+			opts.values = append(opts.values[:i], opts.values[i+1:]...)
+			return
+		}
+	}
+}
+
+// GetMap returns the content of values in a map in order to avoid
+// duplicates.
+// FIXME: can we remove this?
+func (opts *ListOpts) GetMap() map[string]struct{} {
+	ret := make(map[string]struct{})
+	for _, k := range opts.values {
+		ret[k] = struct{}{}
+	}
+	return ret
+}
+
+// GetAll returns the values' slice.
+// FIXME: Can we remove this?
+func (opts *ListOpts) GetAll() []string {
+	return opts.values
+}
+
+// Get checks the existence of the given key.
+func (opts *ListOpts) Get(key string) bool {
+	for _, k := range opts.values {
+		if k == key {
+			return true
+		}
+	}
+	return false
+}
+
+// Len returns the amount of element in the slice.
+func (opts *ListOpts) Len() int {
+	return len(opts.values)
+}
+
+// Validators
+type ValidatorFctType func(val string) (string, error)
+
+func ValidateAttach(val string) (string, error) {
+	if val != "stdin" && val != "stdout" && val != "stderr" {
+		return val, fmt.Errorf("Unsupported stream name: %s", val)
+	}
+	return val, nil
+}
+
+func ValidateLink(val string) (string, error) {
+	if _, err := parseLink(val); err != nil {
+		return val, err
+	}
+	return val, nil
+}
+
+func ValidatePath(val string) (string, error) {
+	var containerPath string
+
+	if strings.Count(val, ":") > 2 {
+		return val, fmt.Errorf("bad format for volumes: %s", val)
+	}
+
+	splited := strings.SplitN(val, ":", 2)
+	if len(splited) == 1 {
+		containerPath = splited[0]
+		val = filepath.Clean(splited[0])
+	} else {
+		containerPath = splited[1]
+		val = fmt.Sprintf("%s:%s", splited[0], filepath.Clean(splited[1]))
+	}
+
+	if !filepath.IsAbs(containerPath) {
+		return val, fmt.Errorf("%s is not an absolute path", containerPath)
+	}
+	return val, nil
+}
+
+func ValidateEnv(val string) (string, error) {
+	arr := strings.Split(val, "=")
+	if len(arr) > 1 {
+		return val, nil
+	}
+	return fmt.Sprintf("%s=%s", val, os.Getenv(val)), nil
+}
+
+func ValidateHost(val string) (string, error) {
+	host, err := utils.ParseHost(DEFAULTHTTPHOST, DEFAULTHTTPPORT, val)
+	if err != nil {
+		return val, err
+	}
+	return host, nil
+}

utils.go

@@ -215,9 +215,9 @@ func MergeConfig(userConf, imageConf *Config) error {
 	return nil
 }
 
-func parseLxcConfOpts(opts utils.ListOpts) ([]KeyValuePair, error) {
-	out := make([]KeyValuePair, len(opts))
-	for i, o := range opts {
+func parseLxcConfOpts(opts ListOpts) ([]KeyValuePair, error) {
+	out := make([]KeyValuePair, opts.Len())
+	for i, o := range opts.GetAll() {
 		k, v, err := parseLxcOpt(o)
 		if err != nil {
 			return nil, err

utils/utils.go

@@ -34,18 +34,6 @@ type Fataler interface {
 	Fatal(args ...interface{})
 }
 
-// ListOpts type
-type ListOpts []string
-
-func (opts *ListOpts) String() string {
-	return fmt.Sprint(*opts)
-}
-
-func (opts *ListOpts) Set(value string) error {
-	*opts = append(*opts, value)
-	return nil
-}
-
 // Go is a basic promise implementation: it wraps calls a function in a goroutine,
 // and returns a channel which will later return the function's return value.
 func Go(f func() error) chan error {