From b144e1e7112e6f3fc086efabae7b11c6c74d686c Mon Sep 17 00:00:00 2001 From: Joe Kralicky Date: Tue, 26 Sep 2023 09:29:28 -0400 Subject: [PATCH] Improve how field values from nested message literals are rendered as strings for flag set defaults --- internal/codegen/cli/extensions.go | 31 ------- internal/codegen/cli/generator.go | 88 ++++++++++++++++--- .../apis/cortexops/cortexops_cli.pb.go | 2 +- 3 files changed, 77 insertions(+), 44 deletions(-) diff --git a/internal/codegen/cli/extensions.go b/internal/codegen/cli/extensions.go index f44c8faad8..ee53014f9e 100644 --- a/internal/codegen/cli/extensions.go +++ b/internal/codegen/cli/extensions.go @@ -2,14 +2,10 @@ package cli import ( "fmt" - "slices" - "github.com/samber/lo" - "google.golang.org/protobuf/compiler/protogen" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/runtime/protoimpl" - "google.golang.org/protobuf/types/dynamicpb" ) func getExtension[T proto.Message](desc protoreflect.Descriptor, ext *protoimpl.ExtensionInfo) (out T, ok bool) { @@ -65,30 +61,3 @@ func applyOptions(desc protoreflect.Descriptor, out proto.Message) { proto.Merge(out, opts) } } - -func (f *FlagSetOptions) ForEachDefault(fieldMessage *protogen.Message, fn func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool) { - if f.Default == nil { - return - } - dm := dynamicpb.NewMessage(fieldMessage.Desc) - if err := f.Default.UnmarshalTo(dm.Interface()); err != nil { - panic(err) - } - orderedRange(dm, fn) -} - -func orderedRange(dm protoreflect.Message, fn func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool) { - ordered := []lo.Tuple2[protoreflect.FieldDescriptor, protoreflect.Value]{} - dm.Range(func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool { - ordered = append(ordered, lo.T2(fd, v)) - return true - }) - slices.SortFunc(ordered, func(a, b lo.Tuple2[protoreflect.FieldDescriptor, protoreflect.Value]) int { - return int(a.A.Number() - b.A.Number()) - }) - for _, t := range ordered { - if !fn(t.A, t.B) { - return - } - } -} diff --git a/internal/codegen/cli/generator.go b/internal/codegen/cli/generator.go index f4eb4f7fae..ee88f7e775 100644 --- a/internal/codegen/cli/generator.go +++ b/internal/codegen/cli/generator.go @@ -14,8 +14,13 @@ import ( "google.golang.org/genproto/googleapis/api/annotations" "google.golang.org/protobuf/compiler/protogen" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protopath" + "google.golang.org/protobuf/reflect/protorange" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/descriptorpb" + "google.golang.org/protobuf/types/dynamicpb" + "google.golang.org/protobuf/types/known/durationpb" + "google.golang.org/protobuf/types/known/timestamppb" ) func NewGenerator() *Generator { @@ -758,19 +763,78 @@ func (cg *Generator) generateFlagSet(g *protogen.GeneratedFile, message *protoge g.P("fs.AddFlagSet(in.", field.GoName, `.FlagSet(append(prefix,"`, kebabName, `")...))`) } - flagSetOpts.ForEachDefault(field.Message, func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool { - fdOpts := FlagOptions{} - applyOptions(fd, &fdOpts) - if fdOpts.Skip { - return true + if flagSetOpts.Default != nil { + dm := dynamicpb.NewMessage(field.Message.Desc) + if err := flagSetOpts.Default.UnmarshalTo(dm.Interface()); err != nil { + panic(err) } - if flagSetOpts.NoPrefix { - g.P(_flagutil.Ident("SetDefValue"), `(fs, `, _strings.Ident("Join"), `(append(prefix, "`, formatKebab(fd.Name()), `"), "."), `, fmt.Sprintf("%q", v.String()), `)`) - } else { - g.P(_flagutil.Ident("SetDefValue"), `(fs, `, _strings.Ident("Join"), `(append(prefix, "`, kebabName, `", "`, formatKebab(fd.Name()), `"), "."), `, fmt.Sprintf("%q", v.String()), `)`) - } - return true - }) + + protorange.Options{ + Stable: true, + }.Range(dm, func(vs protopath.Values) (retVal error) { + v := vs.Index(-1) + if v.Step.Kind() != protopath.FieldAccessStep { + return nil + } + fd := v.Step.FieldDescriptor() + fdOpts := FlagOptions{} + applyOptions(fd, &fdOpts) + if fdOpts.Skip { + return protorange.Break + } + + var valueStr string + if fd.Kind() == protoreflect.MessageKind && !fd.IsMap() { + switch fd.Message().FullName() { + case "google.protobuf.Timestamp": + dm := v.Value.Message().Interface().(*dynamicpb.Message) + wire, _ := proto.Marshal(dm) + ts := ×tamppb.Timestamp{} + proto.Unmarshal(wire, ts) + valueStr = fmt.Sprintf("%q", ts.AsTime().Format(time.RFC3339)) + + retVal = protorange.Break + case "google.protobuf.Duration": + dm := v.Value.Message().Interface().(*dynamicpb.Message) + wire, _ := proto.Marshal(dm) + dur := &durationpb.Duration{} + proto.Unmarshal(wire, dur) + valueStr = fmt.Sprintf("%q", dur.AsDuration().String()) + + retVal = protorange.Break + default: + // recurse into nested messages + return nil + } + } + + if valueStr == "" { + if fd.IsList() { + strs := []string{} + list := v.Value.List() + for i := 0; i < list.Len(); i++ { + strs = append(strs, fmt.Sprintf("%q", list.Get(i).String())) + } + valueStr = fmt.Sprintf("`[%s]`", strings.Join(strs, ",")) + } else { + valueStr = fmt.Sprintf("%q", v.Value.String()) + } + } + + parts := []string{} + for _, part := range vs.Path[1:] { + parts = append(parts, formatKebab(part.FieldDescriptor().Name())) + } + + if flagSetOpts.NoPrefix { + g.P(_flagutil.Ident("SetDefValue"), `(fs, `, _strings.Ident("Join"), `(append(prefix, "`, strings.Join(parts, "."), `"), "."), `, valueStr, `)`) + } else { + g.P(_flagutil.Ident("SetDefValue"), `(fs, `, _strings.Ident("Join"), `(append(prefix, "`, kebabName, `", "`, strings.Join(parts, "."), `"), "."), `, valueStr, `)`) + } + + return + }, nil) + } } continue } diff --git a/plugins/metrics/apis/cortexops/cortexops_cli.pb.go b/plugins/metrics/apis/cortexops/cortexops_cli.pb.go index f990ee192c..c30028640f 100644 --- a/plugins/metrics/apis/cortexops/cortexops_cli.pb.go +++ b/plugins/metrics/apis/cortexops/cortexops_cli.pb.go @@ -607,7 +607,7 @@ func (in *CortexApplicationConfig) FlagSet(prefix ...string) *pflag.FlagSet { fs.AddFlagSet(in.Limits.FlagSet(append(prefix, "limits")...)) flagutil.SetDefValue(fs, strings.Join(append(prefix, "limits", "ingestion-rate"), "."), "600000") flagutil.SetDefValue(fs, strings.Join(append(prefix, "limits", "ingestion-burst-size"), "."), "1000000") - flagutil.SetDefValue(fs, strings.Join(append(prefix, "limits", "compactor-blocks-retention-period"), "."), "seconds:2592000") + flagutil.SetDefValue(fs, strings.Join(append(prefix, "limits", "compactor-blocks-retention-period"), "."), "720h0m0s") if in.RuntimeConfig == nil { in.RuntimeConfig = &runtimeconfig.RuntimeConfigValues{} }