Make config write maintain comments

cmd/crictl/config.go

@@ -23,7 +23,6 @@ import (
 	"strings"
 
 	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli/v2"
 
 	"github.com/kubernetes-sigs/cri-tools/pkg/common"
@@ -78,7 +77,7 @@ var configCommand = &cli.Command{
 			case "debug":
 				fmt.Println(config.Debug)
 			default:
-				logrus.Fatalf("no configuration option named %s", get)
+				return errors.Errorf("no configuration option named %s", get)
 			}
 			return nil
 		} else if context.IsSet("set") {
@@ -88,12 +87,12 @@ var configCommand = &cli.Command{
 				for _, option := range options {
 					pair := strings.Split(option, "=")
 					if len(pair) != 2 {
-						return fmt.Errorf("incorrectly specified option: %v", setting)
+						return errors.Errorf("incorrectly specified option: %v", setting)
 					}
 					key := pair[0]
 					value := pair[1]
 					if err := setValue(key, value, config); err != nil {
-						logrus.Fatal(err)
+						return err
 					}
 				}
 			}
@@ -105,7 +104,7 @@ var configCommand = &cli.Command{
 			}
 			value := context.Args().Get(1)
 			if err := setValue(key, value, config); err != nil {
-				logrus.Fatal(err)
+				return err
 			}
 			return common.WriteConfig(config, configFile)
 		}
@@ -121,21 +120,17 @@ func setValue(key, value string, config *common.Config) error {
 	case "timeout":
 		n, err := strconv.Atoi(value)
 		if err != nil {
-			return err
+			return errors.Wrapf(err, "parse timeout value '%s'", value)
 		}
 		config.Timeout = n
 	case "debug":
-		var debug bool
-		if value == "true" {
-			debug = true
-		} else if value == "false" {
-			debug = false
-		} else {
-			return fmt.Errorf("use true|false for 'debug'")
+		debug, err := strconv.ParseBool(value)
+		if err != nil {
+			return errors.Wrapf(err, "parse debug value '%s'", value)
 		}
 		config.Debug = debug
 	default:
-		return fmt.Errorf("no configuration option named %s", key)
+		return errors.Errorf("no configuration option named %s", key)
 	}
 	return nil
 }

go.mod

