From 0e42b0b6fdaa565b8e0f4410e6cc61f01b9ed219 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Wed, 5 Feb 2025 12:36:52 +0000 Subject: [PATCH] internal/envflag: support int and string flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As requested by Marcel, it would be useful for CUE_DEBUG to support a logging level which would be useful for debugging the evaluator. Similarly, we have talked about transitioning away from a one-off evalv3 boolean experiment flag into an "eval" version string flag, meaning that we could set eval=v2, eval=v3, and also reuse it for future versions like eval=v4. Signed-off-by: Daniel Martí Change-Id: I1a4de176701f31c1b9b5586efbc6fd6b9483dac7 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1208350 Reviewed-by: Roger Peppe TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine --- internal/envflag/flag.go | 76 +++++++++++++++++++++++------------ internal/envflag/flag_test.go | 42 ++++++++++++++++++- 2 files changed, 91 insertions(+), 27 deletions(-) diff --git a/internal/envflag/flag.go b/internal/envflag/flag.go index 2e9699ec2d5..f7fddcd7655 100644 --- a/internal/envflag/flag.go +++ b/internal/envflag/flag.go @@ -44,18 +44,16 @@ func Parse[T any](flags *T, env string) error { for i := 0; i < ft.NumField(); i++ { field := ft.Field(i) name := strings.ToLower(field.Name) - defaultValue := false if tagStr, ok := field.Tag.Lookup("envflag"); ok { for _, f := range strings.Split(tagStr, ",") { key, rest, hasRest := strings.Cut(f, ":") switch key { case "default": - v, err := strconv.ParseBool(rest) + val, err := parseValue(name, field.Type.Kind(), rest) if err != nil { - return fmt.Errorf("invalid default bool value for %s: %v", field.Name, err) + return err } - defaultValue = v - fv.Field(i).SetBool(defaultValue) + fv.Field(i).Set(reflect.ValueOf(val)) case "deprecated": if hasRest { return fmt.Errorf("cannot have a value for deprecated tag") @@ -83,41 +81,67 @@ func Parse[T any](flags *T, env string) error { // even when the previous env var is empty. continue } - name, valueStr, ok := strings.Cut(elem, "=") - // "somename" is short for "somename=true" or "somename=1". - value := true - if ok { - v, err := strconv.ParseBool(valueStr) + name, valueStr, hasValue := strings.Cut(elem, "=") + + index, knownFlag := indexByName[name] + if !knownFlag { + // Unknown option, proceed processing options as long as the format is valid. + errs = append(errs, fmt.Errorf("unknown flag %q", elem)) + continue + } + field := fv.Field(index) + var val any + if hasValue { + var err error + val, err = parseValue(name, field.Kind(), valueStr) if err != nil { - // Invalid format, return an error immediately. - return errInvalid{ - fmt.Errorf("invalid bool value for %s: %v", name, err), - } + errs = append(errs, err) + continue } - value = v - } - index, ok := indexByName[name] - if !ok { - // Unknown option, proceed processing options as long as the format - // is valid. - errs = append(errs, fmt.Errorf("unknown flag %q", elem)) + } else if field.Kind() == reflect.Bool { + // For bools, "somename" is short for "somename=true" or "somename=1". + // This mimicks how Go flags work, e.g. -knob is short for -knob=true. + val = true + } else { + // For any other type, a value must be specified. + // This mimicks how Go flags work, e.g. -output=path does not allow -output. + errs = append(errs, fmt.Errorf("value needed for %s flag %q", field.Kind(), name)) continue } + if deprecated[name] { - // We allow setting deprecated flags to their default value so - // that bold explorers will not be penalised for their - // experimentation. - if fv.Field(index).Bool() != value { + // We allow setting deprecated flags to their default value so that + // bold explorers will not be penalised for their experimentation. + if field.Interface() != val { errs = append(errs, fmt.Errorf("cannot change default value of deprecated flag %q", name)) } continue } - fv.Field(index).SetBool(value) + field.Set(reflect.ValueOf(val)) } return errors.Join(errs...) } +func parseValue(name string, kind reflect.Kind, str string) (any, error) { + var val any + var err error + switch kind { + case reflect.Bool: + val, err = strconv.ParseBool(str) + case reflect.Int: + val, err = strconv.Atoi(str) + case reflect.String: + val = str + default: + return nil, errInvalid{fmt.Errorf("unsupported kind %s", kind)} + } + if err != nil { + return nil, errInvalid{fmt.Errorf("invalid %s value for %s: %v", kind, name, err)} + } + return val, nil +} + // An ErrInvalid indicates a malformed input string. var ErrInvalid = errors.New("invalid value") diff --git a/internal/envflag/flag_test.go b/internal/envflag/flag_test.go index f326ec727e9..5bd232177db 100644 --- a/internal/envflag/flag_test.go +++ b/internal/envflag/flag_test.go @@ -14,6 +14,11 @@ type testFlags struct { DefaultTrue bool `envflag:"default:true"` } +type testTypes struct { + StringDefaultFoo string `envflag:"default:foo"` + IntDefault5 int `envflag:"default:5"` +} + type deprecatedFlags struct { Foo bool `envflag:"deprecated"` Bar bool `envflag:"deprecated,default:true"` @@ -123,9 +128,44 @@ var tests = []struct { DefaultTrue: true, }, "cannot parse TEST_VAR: unknown flag \"other1\"\nunknown flag \"other2\""), }, { - testName: "Invalid", + testName: "InvalidIntForBool", envVal: "foo=2,BarBaz=true", test: invalid(testFlags{DefaultTrue: true}), +}, { + testName: "StringValue", + envVal: "stringdefaultfoo=bar", + test: success(testTypes{ + StringDefaultFoo: "bar", + IntDefault5: 5, + }), +}, { + testName: "StringEmpty", + envVal: "stringdefaultfoo=", + test: success(testTypes{ + StringDefaultFoo: "", + IntDefault5: 5, + }), +}, { + testName: "FailureStringAlone", + envVal: "stringdefaultfoo", + test: failure(testTypes{ + StringDefaultFoo: "foo", + IntDefault5: 5, + }, "cannot parse TEST_VAR: value needed for string flag \"stringdefaultfoo\""), +}, { + testName: "IntValue", + envVal: "intdefault5=123", + test: success(testTypes{ + StringDefaultFoo: "foo", + IntDefault5: 123, + }), +}, { + testName: "IntEmpty", + envVal: "intdefault5=", + test: invalid(testTypes{ + StringDefaultFoo: "foo", + IntDefault5: 5, + }), }, { testName: "DeprecatedWithFalseDefault", envVal: "foo=1",