From 902eaecd577b235000e24a42c89a9bda1aec0770 Mon Sep 17 00:00:00 2001 From: Luke Kingland <58986931+lkingland@users.noreply.github.com> Date: Fri, 17 Feb 2023 05:07:52 +0900 Subject: [PATCH] global config member accessors (#1559) --- pkg/config/config.go | 139 ++++++++++++++++++++++++++++++++++++++ pkg/config/config_test.go | 121 +++++++++++++++++++++++++++++++++ 2 files changed, 260 insertions(+) diff --git a/pkg/config/config.go b/pkg/config/config.go index dc551235..780a9b85 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -4,6 +4,10 @@ import ( "fmt" "os" "path/filepath" + "reflect" + "sort" + "strconv" + "strings" "gopkg.in/yaml.v2" "knative.dev/func/pkg/builders" @@ -49,6 +53,9 @@ type Global struct { Namespace string `yaml:"namespace,omitempty"` Registry string `yaml:"registry,omitempty"` Verbose bool `yaml:"verbose,omitempty"` + // NOTE: all members must include their yaml serialized names, even when + // this is the default, because these tag values are used for the static + // getter/setter accessors to match requests. } // New Config struct with all members set to static defaults. See NewDefaults @@ -108,6 +115,11 @@ func Load(path string) (c Global, err error) { } // Write the config to the given path +// To use the currently configured path (used by the constructor) use File() +// +// c := config.NewDefault() +// c.Verbose = true +// c.Write(config.File()) func (c Global) Write(path string) (err error) { bb, _ := yaml.Marshal(&c) // Marshaling no longer errors; this is back compat return os.WriteFile(path, bb, os.ModePerm) @@ -214,3 +226,130 @@ func CreatePaths() (err error) { } return } + +// Static Accessors +// +// Accessors to globally configurable options are implemented as static +// package functions to retain the benefits of pass-by-value already in use +// on most system structures. +// c = config.Set(c, "key", "value") +// +// This may initially seem confusing to those used to accessors implemented +// as object member functions (`c.Set("x","y"`), but it appears to be worth it: +// +// This method follows the familiar paradigm of other Go builtins (which made +// their choice for similar reasons): +// args = append(args, "newarg") +// ... and thus likewise: +// c = config.Set(c, "key", "value") +// +// We indeed could have implemented these in a more familiar Getter/Setter way +// by making this method have a pointer receiver: +// func (c *Global) Set(key, value string) +// However, this would require the user of the config object to, likely more +// confusingly, declare a new local variable to hold their pointer +// (or perhaps worse, abandon the benefits of pass-by-value from the config +// constructor and always return a pointer from the constructor): +// +// globalConfig := config.NewDefault() +// .... later that day ... +// c := &globalConfig +// c.Set("builder", "foo") +// +// This solution, while it would preserve the very narrow familiar usage of +// 'Set', fails to be clear due to the setup involved (requiring the allocation +// of that pointer). Therefore the accessors are herein implemented more +// functionally, as package static methods. + +// List the globally configurable settings by the key which can be used +// in the accessors Get and Set, and in the associated disk serialized. +// Sorted. +// Implemented as a package-static function because Set is implemented as such. +// See the long-winded explanation above. +func List() []string { + keys := []string{} + t := reflect.TypeOf(Global{}) + for i := 0; i < t.NumField(); i++ { + tt := strings.Split(t.Field(i).Tag.Get("yaml"), ",") + keys = append(keys, tt[0]) + } + sort.Strings(keys) + return keys +} + +// Get the named global config value from the given global config struct. +// Nonexistent values return nil. +// Implemented as a package-static function because Set is implemented as such. +// See the long-winded explanation above. +func Get(c Global, name string) any { + t := reflect.TypeOf(c) + for i := 0; i < t.NumField(); i++ { + if !strings.HasPrefix(t.Field(i).Tag.Get("yaml"), name) { + continue + } + return reflect.ValueOf(c).FieldByName(t.Field(i).Name).Interface() + } + return nil +} + +// Set value of a member by name and a stringified value. +// Fails if the passed value can not be coerced into the value expected +// by the member indicated by name. +func Set(c Global, name, value string) (Global, error) { + fieldValue, err := getField(&c, name) + if err != nil { + return c, err + } + + var v reflect.Value + switch fieldValue.Kind() { + case reflect.String: + v = reflect.ValueOf(value) + case reflect.Bool: + boolValue, err := strconv.ParseBool(value) + if err != nil { + return c, err + } + v = reflect.ValueOf(boolValue) + default: + return c, fmt.Errorf("global config value type not yet implemented: %v", fieldValue.Kind()) + } + fieldValue.Set(v) + + return c, nil +} + +// SetString value of a member by name, returning the updated config. +func SetString(c Global, name, value string) (Global, error) { + return set(c, name, reflect.ValueOf(value)) +} + +// SetBool value of a member by name, returning the updated config. +func SetBool(c Global, name string, value bool) (Global, error) { + return set(c, name, reflect.ValueOf(value)) +} + +// TODO: add more typesafe setters as needed. + +// set using a reflect.Value +func set(c Global, name string, value reflect.Value) (Global, error) { + fieldValue, err := getField(&c, name) + if err != nil { + return c, err + } + fieldValue.Set(value) + return c, nil +} + +// Get an assignable reflect.Value for the struct field with the given yaml +// tag name. +func getField(c *Global, name string) (reflect.Value, error) { + t := reflect.TypeOf(c).Elem() + for i := 0; i < t.NumField(); i++ { + if strings.HasPrefix(t.Field(i).Tag.Get("yaml"), name) { + fieldValue := reflect.ValueOf(c).Elem().FieldByName(t.Field(i).Name) + return fieldValue, nil + } + } + return reflect.Value{}, fmt.Errorf("field not found on global config: %v", name) +} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 269997d4..60f54b70 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -3,6 +3,7 @@ package config_test import ( "os" "path/filepath" + "reflect" "testing" "knative.dev/func/pkg/config" @@ -293,3 +294,123 @@ func TestConfigure(t *testing.T) { } } + +// TestGet_Invalid ensures that attempting to get the value of a nonexistent +// member returns nil. +func TestGet_Invalid(t *testing.T) { + v := config.Get(config.Global{}, "invalid") + if v != nil { + t.Fatalf("expected accessing a nonexistent member to return nil, but got: %v", v) + } +} + +// TestGet_Valid ensures a valid field name returns the value for that field. +// Name is keyed off the yaml serialization key of the field rather than the +// (capitalized) exported member name of the struct in order to be consistent +// with the disk-serialized config file format, and thus integrate nicely with +// CLIs, etc. +func TestGet_Valid(t *testing.T) { + c := config.Global{ + Builder: "myBuilder", + Confirm: true, + } + // Get String + v := config.Get(c, "builder") + if v != "myBuilder" { + t.Fatalf("Did not receive expected value for builder. got: %v", v) + } + // Get Boolean + v = config.Get(c, "confirm") + if v != true { + t.Fatalf("Did not receive expected value for builder. got: %v", v) + } +} + +// TestSet_Invalid ensures that attemptint to set an invalid field errors. +func TestSet_Invalid(t *testing.T) { + _, err := config.SetString(config.Global{}, "invalid", "foo") + if err == nil { + t.Fatal("did not receive expected error setting a nonexistent field") + } +} + +// TestSet_ValidTyped ensures that attempting to set attributes with valid +// names and typed values succeeds. +func TestSet_ValidTyped(t *testing.T) { + cfg := config.Global{} + + // Set a String + cfg, err := config.SetString(cfg, "builder", "myBuilder") + if err != nil { + t.Fatal(err) + } + if cfg.Builder != "myBuilder" { + t.Fatalf("unexpected value for config builder: %v", cfg.Builder) + } + + // Set a Bool + cfg, err = config.SetBool(cfg, "confirm", true) + if err != nil { + t.Fatal(err) + } + if cfg.Builder != "myBuilder" { + t.Fatalf("unexpected value for config builder: %v", cfg.Builder) + } + + // TODO lazily populate typed accessors if/when global config expands to + // include types of additional values. +} + +// TestSet_ValidStrings ensures that setting valid attribute names using +// the string representation of their values succeeds. +func TestSet_ValidStrings(t *testing.T) { + cfg := config.Global{} + + // Set a String from a string + // should be the base case + cfg, err := config.Set(cfg, "builder", "myBuilder") + if err != nil { + t.Fatal(err) + } + if cfg.Builder != "myBuilder" { + t.Fatalf("unexpected value for config builder: %v", cfg.Builder) + } + + // Set a Bool + cfg, err = config.SetBool(cfg, "confirm", true) + if err != nil { + t.Fatal(err) + } + if cfg.Builder != "myBuilder" { + t.Fatalf("unexpected value for config builder: %v", cfg.Builder) + } + + // TODO: lazily populate support of additional types in the implementation + // as needed. +} + +// TestList ensures that the expected result is returned when listing +// the current names and values of the global config. +// The name is the name that can be used with Get and Set. The value is the +// string serialization of the value for the given name. +func TestList(t *testing.T) { + values := config.List() + expected := []string{ + "builder", + "confirm", + "language", + "namespace", + "registry", + "verbose", + } + + if !reflect.DeepEqual(values, expected) { + t.Logf("expected:\n%v", expected) + t.Logf("received:\n%v", values) + t.Fatalf("unexpected list of configurable options.") + } + + // NOTE: due to the strictness of this test, a new slice member will need + // to be added for each new field added to global config. + +}