@@ -20,7 +20,7 @@ require (
 	golang.org/x/net v0.0.0-20191204025024-5ee1b9f4859a
 	golang.org/x/sys v0.0.0-20191204072324-ce4227a45e2e
 	google.golang.org/grpc v1.26.0
-	gopkg.in/yaml.v2 v2.2.8
+	gopkg.in/yaml.v3 v3.0.0-20200506231410-2ff61e1afc86
 	k8s.io/api v0.0.0
 	k8s.io/apimachinery v0.0.0
 	k8s.io/client-go v11.0.0+incompatible

go.sum

@@ -735,6 +735,8 @@ gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I=
 gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.8 h1:obN1ZagJSUGI0Ek/LBmuj4SNLPfIny3KsKFopxRdj10=
 gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
+gopkg.in/yaml.v3 v3.0.0-20200506231410-2ff61e1afc86 h1:OfFoIUYv/me30yv7XlMy4F9RJw8DEm8WQ6QG1Ph4bH0=
+gopkg.in/yaml.v3 v3.0.0-20200506231410-2ff61e1afc86/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gotest.tools v2.1.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw=
 gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo=
 gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw=

pkg/common/config.go

@@ -42,14 +42,13 @@ func GetServerConfigFromFile(configFileName, currentDir string) (*ServerConfigur
 	if _, err := os.Stat(configFileName); err != nil {
 		if !os.IsNotExist(err) {
 			return nil, errors.Wrap(err, "load config file")
-		} else {
-			// If the config file was not found, try looking in the program's
-			// directory as a fallback. This is to accommodate where the config file
-			// is placed with the cri tools binary.
-			configFileName = filepath.Join(filepath.Dir(currentDir), "crictl.yaml")
-			if _, err := os.Stat(configFileName); err != nil {
-				return nil, errors.Wrap(err, "load config file")
-			}
+		}
+		// If the config file was not found, try looking in the program's
+		// directory as a fallback. This is to accommodate where the config file
+		// is placed with the cri tools binary.
+		configFileName = filepath.Join(filepath.Dir(currentDir), "crictl.yaml")
+		if _, err := os.Stat(configFileName); err != nil {
+			return nil, errors.Wrap(err, "load config file")
 		}
 	}
 

pkg/common/file.go

@@ -20,17 +20,20 @@ import (
 	"io/ioutil"
 	"os"
 	gofilepath "path/filepath"
+	"strconv"
 
-	"gopkg.in/yaml.v2"
+	"github.com/pkg/errors"
+	"gopkg.in/yaml.v3"
 )
 
 // Config is the internal representation of the yaml that defines
 // server configuration
 type Config struct {
-	RuntimeEndpoint string `yaml:"runtime-endpoint"`
-	ImageEndpoint   string `yaml:"image-endpoint"`
-	Timeout         int    `yaml:"timeout"`
-	Debug           bool   `yaml:"debug"`
+	RuntimeEndpoint string
+	ImageEndpoint   string
+	Timeout         int
+	Debug           bool
+	yamlData        *yaml.Node //YAML representation of config
 }
 
 // ReadConfig reads from a file with the given name and returns a config or
@@ -40,23 +43,144 @@ func ReadConfig(filepath string) (*Config, error) {
 	if err != nil {
 		return nil, err
 	}
-	config := Config{}
-	err = yaml.Unmarshal(data, &config)
+	yamlConfig := yaml.Node{}
+	err = yaml.Unmarshal(data, &yamlConfig)
+	if err != nil {
+		return nil, err
+	}
+	config, err := getConfigOptions(yamlConfig)
 	if err != nil {
 		return nil, err
 	}
-	return &config, err
+	return config, err
 }
 
 // WriteConfig writes config to file
 // an error if the file was unable to be written to.
 func WriteConfig(c *Config, filepath string) error {
-	data, err := yaml.Marshal(c)
+	if c == nil {
+		c = new(Config)
+		c.yamlData = new(yaml.Node)
+	}
+
+	setConfigOptions(c)
+
+	data, err := yaml.Marshal(c.yamlData)
 	if err != nil {
 		return err
 	}
+
 	if err := os.MkdirAll(gofilepath.Dir(filepath), 0o755); err != nil {
 		return err
 	}
 	return ioutil.WriteFile(filepath, data, 0o644)
 }
+
+// Extracts config options from the yaml data which is loaded from file
+func getConfigOptions(yamlData yaml.Node) (*Config, error) {
+	config := Config{}
+	config.yamlData = &yamlData
+
+	if yamlData.Content == nil || len(yamlData.Content) == 0 ||
+		yamlData.Content[0].Content == nil {
+		return &config, nil
+	}
+	contentLen := len(yamlData.Content[0].Content)
+
+	// YAML representation contains 2 yaml ScalarNodes per config option.
+	// One is config option name and other is the value of the option
+	// These ScalarNodes help preserve comments associated with
+	// the YAML entry
+	for indx := 0; indx < contentLen-1; {
+		configOption := yamlData.Content[0].Content[indx]
+		name := configOption.Value
+		value := yamlData.Content[0].Content[indx+1].Value
+		var err error
+		switch name {
+		case "runtime-endpoint":
+			config.RuntimeEndpoint = value
+		case "image-endpoint":
+			config.ImageEndpoint = value
+		case "timeout":
+			config.Timeout, err = strconv.Atoi(value)
+			if err != nil {
+				return nil, errors.Wrapf(err, "parsing config option '%s'", name)
+			}
+		case "debug":
+			config.Debug, err = strconv.ParseBool(value)
+			if err != nil {
+				return nil, errors.Wrapf(err, "parsing config option '%s'", name)
+			}
+		default:
+			return nil, errors.Errorf("Config option '%s' is not valid", name)
+		}
+		indx += 2
+	}
+
+	return &config, nil
+}
+
+// Set config options on yaml data for persistece to file
+func setConfigOptions(config *Config) {
+	setConfigOption("runtime-endpoint", config.RuntimeEndpoint, config.yamlData)
+	setConfigOption("image-endpoint", config.ImageEndpoint, config.yamlData)
+	setConfigOption("timeout", strconv.Itoa(config.Timeout), config.yamlData)
+	setConfigOption("debug", strconv.FormatBool(config.Debug), config.yamlData)
+}
+
+// Set config option on yaml
+func setConfigOption(configName, configValue string, yamlData *yaml.Node) {
+	if yamlData.Content == nil || len(yamlData.Content) == 0 {
+		yamlData.Kind = yaml.DocumentNode
+		yamlData.Content = make([]*yaml.Node, 1)
+		yamlData.Content[0] = &yaml.Node{
+			Kind: yaml.MappingNode,
+			Tag:  "!!map",
+		}
+	}
+	var contentLen = 0
+	var foundOption = false
+	if yamlData.Content[0].Content != nil {
+		contentLen = len(yamlData.Content[0].Content)
+	}
+
+	// Set value on existing config option
+	for indx := 0; indx < contentLen-1; {
+		name := yamlData.Content[0].Content[indx].Value
+		if name == configName {
+			yamlData.Content[0].Content[indx+1].Value = configValue
+			foundOption = true
+			break
+		}
+		indx += 2
+	}
+
+	// New config option to set
+	// YAML representation contains 2 yaml ScalarNodes per config option.
+	// One is config option name and other is the value of the option
+	// These ScalarNodes help preserve comments associated with
+	// the YAML entry
+	if !foundOption {
+		name := &yaml.Node{
+			Kind:  yaml.ScalarNode,
+			Value: configName,
+			Tag:   "!!str",
+		}
+		var tagType string
+		switch configName {
+		case "timeout":
+			tagType = "!!int"
+		case "debug":
+			tagType = "!!bool"
+		default:
+			tagType = "!!str"
+		}
+
+		value := &yaml.Node{
+			Kind:  yaml.ScalarNode,
+			Value: configValue,
+			Tag:   tagType,
+		}
+		yamlData.Content[0].Content = append(yamlData.Content[0].Content, name, value)
+	}
+}