Skip to content

Commit

Permalink
feat: add pretty printed default to validated flags (#4078)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pmalek
pmalek authored May 25, 2023
1 parent ccafa7c commit e237dc0
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 28 deletions.
11 changes: 6 additions & 5 deletions internal/manager/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ import (
"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane"
"github.com/kong/kubernetes-ingress-controller/v2/internal/konnect"
"github.com/kong/kubernetes-ingress-controller/v2/internal/manager/featuregates"
"github.com/kong/kubernetes-ingress-controller/v2/internal/manager/flags"
)

type OptionalNamespacedName = mo.Option[k8stypes.NamespacedName]

// Type override to be used with OptionalNamespacedName variables to override their type name printed in the help text.
var nnTypeNameOverride = WithTypeNameOverride[OptionalNamespacedName]("namespacedName")
var nnTypeNameOverride = flags.WithTypeNameOverride[OptionalNamespacedName]("namespacedName")

// -----------------------------------------------------------------------------
// Controller Manager - Config
Expand Down Expand Up @@ -154,7 +155,7 @@ func (c *Config) FlagSet() *pflag.FlagSet {
flagSet.StringSliceVar(&c.KongAdminURLs, "kong-admin-url", []string{"http://localhost:8001"},
`Kong Admin URL(s) to connect to in the format "protocol://address:port". `+
`More than 1 URL can be provided, in such case the flag should be used multiple times or a corresponding env variable should use comma delimited addresses.`)
flagSet.Var(NewValidatedValue(&c.KongAdminSvc, namespacedNameFromFlagValue, nnTypeNameOverride), "kong-admin-svc",
flagSet.Var(flags.NewValidatedValue(&c.KongAdminSvc, namespacedNameFromFlagValue, nnTypeNameOverride), "kong-admin-svc",
`Kong Admin API Service namespaced name in "namespace/name" format, to use for Kong Gateway service discovery.`)
flagSet.StringSliceVar(&c.KondAdminSvcPortNames, "kong-admin-svc-port-names", []string{"admin", "admin-tls", "kong-admin", "kong-admin-tls"},
"Names of ports on Kong Admin API service to take into account when doing gateway discovery.")
Expand All @@ -173,7 +174,7 @@ func (c *Config) FlagSet() *pflag.FlagSet {
)

// Kubernetes configurations
flagSet.Var(NewValidatedValue(&c.GatewayAPIControllerName, gatewayAPIControllerNameFromFlagValue, WithDefault(string(gateway.GetControllerName()))), "gateway-api-controller-name", "The controller name to match on Gateway API resources.")
flagSet.Var(flags.NewValidatedValue(&c.GatewayAPIControllerName, gatewayAPIControllerNameFromFlagValue, flags.WithDefault(string(gateway.GetControllerName()))), "gateway-api-controller-name", "The controller name to match on Gateway API resources.")
flagSet.StringVar(&c.KubeconfigPath, "kubeconfig", "", "Path to the kubeconfig file.")
flagSet.StringVar(&c.IngressClassName, "ingress-class", annotations.DefaultIngressClass, `Name of the ingress class to route through this controller.`)
flagSet.StringVar(&c.LeaderElectionID, "election-id", "5b374a9e.konghq.com", `Election id to use for status update.`)
Expand All @@ -184,12 +185,12 @@ func (c *Config) FlagSet() *pflag.FlagSet {
`Namespace(s) to watch for Kubernetes resources. Defaults to all namespaces. To watch multiple namespaces, use a comma-separated list of namespaces.`)

// Ingress status
flagSet.Var(NewValidatedValue(&c.PublishService, namespacedNameFromFlagValue, nnTypeNameOverride), "publish-service",
flagSet.Var(flags.NewValidatedValue(&c.PublishService, namespacedNameFromFlagValue, nnTypeNameOverride), "publish-service",
`Service fronting Ingress resources in "namespace/name" format. The controller will update Ingress status information with this Service's endpoints.`)
flagSet.StringSliceVar(&c.PublishStatusAddress, "publish-status-address", []string{},
`User-provided addresses in comma-separated string format, for use in lieu of "publish-service" `+
`when that Service lacks useful address information (for example, in bare-metal environments).`)
flagSet.Var(NewValidatedValue(&c.PublishServiceUDP, namespacedNameFromFlagValue, nnTypeNameOverride), "publish-service-udp", `Service fronting UDP routing resources in `+
flagSet.Var(flags.NewValidatedValue(&c.PublishServiceUDP, namespacedNameFromFlagValue, nnTypeNameOverride), "publish-service-udp", `Service fronting UDP routing resources in `+
`"namespace/name" format. The controller will update UDP route status information with this Service's `+
`endpoints. If omitted, the same Service will be used for both TCP and UDP routes.`)
flagSet.StringSliceVar(&c.PublishStatusAddressUDP, "publish-status-address-udp", []string{},
Expand Down
18 changes: 17 additions & 1 deletion internal/manager/validated.go → internal/manager/flags/validated.go
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package manager
package flags

import "fmt"

Expand All @@ -8,7 +8,23 @@ type ValidatedValueOpt[T any] func(*ValidatedValue[T])
func WithDefault[T any](defaultValue T) ValidatedValueOpt[T] {
return func(v *ValidatedValue[T]) {
*v.variable = defaultValue

// Assign origin which is used in ValidatedValue[T]'s String() string
// func so that we get a pretty printed default.
v.origin = stringFromAny(defaultValue)
}
}

func stringFromAny(s any) string {
if stringer, ok := s.(fmt.Stringer); ok {
return fmt.Sprintf("%q", stringer.String())
}

if str, ok := s.(string); ok {
return str
}

panic(fmt.Errorf("unknown type %T", s))
}

// WithTypeNameOverride overrides the type name that's printed in the help message.
Expand Down
84 changes: 62 additions & 22 deletions internal/manager/validated_test.go → internal/manager/flags/validated_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package manager_test
package flags_test

import (
"bytes"
"errors"
"fmt"
"strings"
Expand All @@ -9,31 +10,31 @@ import (
"github.com/spf13/pflag"
"github.com/stretchr/testify/require"

"github.com/kong/kubernetes-ingress-controller/v2/internal/manager"
"github.com/kong/kubernetes-ingress-controller/v2/internal/manager/flags"
)

func TestValidatedValue(t *testing.T) {
flags := func() *pflag.FlagSet { return pflag.NewFlagSet("", pflag.ContinueOnError) }
createFlagSet := func() *pflag.FlagSet { return pflag.NewFlagSet("", pflag.ContinueOnError) }

t.Run("string", func(t *testing.T) {
flags := flags()
flagSet := createFlagSet()
var validatedString string
flags.Var(manager.NewValidatedValue(&validatedString, func(s string) (string, error) {
flagSet.Var(flags.NewValidatedValue(&validatedString, func(s string) (string, error) {
if !strings.Contains(s, "magic-token") {
return "", errors.New("no magic token passed")
}
return s, nil
}), "validated-string", "")

t.Run("invalid", func(t *testing.T) {
err := flags.Parse([]string{
err := flagSet.Parse([]string{
"--validated-string", "invalid value",
})
require.Error(t, err)
})

t.Run("valid", func(t *testing.T) {
err := flags.Parse([]string{
err := flagSet.Parse([]string{
"--validated-string", "magic-token",
})
require.NoError(t, err)
Expand All @@ -42,12 +43,12 @@ func TestValidatedValue(t *testing.T) {
})

t.Run("struct", func(t *testing.T) {
flags := flags()
flagSet := createFlagSet()
type customType struct {
p1, p2 string
}
var customTypeVar customType
flags.Var(manager.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
flagSet.Var(flags.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
parts := strings.Split(s, "/")
if len(parts) != 2 {
return customType{}, fmt.Errorf("expected '<string>/<string>' format, got: %q", s)
Expand All @@ -57,43 +58,44 @@ func TestValidatedValue(t *testing.T) {
}), "custom-type", "")

t.Run("valid", func(t *testing.T) {
err := flags.Parse([]string{"--custom-type", "valid/value"})
err := flagSet.Parse([]string{"--custom-type", "valid/value"})
require.NoError(t, err)
require.Equal(t, customType{p1: "valid", p2: "value"}, customTypeVar)
})

t.Run("invalid", func(t *testing.T) {
err := flags.Parse([]string{"--custom-type", "invalid/format/"})
err := flagSet.Parse([]string{"--custom-type", "invalid/format/"})
require.ErrorContains(t, err, "expected '<string>/<string>'")
})
})

t.Run("with default", func(t *testing.T) {
flags := flags()
flagSet := createFlagSet()

var validatedString string
flags.Var(manager.NewValidatedValue(&validatedString, func(s string) (string, error) {
flagSet.Var(flags.NewValidatedValue(&validatedString, func(s string) (string, error) {
if !strings.Contains(s, "magic-token") {
return "", errors.New("no magic token passed")
}
return s, nil
}, manager.WithDefault("default-value")), "flag-with-default", "")
}, flags.WithDefault("default-value")), "flag-with-default", "")

t.Run("empty", func(t *testing.T) {
err := flags.Parse(nil)
err := flagSet.Parse(nil)
require.NoError(t, err)
v := validatedString
require.Equal(t, "default-value", v)
})

t.Run("invalid", func(t *testing.T) {
err := flags.Parse([]string{
err := flagSet.Parse([]string{
"--flag-with-default", "invalid value",
})
require.Error(t, err)
})

t.Run("valid", func(t *testing.T) {
err := flags.Parse([]string{
err := flagSet.Parse([]string{
"--flag-with-default", "magic-token",
})
require.NoError(t, err)
Expand All @@ -102,10 +104,48 @@ func TestValidatedValue(t *testing.T) {
})
}

type customStringer struct{}

func (cs customStringer) String() string {
return "custom-string-default"
}

func TestValidatedValue_WithDefault(t *testing.T) {
createFlagSet := func() *pflag.FlagSet { return pflag.NewFlagSet("", pflag.ContinueOnError) }

t.Run("default printed in usage for string flag", func(t *testing.T) {
flagSet := createFlagSet()

var validatedString string
flagSet.Var(flags.NewValidatedValue(&validatedString, func(s string) (string, error) {
return s, nil
}, flags.WithDefault("default-value")), "flag-with-default", "")

b := bytes.Buffer{}
flagSet.SetOutput(&b)
flagSet.PrintDefaults()
require.Contains(t, b.String(), `(default "default-value")`)
})

t.Run("default printed in usage for fmt.Stringer flag", func(t *testing.T) {
flagSet := createFlagSet()

var cs customStringer
flagSet.Var(flags.NewValidatedValue(&cs, func(s string) (customStringer, error) {
return customStringer{}, nil
}, flags.WithDefault(customStringer{})), "flag-with-default", "")

b := bytes.Buffer{}
flagSet.SetOutput(&b)
flagSet.PrintDefaults()
require.Contains(t, b.String(), `(default "custom-string-default")`)
})
}

func TestValidatedValue_Type(t *testing.T) {
t.Run("string", func(t *testing.T) {
var validatedString string
vv := manager.NewValidatedValue(&validatedString, func(s string) (string, error) {
vv := flags.NewValidatedValue(&validatedString, func(s string) (string, error) {
return s, nil
})
require.Equal(t, "string", vv.Type())
Expand All @@ -114,18 +154,18 @@ func TestValidatedValue_Type(t *testing.T) {
t.Run("struct", func(t *testing.T) {
type customType struct{}
var customTypeVar customType
vv := manager.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
vv := flags.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
return customType{}, nil
})
require.Equal(t, "manager_test.customType", vv.Type())
require.Equal(t, "flags_test.customType", vv.Type())
})

t.Run("overridden type name", func(t *testing.T) {
type customType struct{}
var customTypeVar customType
vv := manager.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
vv := flags.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
return customType{}, nil
}, manager.WithTypeNameOverride[customType]("custom-type-override"))
}, flags.WithTypeNameOverride[customType]("custom-type-override"))
require.Equal(t, "custom-type-override", vv.Type())
})
}

0 comments on commit e237dc0

Please sign in to comment.