PO to GMP Migration Tool: Orchestrator & CLI Boilerplate#1961
PO to GMP Migration Tool: Orchestrator & CLI Boilerplate#1961karthunni wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new command-line tool gmp-migrate and a supporting migrate package to facilitate the migration of Prometheus Operator configurations to Google Managed Prometheus (GMP). The core logic parses Kubernetes manifests, caches them for cross-resource resolution, and runs registered converters to output translated manifests. The review feedback focuses on improving the tool's robustness and testability. Key recommendations include: ensuring deterministic resource conversion by sorting map keys, preventing file collisions by incorporating namespaces into output filenames, decoupling standard input (Stdin) for better testability, using filepath.WalkDir for more efficient directory traversal, and adding defensive nil checks within the ResourceCache methods.
| for kind, nsMap := range m.cache.resources { | ||
| converter, registered := m.converters[kind] | ||
| if !registered { | ||
| continue | ||
| } | ||
|
|
||
| for _, res := range nsMap { |
There was a problem hiding this comment.
Since map iteration in Go is randomized, iterating over m.cache.resources directly makes the resource conversion order non-deterministic. This results in unpredictable log ordering and output generation. Sort the kinds and resource keys alphabetically to ensure deterministic execution.
kinds := make([]string, 0, len(m.cache.resources))
for kind := range m.cache.resources {
kinds = append(kinds, kind)
}
sort.Strings(kinds)
for _, kind := range kinds {
converter, registered := m.converters[kind]
if !registered {
continue
}
nsMap := m.cache.resources[kind]
keys := make([]string, 0, len(nsMap))
for key := range nsMap {
keys = append(keys, key)
}
sort.Strings(keys)
for _, key := range keys {
res := nsMap[key]References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
| for _, out := range outputs { | ||
| filename := fmt.Sprintf("%s-%s.yaml", strings.ToLower(out.GetKind()), strings.ToLower(out.GetName())) | ||
| fp := filepath.Join(outputDir, filename) |
There was a problem hiding this comment.
In Kubernetes, namespaced resources (e.g., Service, PodMonitor) can have the same name across different namespaces. Writing them to files using only Kind and Name will cause resources in different namespaces to silently overwrite each other. Include the namespace in the filename for namespaced resources to prevent collisions.
for _, out := range outputs {
var filename string
if ns := out.GetNamespace(); ns != "" {
filename = fmt.Sprintf("%s-%s-%s.yaml", strings.ToLower(out.GetKind()), strings.ToLower(ns), strings.ToLower(out.GetName()))
} else {
filename = fmt.Sprintf("%s-%s.yaml", strings.ToLower(out.GetKind()), strings.ToLower(out.GetName()))
}
fp := filepath.Join(outputDir, filename)References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
| import ( | ||
| "bufio" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| k8syaml "k8s.io/apimachinery/pkg/util/yaml" | ||
| "sigs.k8s.io/yaml" | ||
| ) |
There was a problem hiding this comment.
To support deterministic sorting of resources during conversion, import the standard sort package.
| import ( | |
| "bufio" | |
| "fmt" | |
| "io" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | |
| k8syaml "k8s.io/apimachinery/pkg/util/yaml" | |
| "sigs.k8s.io/yaml" | |
| ) | |
| import ( | |
| "bufio" | |
| "fmt" | |
| "io" | |
| "os" | |
| "path/filepath" | |
| "sort" | |
| "strings" | |
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | |
| k8syaml "k8s.io/apimachinery/pkg/util/yaml" | |
| "sigs.k8s.io/yaml" | |
| ) |
References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
| // Decoupled output streams (defaults to os.Stdout/os.Stderr) | ||
| Stdout io.Writer | ||
| Stderr io.Writer | ||
| } |
There was a problem hiding this comment.
To allow full testability and consistency with Stdout and Stderr, decouple the input stream by adding a Stdin field to the Migrator struct.
| // Decoupled output streams (defaults to os.Stdout/os.Stderr) | |
| Stdout io.Writer | |
| Stderr io.Writer | |
| } | |
| // Decoupled streams (defaults to os.Stdin/os.Stdout/os.Stderr) | |
| Stdin io.Reader | |
| Stdout io.Writer | |
| Stderr io.Writer | |
| } |
References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
| func NewMigrator() *Migrator { | ||
| return &Migrator{ | ||
| converters: make(map[string]ResourceConverter), | ||
| cache: NewResourceCache(), | ||
| Stdout: os.Stdout, | ||
| Stderr: os.Stderr, | ||
| } |
There was a problem hiding this comment.
Initialize the new Stdin field to os.Stdin by default in NewMigrator.
| func NewMigrator() *Migrator { | |
| return &Migrator{ | |
| converters: make(map[string]ResourceConverter), | |
| cache: NewResourceCache(), | |
| Stdout: os.Stdout, | |
| Stderr: os.Stderr, | |
| } | |
| func NewMigrator() *Migrator { | |
| return &Migrator{ | |
| converters: make(map[string]ResourceConverter), | |
| cache: NewResourceCache(), | |
| Stdin: os.Stdin, | |
| Stdout: os.Stdout, | |
| Stderr: os.Stderr, | |
| } | |
| } |
References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
| if path == "-" { | ||
| return m.parseYAMLStream(os.Stdin) | ||
| } |
There was a problem hiding this comment.
Use the decoupled m.Stdin stream instead of hardcoding os.Stdin when reading from standard input.
| if path == "-" { | |
| return m.parseYAMLStream(os.Stdin) | |
| } | |
| if path == "-" { | |
| return m.parseYAMLStream(m.Stdin) | |
| } |
References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
| return filepath.Walk(path, func(fp string, fi os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if fi.IsDir() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Use filepath.WalkDir instead of filepath.Walk for directory traversal. filepath.WalkDir is significantly more efficient because it avoids calling os.Lstat on every file and directory.
| return filepath.Walk(path, func(fp string, fi os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if fi.IsDir() { | |
| return nil | |
| } | |
| return filepath.WalkDir(path, func(fp string, d os.DirEntry, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if d.IsDir() { | |
| return nil | |
| } |
References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
| // Add adds a resource to the cache. | ||
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | ||
| kind := u.GetKind() | ||
| if _, ok := c.resources[kind]; !ok { | ||
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | ||
| } |
There was a problem hiding this comment.
Add a defensive check to prevent nil pointer dereference if u is nil. Also, lazily initialize c.resources if the cache was instantiated directly without using NewResourceCache.
| // Add adds a resource to the cache. | |
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | |
| kind := u.GetKind() | |
| if _, ok := c.resources[kind]; !ok { | |
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | |
| } | |
| // Add adds a resource to the cache. | |
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | |
| if u == nil { | |
| return | |
| } | |
| if c.resources == nil { | |
| c.resources = make(map[string]map[string]*unstructured.Unstructured) | |
| } | |
| kind := u.GetKind() | |
| if _, ok := c.resources[kind]; !ok { | |
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | |
| } |
References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
There was a problem hiding this comment.
I think we'd rather panic in this case, we should never be adding nil, and also are expected to use the constructor
| // Get retrieves a resource from the cache by kind, namespace, and name. | ||
| func (c *ResourceCache) Get(kind, namespace, name string) (*unstructured.Unstructured, bool) { | ||
| nsMap, ok := c.resources[kind] | ||
| if !ok { | ||
| return nil, false | ||
| } |
There was a problem hiding this comment.
Add a defensive check to prevent a nil pointer dereference or panic if c.resources is nil (e.g., if the cache was instantiated directly).
| // Get retrieves a resource from the cache by kind, namespace, and name. | |
| func (c *ResourceCache) Get(kind, namespace, name string) (*unstructured.Unstructured, bool) { | |
| nsMap, ok := c.resources[kind] | |
| if !ok { | |
| return nil, false | |
| } | |
| // Get retrieves a resource from the cache by kind, namespace, and name. | |
| func (c *ResourceCache) Get(kind, namespace, name string) (*unstructured.Unstructured, bool) { | |
| if c.resources == nil { | |
| return nil, false | |
| } | |
| nsMap, ok := c.resources[kind] | |
| if !ok { | |
| return nil, false | |
| } |
References
- Adhering to the repository style guide by proposing code changes using the GitHub suggestion block format. (link)
There was a problem hiding this comment.
I think we'd rather panic in this case, we are expected to use the constructor
karthunni
force-pushed
the
karthunni/podmonitor-migration
branch
from
9857444 to
4976085
Compare
karthunni
force-pushed
the
karthunni/podmonitor-migration
branch
from
1de300d to
036f215
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new migration tool, gmp-migrate, designed to translate Prometheus Operator configurations to Google Managed Prometheus (GMP). It includes a Dockerfile, a CLI entry point, core migration orchestration logic, and a resource caching mechanism. The review feedback highlights several critical improvements to enhance the tool's robustness and safety: ensuring the CLI exits with a non-zero code when individual resource migrations fail, making file extension checks case-insensitive, skipping nameless resources to avoid cache collisions, deep-copying cached resources before conversion to prevent unintended mutations, and adding nil-safety checks to the resource cache.
| migrator := migrate.NewMigrator() | ||
| _, err := migrator.Run(*inputFile) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "[ERROR] Migration failed: %v\n", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
If some resources fail to migrate, report.FailedCount is incremented, but migrator.Run still returns a nil error. As a result, the CLI exits with code 0. This is unsafe for CI/CD automation pipelines, which rely on non-zero exit codes to detect failures. The CLI should check report.FailedCount > 0 and exit with a non-zero code.
| migrator := migrate.NewMigrator() | |
| _, err := migrator.Run(*inputFile) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "[ERROR] Migration failed: %v\n", err) | |
| os.Exit(1) | |
| } | |
| migrator := migrate.NewMigrator() | |
| report, err := migrator.Run(*inputFile) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "[ERROR] Migration failed: %v\n", err) | |
| os.Exit(1) | |
| } | |
| if report.FailedCount > 0 { | |
| fmt.Fprintf(os.Stderr, "[ERROR] Migration completed with %d failed resource(s)\n", report.FailedCount) | |
| os.Exit(1) | |
| } |
migrator := migrate.NewMigrator()
report, err := migrator.Run(*inputFile)
if err != nil {
fmt.Fprintf(os.Stderr, "[ERROR] Migration failed: %v\n", err)
os.Exit(1)
}
if report.FailedCount > 0 {
fmt.Fprintf(os.Stderr, "[ERROR] Migration completed with %d failed resource(s)\n", report.FailedCount)
os.Exit(1)
}References
- When proposing a code change, always use the GitHub 'suggestion' Markdown block. (link)
| if d.IsDir() { | ||
| return nil | ||
| } | ||
| if strings.HasSuffix(fp, ".yaml") || strings.HasSuffix(fp, ".yml") { |
There was a problem hiding this comment.
The file extension check is currently case-sensitive. If a user has files with uppercase extensions (e.g., .YAML or .Yml), they will be silently skipped during the migration. Using filepath.Ext and strings.ToLower makes the file discovery much more robust.
| if strings.HasSuffix(fp, ".yaml") || strings.HasSuffix(fp, ".yml") { | |
| ext := strings.ToLower(filepath.Ext(fp)) | |
| if ext == ".yaml" || ext == ".yml" { |
ext := strings.ToLower(filepath.Ext(fp))
if ext == ".yaml" || ext == ".yml" {References
- When proposing a code change, always use the GitHub 'suggestion' Markdown block. (link)
| if u.Object == nil || u.GetKind() == "" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The parser currently checks for u.GetKind() == "" but does not check for u.GetName() == "". If an invalid or incomplete resource without a name is parsed, it will be added to the cache and will overwrite other nameless resources under the key "default/". We should skip resources that do not have a name.
| if u.Object == nil || u.GetKind() == "" { | |
| continue | |
| } | |
| if u.Object == nil || u.GetKind() == "" || u.GetName() == "" { | |
| continue | |
| } |
| if u.Object == nil || u.GetKind() == "" { | |
| continue | |
| } | |
| if u.Object == nil || u.GetKind() == "" || u.GetName() == "" { | |
| continue | |
| } |
References
- When proposing a code change, always use the GitHub 'suggestion' Markdown block. (link)
| res := nsMap[key] | ||
| outputs, logs, err := converter.Convert(res, m.cache) |
There was a problem hiding this comment.
The cached resource res is passed directly to converter.Convert(res, m.cache). Since unstructured.Unstructured is a pointer type containing nested maps, any modification made by the converter will mutate the cached resource. This can lead to non-deterministic behavior or bugs if other converters look up the same resource later. Passing a deep copy of the resource ensures the cache remains immutable and safe.
| res := nsMap[key] | |
| outputs, logs, err := converter.Convert(res, m.cache) | |
| res := nsMap[key].DeepCopy() | |
| outputs, logs, err := converter.Convert(res, m.cache) |
| res := nsMap[key] | |
| outputs, logs, err := converter.Convert(res, m.cache) | |
| res := nsMap[key].DeepCopy() | |
| outputs, logs, err := converter.Convert(res, m.cache) |
References
- When proposing a code change, always use the GitHub 'suggestion' Markdown block. (link)
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | ||
| kind := u.GetKind() | ||
| if _, ok := c.resources[kind]; !ok { | ||
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | ||
| } |
There was a problem hiding this comment.
To prevent potential nil pointer dereferences and map assignment panics, we should add a nil check for u and lazily initialize c.resources if the ResourceCache was instantiated as a zero value (e.g., var cache ResourceCache).
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | |
| kind := u.GetKind() | |
| if _, ok := c.resources[kind]; !ok { | |
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | |
| } | |
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | |
| if u == nil { | |
| return | |
| } | |
| if c.resources == nil { | |
| c.resources = make(map[string]map[string]*unstructured.Unstructured) | |
| } | |
| kind := u.GetKind() | |
| if _, ok := c.resources[kind]; !ok { | |
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | |
| } |
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | |
| kind := u.GetKind() | |
| if _, ok := c.resources[kind]; !ok { | |
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | |
| } | |
| func (c *ResourceCache) Add(u *unstructured.Unstructured) { | |
| if u == nil { | |
| return | |
| } | |
| if c.resources == nil { | |
| c.resources = make(map[string]map[string]*unstructured.Unstructured) | |
| } | |
| kind := u.GetKind() | |
| if _, ok := c.resources[kind]; !ok { | |
| c.resources[kind] = make(map[string]*unstructured.Unstructured) | |
| } |
References
- When proposing a code change, always use the GitHub 'suggestion' Markdown block. (link)
1 participant
This PR implements the migration pipeline and CLI carriage boilerplate for the
gmp-migratetool.LogMessage,MigrationReport) routed toStderrto keepStdoutclean for Unix piping.