Decoupled and organized formatters, updated workload handlers to use … #14
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR decouples workload formatting logic into dedicated domain-specific formatters and updates workload handlers to use the new formatter architecture. This refactoring improves code organization, maintainability, and testability by separating formatting concerns from handler logic.
- Introduces a new
formatterspackage with domain-specific formatter modules for workloads, networking, storage, configuration, RBAC, and cluster resources - Updates workload handlers to use the new
WorkloadsFormatterinstead of embedded formatter methods - Adds comprehensive test coverage for the new formatter functions
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/api/v1/formatters/workloads.go |
Core workloads formatter with pod, deployment, and job formatting functions |
internal/api/v1/formatters/workloads_test.go |
Comprehensive tests for workloads formatter functions |
internal/api/v1/formatters/storage.go |
Storage formatter for PVs, PVCs, and storage classes |
internal/api/v1/formatters/networking.go |
Networking formatter for services, ingresses, and endpoints |
internal/api/v1/formatters/cluster.go |
Cluster formatter for nodes, namespaces, and API resources |
internal/api/v1/formatters/config.go |
Configuration formatter for ConfigMaps and ResourceQuotas |
internal/api/v1/formatters/common.go |
Common formatting utilities shared across formatters |
internal/api/handlers_workloads.go |
Updated workload handlers to use new formatters |
internal/api/handlers_cluster.go |
Updated cluster handlers to use new formatters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| age := "unknown" | ||
| if !sc.CreationTimestamp.IsZero() { | ||
| age = time.Since(sc.CreationTimestamp.Time).String() |
There was a problem hiding this comment.
Inconsistent age calculation method. This should use the calculateAge function like other formatters for consistent formatting.
| age := "unknown" | |
| if !sc.CreationTimestamp.IsZero() { | |
| age = time.Since(sc.CreationTimestamp.Time).String() | |
| age = calculateAge(sc.CreationTimestamp.Time) |
Copilot uses AI. Check for mistakes.
| age := "unknown" | ||
| if !csi.CreationTimestamp.IsZero() { | ||
| age = time.Since(csi.CreationTimestamp.Time).String() |
There was a problem hiding this comment.
Inconsistent age calculation method. This should use the calculateAge function like other formatters for consistent formatting.
| age := "unknown" | |
| if !csi.CreationTimestamp.IsZero() { | |
| age = time.Since(csi.CreationTimestamp.Time).String() | |
| age = calculateAge(csi.CreationTimestamp.Time) |
Copilot uses AI. Check for mistakes.
| age := "unknown" | ||
| if !creationTimestamp.IsZero() { | ||
| age = time.Since(creationTimestamp).String() | ||
| } |
There was a problem hiding this comment.
Inconsistent age calculation method. This should use the calculateAge function like other formatters for consistent formatting.
Copilot uses AI. Check for mistakes.
| // CalculateAge calculates a human-readable age string from a creation time | ||
| func CalculateAge(creationTime time.Time) string { |
There was a problem hiding this comment.
This public function duplicates the private calculateAge function used throughout the formatters. Either use this public version consistently or remove the duplication.
Copilot uses AI. Check for mistakes.
| // Calculate age | ||
| age := "unknown" | ||
| if !namespace.CreationTimestamp.IsZero() { | ||
| age = time.Since(namespace.CreationTimestamp.Time).String() | ||
| } | ||
|
|
||
| // Count labels and annotations | ||
| labelsCount := 0 | ||
| annotationsCount := 0 | ||
| if namespace.Labels != nil { | ||
| labelsCount = len(namespace.Labels) | ||
| } | ||
| if namespace.Annotations != nil { | ||
| annotationsCount = len(namespace.Annotations) | ||
| } | ||
|
|
||
| return map[string]interface{}{ | ||
| "name": namespace.Name, | ||
| "status": string(namespace.Status.Phase), | ||
| "age": age, | ||
| "labelsCount": labelsCount, | ||
| "annotationsCount": annotationsCount, | ||
| "creationTimestamp": namespace.CreationTimestamp.Time, | ||
| "labels": namespace.Labels, | ||
| "annotations": namespace.Annotations, | ||
| } |
There was a problem hiding this comment.
The FormatNamespaceSummary function duplicates logic from the NamespaceToResponse method but uses inconsistent age calculation. Consider removing this duplicate or making it consistent.
| // Calculate age | |
| age := "unknown" | |
| if !namespace.CreationTimestamp.IsZero() { | |
| age = time.Since(namespace.CreationTimestamp.Time).String() | |
| } | |
| // Count labels and annotations | |
| labelsCount := 0 | |
| annotationsCount := 0 | |
| if namespace.Labels != nil { | |
| labelsCount = len(namespace.Labels) | |
| } | |
| if namespace.Annotations != nil { | |
| annotationsCount = len(namespace.Annotations) | |
| } | |
| return map[string]interface{}{ | |
| "name": namespace.Name, | |
| "status": string(namespace.Status.Phase), | |
| "age": age, | |
| "labelsCount": labelsCount, | |
| "annotationsCount": annotationsCount, | |
| "creationTimestamp": namespace.CreationTimestamp.Time, | |
| "labels": namespace.Labels, | |
| "annotations": namespace.Annotations, | |
| } | |
| formatter := NewClusterFormatter() | |
| return formatter.NamespaceToResponse(namespace) |
Copilot uses AI. Check for mistakes.
1 participant
…them