Refactor config into two-phase processing with shared flags (#9)

* refactor: split config processing into PreProcess and Process phases

* make lint and make test

Author: liamcervante

internal/api/dashboard/config.go

@@ -3,11 +3,14 @@ package dashboard
 import (
 	"net/http"
 
+	"github.com/infracost/cli/internal/config/process"
 	"github.com/infracost/cli/pkg/environment"
 )
 
 var (
-	defaultValues = map[environment.Environment]map[string]string{
+	_ process.Processor = (*Config)(nil)
+
+	defaultValues = map[string]map[string]string{
 		environment.Production: {
 			"endpoint": "https://dashboard.api.infracost.io",
 		},
@@ -21,7 +24,14 @@ var (
 )
 
 type Config struct {
-	Endpoint string `env:"INFRACOST_CLI_DASHBOARD_ENDPOINT" flag:"dashboard-endpoint;hidden" usage:"The endpoint for the Infracost dashboard"`
+	Environment string `flagvalue:"environment"`
+	Endpoint    string `env:"INFRACOST_CLI_DASHBOARD_ENDPOINT" flag:"dashboard-endpoint;hidden" usage:"The endpoint for the Infracost dashboard"`
+}
+
+func (c *Config) Process() {
+	if c.Endpoint == "" {
+		c.Endpoint = defaultValues[c.Environment]["endpoint"]
+	}
 }
 
 func (c *Config) Client(client *http.Client) *Client {
@@ -30,9 +40,3 @@ func (c *Config) Client(client *http.Client) *Client {
 		config: c,
 	}
 }
-
-func (c *Config) ApplyDefaults(env environment.Environment) {
-	if c.Endpoint == "" {
-		c.Endpoint = defaultValues[env]["endpoint"]
-	}
-}

internal/cache/cache_test.go

@@ -51,7 +51,7 @@ func testOutput() format.Output {
 func testConfig(t *testing.T) *Config {
 	t.Helper()
 	c := &Config{Cache: filepath.Join(t.TempDir(), "cache")}
-	c.ApplyDefaults()
+	c.Process()
 	return c
 }
 

internal/cache/config.go

@@ -7,10 +7,16 @@ import (
 	"path/filepath"
 	"time"
 
+	"github.com/infracost/cli/internal/api/events"
+	config "github.com/infracost/cli/internal/config/process"
 	"github.com/infracost/cli/internal/logging"
 	"github.com/shirou/gopsutil/process"
 )
 
+var (
+	_ config.Processor = (*Config)(nil)
+)
+
 type Config struct {
 	// Cache is where the cache files should go.
 	Cache string `env:"INFRACOST_CLI_CACHE_DIRECTORY"`
@@ -23,7 +29,7 @@ type Config struct {
 	SessionID string `env:"INFRACOST_SESSION_ID"`
 }
 
-func (c *Config) ApplyDefaults() {
+func (c *Config) Process() {
 	if len(c.Cache) == 0 {
 		c.Cache = defaultCachePath()
 	}
@@ -33,6 +39,7 @@ func (c *Config) ApplyDefaults() {
 	if c.SessionID == "" {
 		c.SessionID = getSessionID()
 	}
+	events.RegisterMetadata("session", c.SessionID)
 }
 
 func defaultCachePath() string {

internal/config/config.go

@@ -6,12 +6,15 @@ import (
 	"github.com/infracost/cli/internal/api/dashboard"
 	"github.com/infracost/cli/internal/api/events"
 	"github.com/infracost/cli/internal/cache"
+	"github.com/infracost/cli/internal/config/process"
 	"github.com/infracost/cli/internal/logging"
 	"github.com/infracost/cli/pkg/auth"
 	"github.com/infracost/cli/pkg/environment"
 	"github.com/infracost/cli/pkg/plugins"
-	"github.com/spf13/cobra"
-	"github.com/spf13/pflag"
+)
+
+var (
+	_ process.Processor = (*Config)(nil)
 )
 
 // Config contains the configuration for the CLI.
@@ -51,18 +54,9 @@ type Config struct {
 	Cache cache.Config
 }
 
-func (config *Config) RegisterEventMetadata(cmd *cobra.Command) {
-	events.RegisterMetadata("command", cmd.Name())
-	events.RegisterMetadata("flags", func() []string {
-		var flags []string
-		cmd.Flags().Visit(func(flag *pflag.Flag) {
-			flags = append(flags, flag.Name)
-		})
-		return flags
-	}())
-	events.RegisterMetadata("session", config.Cache.SessionID)
+func (config *Config) Process() {
 	events.RegisterMetadata("cloudEnabled", os.Getenv("INFRACOST_ENABLE_CLOUD") == "true")
 	events.RegisterMetadata("dashboardEnabled", os.Getenv("INFRACOST_ENABLE_DASHBOARD") == "true")
-	events.RegisterMetadata("environment", string(config.Environment))
+	events.RegisterMetadata("environment", config.Environment.String())
 	events.RegisterMetadata("isDefaultPricingApiEndpoint", config.PricingEndpoint == "https://pricing.api.infracost.io")
 }

internal/config/config_test.go

@@ -0,0 +1,27 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/infracost/cli/internal/config/process"
+	"github.com/spf13/pflag"
+	"github.com/stretchr/testify/require"
+)
+
+func TestConfig_Process(t *testing.T) {
+	var cfg Config
+
+	flags := pflag.NewFlagSet("", pflag.ContinueOnError)
+
+	// first, make sure that preprocess doesn't error or panic when no values provided.
+	if diags := process.PreProcess(&cfg, flags); diags.Len() != 0 {
+		t.Fatal(diags)
+	}
+	require.NoError(t, flags.Parse(nil)) // we have no required flags yet, so will provide nothing
+	process.Process(&cfg)                // make sure doesn't panic
+
+	// environment is a shared flag, so let's make sure that all worked
+	require.Equal(t, "prod", cfg.Environment.String())
+	require.Equal(t, "prod", cfg.Auth.Environment)
+	require.Equal(t, "prod", cfg.Dashboard.Environment)
+}

internal/config/process.go internal/config/process/preprocess.gointernal/config/process.go renamed to internal/config/process/preprocess.go

@@ -1,4 +1,4 @@
-package config
+package process
 
 import (
 	"fmt"
@@ -17,50 +17,32 @@ var (
 	valueType = reflect.TypeOf((*pflag.Value)(nil)).Elem()
 )
 
-// Process populates a struct's fields from environment variables and command-line flags.
-//
-// The target must be a pointer to a struct. Each field can be tagged with `env`, `flag`, `usage`,
-// and `default` to specify how the field should be populated.
-//
-// If no tag is specified, the field name is used as the basis for both the environment variable
-// (converted to SCREAMING_SNAKE_CASE) and the flag name (converted to kebab-case). This behavior
-// can be disabled by setting the `env` or `flag` tag to `-`.
-//
-// Example:
-//
-//	type MyConfig struct {
-//	  MyField string `env:"MY_FIELD" flag:"my-field;hidden" usage:"my field usage" default:"my-default"`
-//	}
-//
-// Flags can be marked as hidden by appending `;hidden` to the `flag` tag.
-//
-// Process supports nested structs, and basic types such as strings, integers, and booleans.
-// Additionally, any type implementing the pflag.Value interface is supported.
-func Process(target interface{}, flags *pflag.FlagSet) *diagnostic.Diagnostics {
+// PreProcess walks the target struct and hydrates fields based on struct tags.
+// Fields tagged with `env:"VAR"` are populated from environment variables,
+// `default:"val"` sets a fallback when no env var is found and the field is zero,
+// and `flag:"name"` registers a pflag on the provided FlagSet. Nested structs
+// are recursed into unless they implement pflag.Value, in which case they are
+// treated as leaf values. The target must be a pointer to a struct.
+func PreProcess(target interface{}, flags *pflag.FlagSet) *diagnostic.Diagnostics {
 	v := reflect.ValueOf(target)
 
 	if v.Kind() == reflect.Interface {
-		// we'll support interfaces, but we'll only allow one level of indirection
-		// basically, just allows people to pass in things that have been assigned to a interface{} or any type
-		// constraint
 		v = v.Elem()
 	}
 
 	if v.Kind() != reflect.Pointer {
-		// but, we must have a pointer to a struct at the next level
 		panic("target must be a pointer to a struct")
 	}
-	v = v.Elem() // unpack the pointer
+	v = v.Elem()
 
 	if v.Kind() != reflect.Struct {
-		// we must now actually have the struct we're going to be working on
 		panic("target must be a pointer to a struct")
 	}
 
-	return processStruct(v, flags)
+	return preprocess(v, flags)
 }
 
-func processStruct(v reflect.Value, flags *pflag.FlagSet) *diagnostic.Diagnostics {
+func preprocess(v reflect.Value, flags *pflag.FlagSet) *diagnostic.Diagnostics {
 	var diags *diagnostic.Diagnostics
 
 	t := v.Type()
@@ -74,31 +56,37 @@ func processStruct(v reflect.Value, flags *pflag.FlagSet) *diagnostic.Diagnostic
 
 		envName, hasEnvName := field.Tag.Lookup("env")
 		flagValue, hasFlagValue := field.Tag.Lookup("flag")
+		flagTargetName, hasFlagTarget := field.Tag.Lookup("flagvalue")
 		defaultValue, hasDefaultValue := field.Tag.Lookup("default")
 
 		hasEnvName = hasEnvName && envName != ""
 		hasFlagValue = hasFlagValue && flagValue != ""
+		hasFlagTarget = hasFlagTarget && flagTargetName != ""
 		hasDefaultValue = hasDefaultValue && defaultValue != ""
 
 		currentType, parentType := unpackType(fieldValue.Type(), fieldValue.Addr().Type())
 		isPflagValue := parentType.Implements(valueType)
 
 		if currentType.Kind() == reflect.Struct && !isPflagValue {
-			if hasEnvName || hasFlagValue || hasDefaultValue {
+			if hasEnvName || hasFlagValue || hasDefaultValue || hasFlagTarget {
 				// programmer error, so we panic
-				panic("nested structs cannot be tagged with env, flag or default, or they must implement pflag.Value")
+				panic("nested structs cannot be tagged with env, flag, flagvalue or default, or they must implement pflag.Value")
 			}
 
 			current, _ := unpackValue(fieldValue)
 
 			// Then we have a struct that needs to be processed recursively.
-			if err := processStruct(current, flags); err != nil {
+			if err := preprocess(current, flags); err != nil {
 				return err
 			}
 		}
 
-		if !hasEnvName && !hasFlagValue && !hasDefaultValue {
-			// if we have no env, flag or default value then we're not going to touch this field
+		if hasFlagTarget && (hasFlagValue || hasEnvName || hasDefaultValue) {
+			panic("flagvalue cannot be combined with flag, env, or default tags")
+		}
+
+		if !hasEnvName && !hasFlagValue && !hasFlagTarget && !hasDefaultValue {
+			// if we have no env, flag, flagvalue or default value then we're not going to touch this field
 			continue
 		}
 
@@ -142,11 +130,35 @@ func processStruct(v reflect.Value, flags *pflag.FlagSet) *diagnostic.Diagnostic
 			registerFlag(parent, flags, flagName, field.Tag.Get("usage"), hidden, isPflagValue)
 		}
 
+		if hasFlagTarget {
+			existing := flags.Lookup(flagTargetName)
+			if existing == nil {
+				panic(fmt.Sprintf("flagvalue %q references flag that has not been registered", flagTargetName))
+			}
+
+			sf, ok := existing.Value.(SharedFlag)
+			if !ok {
+				panic(fmt.Sprintf("flagvalue %q references flag that does not implement SharedFlag", flagTargetName))
+			}
+
+			if fieldValue.Kind() != reflect.String {
+				panic(fmt.Sprintf("flagvalue %q can only be used on string fields", flagTargetName))
+			}
+
+			target := fieldValue.Addr().Interface().(*string)
+			if existing.DefValue != "" {
+				*target = existing.DefValue
+			}
+			sf.AddTarget(target)
+		}
+
 	}
 
 	return diags
 }
 
+// setFieldValue sets a reflected field's value from a string. It handles
+// pflag.Value types, time.Duration, and primitive kinds (string, int, bool).
 func setFieldValue(v reflect.Value, s string, isPflagValue bool) error {
 	if isPflagValue {
 		pv := v.Addr().Interface().(pflag.Value)
@@ -243,30 +255,3 @@ func registerFlag(v reflect.Value, flags *pflag.FlagSet, name string, usage stri
 		}
 	}
 }
-
-// unpackType will unwrap the type, iterating through pointers and interfaces until the real type has been discovered.
-// It returns the source type and the parent type to it.
-func unpackType(t reflect.Type, parent reflect.Type) (reflect.Type, reflect.Type) {
-	for t.Kind() == reflect.Pointer {
-		parent = t
-		t = t.Elem() // then unpack all the pointers to get the real core type
-	}
-	return t, parent
-}
-
-// unpackValue will unwrap the provided value, iterating through pointers until the real value has been discovered.
-// It returns the source value and the parent value to it.
-//
-// We'll initialize pointers as we go, but not the inner most pointer. Callers must check the returned parent value
-// for nil, before they try and set any values on the returned value.
-func unpackValue(value reflect.Value) (reflect.Value, reflect.Value) {
-	parent := value.Addr()
-	for value.Kind() == reflect.Pointer {
-		parent = value
-		if value.IsNil() {
-			value.Set(reflect.New(value.Type().Elem()))
-		}
-		value = value.Elem()
-	}
-	return value, parent
-}