From 0c8c2339f248fd07a4025423a229309cc34e5059 Mon Sep 17 00:00:00 2001 From: Santhanu Vinod Date: Wed, 18 Jun 2025 01:27:25 +0530 Subject: [PATCH 1/2] cmd: Add bottom-up config.toml.example generator Implements configuration extraction tool to automatically keep config.toml.example in sync with config.go changes. Uses reflectwalk and bottom-up tree construction with synchronized stacks. Selected this approach over AST parsing to avoid complex algorithms and reduce maintenance overhead. Features: - Depth-first traversal with two-stack algorithm - Supports minimal/complete extraction modes via struct tags - Handles nested structs, pointers, slices, arrays, and maps - Comprehensive test coverage for all scenarios Related: https://gitlab.com/gitlab-org/gitaly/-/issues/6730 --- cmd/cfg-example-gen/cfg_extractor.go | 197 ++++++++++++++++++++ cmd/cfg-example-gen/cfg_extractor_test.go | 210 ++++++++++++++++++++++ cmd/cfg-example-gen/main.go | 23 +++ cmd/cfg-example-gen/stack.go | 25 +++ cmd/cfg-example-gen/testdata.go | 67 +++++++ cmd/cfg-example-gen/testhelper_test.go | 11 ++ go.mod | 1 + go.sum | 2 + 8 files changed, 536 insertions(+) create mode 100644 cmd/cfg-example-gen/cfg_extractor.go create mode 100644 cmd/cfg-example-gen/cfg_extractor_test.go create mode 100644 cmd/cfg-example-gen/main.go create mode 100644 cmd/cfg-example-gen/stack.go create mode 100644 cmd/cfg-example-gen/testdata.go create mode 100644 cmd/cfg-example-gen/testhelper_test.go diff --git a/cmd/cfg-example-gen/cfg_extractor.go b/cmd/cfg-example-gen/cfg_extractor.go new file mode 100644 index 0000000000..af763411f6 --- /dev/null +++ b/cmd/cfg-example-gen/cfg_extractor.go @@ -0,0 +1,197 @@ +package main + +import ( + "fmt" + "reflect" + "strings" + + "github.com/mitchellh/reflectwalk" +) + +// MinimalExtractionTag represents the tag for minimal extraction +const MinimalExtractionTag = "minimal" + +type cfgExtractor struct { + isMapKey bool + isMinimal bool + containers stack[any] + keys stack[string] + cfg any +} + +// NewCfgExtractor creates a new configuration extractor +func NewCfgExtractor(cfg any, isMinimal bool) *cfgExtractor { + return &cfgExtractor{ + isMinimal: isMinimal, + cfg: cfg, + containers: stack[any]{ + items: make([]any, 0), + }, + keys: stack[string]{ + items: make([]string, 0), + }, + } +} + +// Run executes the configuration extraction process and returns the extracted data structure. +func (c *cfgExtractor) Run() (any, error) { + err := reflectwalk.Walk(c.cfg, c) + if err != nil { + return nil, err + } + + cfgMap, ok := c.containers.pop() + if !ok { + return nil, fmt.Errorf("missing output container") + } + + return cfgMap, nil +} + +// Enter is called when we traverse deeper into the struct. If we're entering a +// composite type like a map or slice, we also set up an empty container which is +// later populated when we walk the primitives within the type. +func (c *cfgExtractor) Enter(l reflectwalk.Location) error { + c.isMapKey = l == reflectwalk.MapKey + + switch l { + case reflectwalk.Struct, reflectwalk.Map: + c.containers.push(map[string]any{}) + case reflectwalk.Slice, reflectwalk.Array: + c.containers.push([]any{}) + } + + return nil +} + +// Exit is called when we traverse out of the struct. We need to add the current +// container's values to the parent container as the recursion unwinds. +func (c *cfgExtractor) Exit(l reflectwalk.Location) error { + // The first value of the stack is the target struct itself + if c.containers.len() <= 1 { + return nil + } + + switch l { + case reflectwalk.Struct, reflectwalk.Slice, reflectwalk.Array, reflectwalk.Map: + container, ok := c.containers.pop() + if !ok { + return fmt.Errorf("missing container for %s", l) + } + + parentContainer, ok := c.containers.pop() + if !ok { + return fmt.Errorf("missing parent container for %s", l) + } + + switch ct := parentContainer.(type) { + case map[string]any: + key, ok := c.keys.pop() + if !ok { + return fmt.Errorf("missing key to store %s in parent map-container", l) + } + + ct[key] = container + c.containers.push(ct) + case []any: + ct = append(ct, container) + c.containers.push(ct) + default: + return fmt.Errorf("unexpected parent type %T for %s", ct, l) + } + } + + return nil +} + +// Primitive handles map and slice/array values. The container is popped from the stack +// and v is appropriately placed within it. +func (c *cfgExtractor) Primitive(v reflect.Value) error { + // Map keys are handled in MapElem + if c.isMapKey { + return nil + } + + var value any + if v.IsValid() && v.CanInterface() { + value = v.Interface() + } + + container, ok := c.containers.pop() + if !ok { + return fmt.Errorf("missing container for value %s", value) + } + + switch ct := container.(type) { + case map[string]any: + // The map container can house multiple key/value pairs, so we need to + // pop the appropriate key. We would've just pushed this key in MapElem(). + key, ok := c.keys.pop() + if !ok { + return fmt.Errorf("missing key to store %s in map-container", value) + } + + ct[key] = value + c.containers.push(ct) + case []any: + // Slices and arrays are simpler. We can just append to the container. + ct = append(ct, value) + c.containers.push(ct) + default: + return fmt.Errorf("unexpected parent type %T for %s", ct, value) + } + + return nil +} + +func (c *cfgExtractor) StructField(field reflect.StructField, value reflect.Value) error { + if c.isMinimal { + exampleTagValue, ok := field.Tag.Lookup("example") + if !ok { + return reflectwalk.SkipEntry + } + + if exampleTagValue != MinimalExtractionTag { + return fmt.Errorf("example tag must be %q, got %q", MinimalExtractionTag, exampleTagValue) + } + } + + // If the `toml` tag was provided, we need to use that as the key name. + if tagVals, ok := getTagValues(field.Tag, "toml"); ok { + fname := strings.TrimSpace(tagVals[0]) + c.keys.push(fname) + } else { + c.keys.push(field.Name) + } + + return nil +} + +func (c *cfgExtractor) Struct(v reflect.Value) error { + return nil +} + +func (c *cfgExtractor) Map(m reflect.Value) error { + return nil +} + +// MapElem handles each key/value pair within a map. We only use the key here +// as the values are handled in Primitive(). +func (c *cfgExtractor) MapElem(_, k, _ reflect.Value) error { + key, ok := k.Interface().(string) + if !ok { + return fmt.Errorf("unsupported map key type %T: only string keys are supported", k.Interface()) + } + + c.keys.push(key) + return nil +} + +func getTagValues(tag reflect.StructTag, key string) ([]string, bool) { + values, ok := tag.Lookup(key) + if !ok || strings.TrimSpace(values) == "" { + return nil, false + } + + return strings.Split(values, ","), true +} diff --git a/cmd/cfg-example-gen/cfg_extractor_test.go b/cmd/cfg-example-gen/cfg_extractor_test.go new file mode 100644 index 0000000000..595dce824c --- /dev/null +++ b/cmd/cfg-example-gen/cfg_extractor_test.go @@ -0,0 +1,210 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestCfgExtractor(t *testing.T) { + host := "localhost" + testCases := []struct { + name string + cfg Cfg + isMinimal bool + expected map[string]any + }{ + { + name: "complete extraction with empty config", + cfg: Cfg{}, + isMinimal: false, + expected: map[string]any{ + "name": "", + "cfg_port": 0, + "timeout": 0.0, + "Debug": false, + "host": nil, + "tags": []any{}, + "RetryDelays": []any{0, 0, 0}, + "metadata": map[string]any{}, + "credentials": map[string]any{ + "username": "", + "password": "", + }, + "advanced": nil, + "SubConfig": map[string]any{ + "enabled": false, + "Keywords": []any{}, + }, + }, + }, + { + name: "complete extraction with default values", + cfg: Cfg{ + Name: "my-app", + Port: 8080, + Timeout: 15.5, + Debug: true, + Host: &host, + Tags: []string{"alpha", "beta", "release"}, + RetryDelays: [3]int{1, 3, 5}, + Metadata: map[string]string{ + "env": "production", + "version": "1.2.3", + }, + Creds: Credentials{ + Username: "admin", + Password: "s3cr3t", + }, + Advanced: &SubConfig{ + Enabled: true, + Keywords: []string{"performance", "secure", "scalable"}, + }, + SubConfig: SubConfig{ + Enabled: false, + Keywords: []string{"legacy", "deprecated"}, + }, + }, + isMinimal: false, + expected: map[string]any{ + "name": "my-app", + "cfg_port": 8080, + "timeout": 15.5, + "Debug": true, + "host": host, + "tags": []any{"alpha", "beta", "release"}, + "RetryDelays": []any{1, 3, 5}, + "metadata": map[string]any{ + "env": "production", + "version": "1.2.3", + }, + "credentials": map[string]any{ + "username": "admin", + "password": "s3cr3t", + }, + "advanced": map[string]any{ + "enabled": true, + "Keywords": []any{"performance", "secure", "scalable"}, + }, + "SubConfig": map[string]any{ + "enabled": false, + "Keywords": []any{"legacy", "deprecated"}, + }, + }, + }, + { + name: "minimal extraction with default values", + cfg: Cfg{ + Name: "my-app", + Port: 8080, + Timeout: 15.5, + Debug: true, + Host: &host, + Tags: []string{"alpha", "beta", "release"}, + RetryDelays: [3]int{1, 3, 5}, + Metadata: map[string]string{ + "env": "production", + "version": "1.2.3", + }, + Creds: Credentials{ + Username: "admin", + Password: "s3cr3t", + }, + Advanced: &SubConfig{ + Enabled: true, + Keywords: []string{"performance", "secure", "scalable"}, + }, + SubConfig: SubConfig{ + Enabled: false, + Keywords: []string{"legacy", "deprecated"}, + }, + }, + isMinimal: true, + expected: map[string]any{ + "name": "my-app", + "tags": []any{"alpha", "beta", "release"}, + "credentials": map[string]any{ + "username": "admin", + "password": "s3cr3t", + }, + "advanced": map[string]any{}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output, err := NewCfgExtractor(tc.cfg, tc.isMinimal).Run() + require.NoError(t, err) + require.Equal(t, tc.expected, output) + }) + } +} + +func TestCfgExtractor_Errors(t *testing.T) { + tests := []struct { + name string + cfg any + isMinimal bool + expectedErr string + }{ + { + name: "invalid example tag", + cfg: struct { + Name string `example:"invalid"` + }{Name: "test"}, + isMinimal: true, + expectedErr: `example tag must be "minimal", got "invalid"`, + }, + { + name: "unsupported map key type", + cfg: struct { + Data map[int]string `toml:"data"` + }{Data: map[int]string{1: "value"}}, + isMinimal: false, + expectedErr: "unsupported map key type int: only string keys are supported", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewCfgExtractor(tt.cfg, tt.isMinimal).Run() + require.Error(t, err) + require.Contains(t, err.Error(), tt.expectedErr) + }) + } +} + +func TestCfgExtractor_EdgeCases(t *testing.T) { + t.Run("nil pointer to struct", func(t *testing.T) { + cfg := struct { + p *struct { + name string + } + }{ + p: nil, + } + + output, err := NewCfgExtractor(cfg, false).Run() + require.NoError(t, err) + + require.NotNil(t, output) + require.Equal(t, map[string]any{"p": nil}, output) + }) + + t.Run("minimal extraction with nil pointer to struct", func(t *testing.T) { + cfg := struct { + p *struct { + name string `example:"minimal"` + } `example:"minimal"` + }{ + p: nil, + } + + output, err := NewCfgExtractor(cfg, true).Run() + require.NoError(t, err) + + require.NotNil(t, output) + require.Equal(t, map[string]any{"p": nil}, output) + }) +} diff --git a/cmd/cfg-example-gen/main.go b/cmd/cfg-example-gen/main.go new file mode 100644 index 0000000000..f318cc38d1 --- /dev/null +++ b/cmd/cfg-example-gen/main.go @@ -0,0 +1,23 @@ +package main + +import ( + "log" + + "github.com/pelletier/go-toml/v2" +) + +func main() { + cfg := getFakeData() + + cfgMap, err := NewCfgExtractor(cfg, false).Run() + if err != nil { + log.Fatalf("failed to extract configuration: %v", err) + } + + output, err := toml.Marshal(&cfgMap) + if err != nil { + log.Fatalf("failed to marshal configuration to TOML: %v", err) + } + + log.Printf("Output:\n%s", output) +} diff --git a/cmd/cfg-example-gen/stack.go b/cmd/cfg-example-gen/stack.go new file mode 100644 index 0000000000..60c6c48a63 --- /dev/null +++ b/cmd/cfg-example-gen/stack.go @@ -0,0 +1,25 @@ +package main + +type stack[T any] struct { + items []T +} + +func (s *stack[T]) push(item T) { + s.items = append(s.items, item) +} + +func (s *stack[T]) pop() (T, bool) { + if len(s.items) == 0 { + var z T + return z, false + } + + item := s.items[len(s.items)-1] + s.items = s.items[:len(s.items)-1] + + return item, true +} + +func (s *stack[T]) len() int { + return len(s.items) +} diff --git a/cmd/cfg-example-gen/testdata.go b/cmd/cfg-example-gen/testdata.go new file mode 100644 index 0000000000..e13822e8d3 --- /dev/null +++ b/cmd/cfg-example-gen/testdata.go @@ -0,0 +1,67 @@ +package main + +type Credentials struct { + Username string `toml:"username" example:"minimal"` + Password string `toml:"password" example:"minimal"` +} + +type SubConfig struct { + Enabled bool `toml:"enabled"` + Keywords []string +} + +type Cfg struct { + // Primitive types + Name string `toml:"name" example:"minimal"` + Port int `toml:"cfg_port"` + Timeout float64 `toml:"timeout"` + Debug bool + + // Pointers to primitives + Host *string `toml:"host"` + + // Slices and arrays + Tags []string `toml:"tags" example:"minimal"` + RetryDelays [3]int + + // Maps + Metadata map[string]string `toml:"metadata"` + + // Struct + Creds Credentials `toml:"credentials" example:"minimal"` + + // Pointer to struct + Advanced *SubConfig `toml:"advanced" example:"minimal"` + + // Anonymous struct field (embedded) + SubConfig +} + +func getFakeData() Cfg { + host := "localhost" + return Cfg{ + Name: "my-app", + Port: 8080, + Timeout: 15.5, + Debug: true, + Host: &host, + Tags: []string{"alpha", "beta", "release"}, + RetryDelays: [3]int{1, 3, 5}, + Metadata: map[string]string{ + "env": "production", + "version": "1.2.3", + }, + Creds: Credentials{ + Username: "admin", + Password: "s3cr3t", + }, + Advanced: &SubConfig{ + Enabled: true, + Keywords: []string{"performance", "secure", "scalable"}, + }, + SubConfig: SubConfig{ + Enabled: false, + Keywords: []string{"legacy", "deprecated"}, + }, + } +} diff --git a/cmd/cfg-example-gen/testhelper_test.go b/cmd/cfg-example-gen/testhelper_test.go new file mode 100644 index 0000000000..432115dc03 --- /dev/null +++ b/cmd/cfg-example-gen/testhelper_test.go @@ -0,0 +1,11 @@ +package main + +import ( + "testing" + + "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" +) + +func TestMain(m *testing.M) { + testhelper.Run(m) +} diff --git a/go.mod b/go.mod index b4fdfc3a99..c860499e8e 100644 --- a/go.mod +++ b/go.mod @@ -187,6 +187,7 @@ require ( github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect github.com/mattn/go-colorable v0.1.13 // indirect github.com/mattn/go-runewidth v0.0.16 // indirect + github.com/mitchellh/reflectwalk v1.0.2 github.com/moby/sys/userns v0.1.0 // indirect github.com/montanaflynn/stats v0.7.0 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect diff --git a/go.sum b/go.sum index a9d9264389..dd30ea6c4f 100644 --- a/go.sum +++ b/go.sum @@ -529,6 +529,8 @@ github.com/mdlayher/socket v0.4.1 h1:eM9y2/jlbs1M615oshPQOHZzj6R6wMT7bX5NPiQvn2U github.com/mdlayher/socket v0.4.1/go.mod h1:cAqeGjoufqdxWkD7DkpyS+wcefOtmu5OQ8KuoJGIReA= github.com/miekg/dns v1.1.68 h1:jsSRkNozw7G/mnmXULynzMNIsgY2dHC8LO6U6Ij2JEA= github.com/miekg/dns v1.1.68/go.mod h1:fujopn7TB3Pu3JM69XaawiU0wqjpL9/8xGop5UrTPps= +github.com/mitchellh/reflectwalk v1.0.2 h1:G2LzWKi524PWgd3mLHV8Y5k7s6XUvT0Gef6zxSIeXaQ= +github.com/mitchellh/reflectwalk v1.0.2/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= github.com/moby/sys/userns v0.1.0 h1:tVLXkFOxVu9A64/yh59slHVv9ahO9UIev4JZusOLG/g= github.com/moby/sys/userns v0.1.0/go.mod h1:IHUYgu/kao6N8YZlp9Cf444ySSvCmDlmzUcYfDHOl28= github.com/montanaflynn/stats v0.6.3/go.mod h1:wL8QJuTMNUDYhXwkmfOly8iTdp5TEcJFWZD2D7SIkUc= -- GitLab From 966626b2b352d8dc70c70923025053bad5dba274 Mon Sep 17 00:00:00 2001 From: Santhanu Vinod Date: Sat, 2 Aug 2025 20:53:03 +0530 Subject: [PATCH 2/2] cmd: Integrate cfg-example-gen with Gitaly config Use actual Gitaly config structs with empty values to generate config.toml.example files from the real configuration schema. Related: https://gitlab.com/gitlab-org/gitaly/-/issues/6730 --- cmd/cfg-example-gen/cfg_extractor_test.go | 37 +++++++++++++ cmd/cfg-example-gen/main.go | 3 +- cmd/cfg-example-gen/testdata.go | 67 ----------------------- cmd/cfg-example-gen/testhelper_test.go | 2 +- 4 files changed, 40 insertions(+), 69 deletions(-) delete mode 100644 cmd/cfg-example-gen/testdata.go diff --git a/cmd/cfg-example-gen/cfg_extractor_test.go b/cmd/cfg-example-gen/cfg_extractor_test.go index 595dce824c..5aa9adff4d 100644 --- a/cmd/cfg-example-gen/cfg_extractor_test.go +++ b/cmd/cfg-example-gen/cfg_extractor_test.go @@ -6,6 +6,43 @@ import ( "github.com/stretchr/testify/require" ) +type Credentials struct { + Username string `example:"minimal" toml:"username"` + Password string `example:"minimal" toml:"password"` +} + +type SubConfig struct { + Enabled bool `toml:"enabled"` + Keywords []string +} + +type Cfg struct { + // Primitive types + Name string `example:"minimal" toml:"name"` + Debug bool + Port int `toml:"cfg_port"` + Timeout float64 `toml:"timeout"` + + // Slices and arrays + Tags []string `example:"minimal" toml:"tags"` + // Pointers to primitives + Host *string `toml:"host"` + + RetryDelays [3]int + + // Maps + Metadata map[string]string `toml:"metadata"` + + // Struct + Creds Credentials `example:"minimal" toml:"credentials"` + + // Pointer to struct + Advanced *SubConfig `example:"minimal" toml:"advanced"` + + // Anonymous struct field (embedded) + SubConfig +} + func TestCfgExtractor(t *testing.T) { host := "localhost" testCases := []struct { diff --git a/cmd/cfg-example-gen/main.go b/cmd/cfg-example-gen/main.go index f318cc38d1..bb7883f234 100644 --- a/cmd/cfg-example-gen/main.go +++ b/cmd/cfg-example-gen/main.go @@ -4,10 +4,11 @@ import ( "log" "github.com/pelletier/go-toml/v2" + "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/config" ) func main() { - cfg := getFakeData() + cfg := config.Cfg{} cfgMap, err := NewCfgExtractor(cfg, false).Run() if err != nil { diff --git a/cmd/cfg-example-gen/testdata.go b/cmd/cfg-example-gen/testdata.go deleted file mode 100644 index e13822e8d3..0000000000 --- a/cmd/cfg-example-gen/testdata.go +++ /dev/null @@ -1,67 +0,0 @@ -package main - -type Credentials struct { - Username string `toml:"username" example:"minimal"` - Password string `toml:"password" example:"minimal"` -} - -type SubConfig struct { - Enabled bool `toml:"enabled"` - Keywords []string -} - -type Cfg struct { - // Primitive types - Name string `toml:"name" example:"minimal"` - Port int `toml:"cfg_port"` - Timeout float64 `toml:"timeout"` - Debug bool - - // Pointers to primitives - Host *string `toml:"host"` - - // Slices and arrays - Tags []string `toml:"tags" example:"minimal"` - RetryDelays [3]int - - // Maps - Metadata map[string]string `toml:"metadata"` - - // Struct - Creds Credentials `toml:"credentials" example:"minimal"` - - // Pointer to struct - Advanced *SubConfig `toml:"advanced" example:"minimal"` - - // Anonymous struct field (embedded) - SubConfig -} - -func getFakeData() Cfg { - host := "localhost" - return Cfg{ - Name: "my-app", - Port: 8080, - Timeout: 15.5, - Debug: true, - Host: &host, - Tags: []string{"alpha", "beta", "release"}, - RetryDelays: [3]int{1, 3, 5}, - Metadata: map[string]string{ - "env": "production", - "version": "1.2.3", - }, - Creds: Credentials{ - Username: "admin", - Password: "s3cr3t", - }, - Advanced: &SubConfig{ - Enabled: true, - Keywords: []string{"performance", "secure", "scalable"}, - }, - SubConfig: SubConfig{ - Enabled: false, - Keywords: []string{"legacy", "deprecated"}, - }, - } -} diff --git a/cmd/cfg-example-gen/testhelper_test.go b/cmd/cfg-example-gen/testhelper_test.go index 432115dc03..bd6a4580c8 100644 --- a/cmd/cfg-example-gen/testhelper_test.go +++ b/cmd/cfg-example-gen/testhelper_test.go @@ -3,7 +3,7 @@ package main import ( "testing" - "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" + "gitlab.com/gitlab-org/gitaly/v18/internal/testhelper" ) func TestMain(m *testing.M) { -- GitLab