PO to GMP Migration Tool: Refactor Common Conversion Functions

[karthunni]

Refactoring conversion functions to reduce code duplication and simplify implementation for impending ServiceMonitor migration logic.

[gemini-code-assist]

Code Review

This pull request refactors the migration helpers and PodMonitor converter by extracting common logic into shared helper functions and introducing a commonMonitorSpec struct. Feedback is provided to address a bug in resolveFilterRunning where explicit filterRunning: true configurations are silently dropped, and to add defensive nil checks for the spec parameter in buildPodMonitoring and buildClusterPodMonitoring to prevent potential nil pointer dereferences.

[gemini-code-assist]

The current implementation of resolveFilterRunning silently drops explicit filterRunning: true configurations. If all endpoints are configured with filterRunning: true (or a mix of true and nil), the function returns nil, which causes the generated resource to omit the field and potentially default to false in GMP. This can lead to scraping failed or completed pods (like completed K8s Jobs), resulting in noisy logs and irrelevant metrics. We should preserve explicit true settings when no endpoints are set to false.

func resolveFilterRunning(filterRunnings []*bool, logger *slog.Logger) *bool {
	var hasFalse, hasTrue, hasNil bool
	for _, fr := range filterRunnings {
		if fr == nil {
			hasNil = true
		} else if !*fr {
			hasFalse = true
		} else {
			hasTrue = true
		}
	}
	if hasFalse {
		falseVal := false
		if hasTrue || hasNil {
			logger.Warn("Endpoint-level configuration conflict detected: some endpoints are configured with 'filterRunning: false' and others with 'true' (or default), but GMP only supports 'filterRunning' at the resource level. Setting 'filterRunning: false' globally.")
		}
		return &falseVal
	}
	if hasTrue {
		trueVal := true
		return &trueVal
	}
	return nil
}

[karthunni]

GMP operator defaults p.Spec.FilterRunning == nil to true, but we can be explicit if preferred

[karthunni]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the migration helpers and PodMonitor converter to reduce code duplication and improve maintainability. It introduces a shared commonMonitorSpec struct and extracts several helper functions for resolving scrape intervals, timeouts, proxy URLs, authentication, TLS settings, and unsupported fields. These helpers are then utilized to consolidate the conversion logic for both PodMonitoring and ClusterPodMonitoring resources. I have no feedback to provide as the changes are clean and well-structured.

[karthunni]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the migration logic for PodMonitor resources by extracting common conversion logic, validation, and warning routines into helper functions within pkg/migrate/helpers.go. It also introduces a commonMonitorSpec struct to share configurations between namespaced and cluster-scoped resources, significantly reducing code duplication. The reviewer feedback suggests adding a nil check for the gmpEp parameter in the new applyAuthAndTLS helper function to prevent potential nil pointer dereferences.

[karthunni]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the PodMonitor migration logic by modularizing helper functions into helpers.go and introducing a shared commonMonitorSpec struct to streamline the conversion of both PodMonitoring and ClusterPodMonitoring resources. The review feedback focuses on improving resilience during migration; specifically, it suggests refactoring determineNamespaceScoping, resolveScrapeIntervalAndTimeout, and convertProxyURL to log warnings and fall back to safe defaults or placeholders instead of returning fatal errors when encountering non-fatal configuration issues.

[gemini-code-assist]

According to the general rules, non-fatal configuration issues (such as empty selector names or invalid namespace matchNames) should log a warning and fall back to a safe default instead of returning a fatal error. This prevents the entire migration process from failing. We should refactor determineNamespaceScoping to accept a logger and return the default namespace when matchNames contains only empty or invalid values.

Suggested change

	func determineNamespaceScoping(nsSel pomonitoringv1.NamespaceSelector, defaultNS string) ([]string, bool, error) {
	if nsSel.Any {
	return nil, true, nil
	}
	if len(nsSel.MatchNames) > 0 {
	targetNamespaces := parseAndCleanNamespaces(nsSel.MatchNames)
	if len(targetNamespaces) == 0 {
	return nil, false, errors.New("namespaceSelector.matchNames contains only empty or invalid values")
	}
	return targetNamespaces, false, nil
	}
	return []string{defaultNS}, false, nil
	}
	func determineNamespaceScoping(logger *slog.Logger, nsSel pomonitoringv1.NamespaceSelector, defaultNS string) ([]string, bool) {
	if nsSel.Any {
	return nil, true
	}
	if len(nsSel.MatchNames) > 0 {
	targetNamespaces := parseAndCleanNamespaces(nsSel.MatchNames)
	if len(targetNamespaces) == 0 {
	logger.Warn("namespaceSelector.matchNames contains only empty or invalid values. Falling back to default namespace.",
	slog.String("default_namespace", defaultNS))
	return []string{defaultNS}, false
	}
	return targetNamespaces, false
	}
	return []string{defaultNS}, false
	}

References

1. For non-fatal configuration issues (such as missing keys, empty selector names, or missing referenced resources), log a warning and inject a placeholder string instead of returning a fatal error, to prevent the entire migration process from failing.

[karthunni]

Not sure where this "general rule" is coming from, but anything malformed configuration should lead to errors (i.e if migrating would lead to failed scrapes).