Refactordto #12
Refactordto #12
Conversation
- Create internal/api/v1/dto package with domain-organized DTOs: - common.go: shared structures (pagination, responses, metadata) - actions.go: bulk actions, apply configurations, validation - auth.go: authentication, authorization, user profiles - workloads.go: pods, deployments, jobs, cronjobs, etc. - networking.go: services, ingresses, network policies, Istio - storage.go: PVs, PVCs, storage classes, CSI drivers, snapshots - secrets.go: secrets and ConfigMaps - rbac.go: roles, role bindings, cluster roles, RBAC builder - cluster.go: nodes, namespaces, CRDs, API resources, overview - monitoring.go: metrics, time series, health checks, analytics - events.go: Kubernetes events - Update representative handlers to use centralized DTOs: - handlers_workloads.go: pods list/get using dto.ListResponse, dto.APIResponse, dto.ErrorResponse - handlers_secrets.go: remove duplicate SecretSummary/Detail/CreateRequest/UpdateRequest - handlers_actions_*.go: remove duplicate BulkActionRequest/Response structures - Add comprehensive test coverage for DTO JSON serialization/deserialization - Maintain 100% API compatibility - no breaking changes to HTTP responses - Build and tests pass successfully This consolidation eliminates duplicate struct definitions and provides a single source of truth for API contracts per PR 3 of the refactor plan.
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a comprehensive DTO (Data Transfer Object) refactoring for the API v1 layer. The refactoring centralizes request/response structures into a dedicated dto package to eliminate duplication and provide consistent API interfaces across all handlers.
- Introduces a new
internal/api/v1/dtopackage with domain-organized DTOs (workloads, networking, storage, secrets, RBAC, cluster, monitoring, events, auth, and actions) - Migrates existing inline struct definitions from handler files to the centralized DTO package
- Updates all handler files to use the new DTO package instead of locally defined structs
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/v1/dto/workloads.go | Defines workload-related DTOs (pods, deployments, jobs, etc.) |
| internal/api/v1/dto/storage.go | Defines storage-related DTOs (PVs, PVCs, storage classes, CSI drivers) |
| internal/api/v1/dto/secrets.go | Defines secret and ConfigMap DTOs with CRUD operations |
| internal/api/v1/dto/rbac.go | Defines RBAC-related DTOs (roles, bindings, builder requests) |
| internal/api/v1/dto/networking.go | Defines networking DTOs (services, ingresses, network policies, Istio resources) |
| internal/api/v1/dto/monitoring.go | Defines monitoring and metrics-related DTOs |
| internal/api/v1/dto/events.go | Defines Kubernetes events DTOs |
| internal/api/v1/dto/doc.go | Provides package documentation and usage examples |
| internal/api/v1/dto/common_test.go | Contains unit tests for common DTO structures |
| internal/api/v1/dto/common.go | Defines shared structures like pagination and API responses |
| internal/api/v1/dto/cluster.go | Defines cluster-level DTOs (nodes, namespaces, CRDs, overview) |
| internal/api/v1/dto/auth.go | Defines authentication and authorization DTOs |
| internal/api/v1/dto/actions.go | Defines bulk action and apply configuration DTOs |
| internal/api/handlers_workloads.go | Updates to use centralized DTO package for workload operations |
| internal/api/handlers_secrets.go | Removes local struct definitions and imports from DTO package |
| internal/api/handlers_actions_stubs.go | Updates to use DTO package for bulk action requests |
| internal/api/handlers_actions_pods.go | Removes local struct definitions and uses DTO package |
| internal/api/handlers_actions_common.go | Updates to use DTO package for action validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Name string `json:"name"` | ||
| Driver string `json:"driver"` | ||
| DeletionPolicy string `json:"deletionPolicy"` | ||
| Parameters map[string]interface{} `json:"parameters"` |
There was a problem hiding this comment.
The VolumeSnapshotClassSummary struct has inconsistent field types. The Parameters field uses map[string]interface{} while StorageClassSummary uses map[string]string for the same concept. Consider standardizing the parameter types across similar structures for consistency.
| Parameters map[string]interface{} `json:"parameters"` | |
| Parameters map[string]string `json:"parameters"` |
Copilot uses AI. Check for mistakes.
| RoleRef string `json:"roleRef"` // Frontend expects this field | ||
| Subjects int `json:"subjects"` // Frontend expects 'subjects', not 'subjectCount' | ||
| SubjectsDisplay string `json:"subjectsDisplay"` // Frontend expects this field | ||
| SubjectCount int `json:"subjectCount"` // Keep for backward compatibility |
There was a problem hiding this comment.
The RoleBindingSummary struct contains duplicate fields for the same data (Subjects and SubjectCount). This creates potential for data inconsistency. Consider removing the deprecated field or documenting a migration plan to eliminate this duplication.
Copilot uses AI. Check for mistakes.
| Name string `json:"name"` | ||
| Controller string `json:"controller"` | ||
| IsDefault bool `json:"isDefault"` | ||
| Parameters map[string]interface{} `json:"parameters,omitempty"` |
There was a problem hiding this comment.
The IngressClassSummary struct uses map[string]interface{} for Parameters, which is inconsistent with other similar structures that use map[string]string. This type inconsistency could lead to serialization issues and makes the API less predictable.
| Parameters map[string]interface{} `json:"parameters,omitempty"` | |
| Parameters map[string]string `json:"parameters,omitempty"` |
Copilot uses AI. Check for mistakes.
| // GatewaySummary represents a summary view of an Istio Gateway | ||
| type GatewaySummary struct { | ||
| ID string `json:"id"` | ||
| Name string `json:"name"` | ||
| Namespace string `json:"namespace"` | ||
| Addresses []string `json:"addresses"` | ||
| Ports []map[string]interface{} `json:"ports"` | ||
| Age string `json:"age"` | ||
| CreationTimestamp time.Time `json:"creationTimestamp"` | ||
| Labels map[string]string `json:"labels"` | ||
| Annotations map[string]string `json:"annotations"` |
There was a problem hiding this comment.
The GatewaySummary struct uses []map[string]interface{} for Ports, which lacks type safety. Consider defining a specific struct type for port configurations to improve type safety and API documentation.
| // GatewaySummary represents a summary view of an Istio Gateway | |
| type GatewaySummary struct { | |
| ID string `json:"id"` | |
| Name string `json:"name"` | |
| Namespace string `json:"namespace"` | |
| Addresses []string `json:"addresses"` | |
| Ports []map[string]interface{} `json:"ports"` | |
| Age string `json:"age"` | |
| CreationTimestamp time.Time `json:"creationTimestamp"` | |
| Labels map[string]string `json:"labels"` | |
| Annotations map[string]string `json:"annotations"` | |
| // GatewayPortSummary represents a summary of a port in an Istio Gateway | |
| type GatewayPortSummary struct { | |
| Name string `json:"name"` | |
| Number int `json:"number"` | |
| Protocol string `json:"protocol"` | |
| } | |
| // GatewaySummary represents a summary view of an Istio Gateway | |
| type GatewaySummary struct { | |
| ID string `json:"id"` | |
| Name string `json:"name"` | |
| Namespace string `json:"namespace"` | |
| Addresses []string `json:"addresses"` | |
| Ports []GatewayPortSummary `json:"ports"` | |
| Age string `json:"age"` | |
| CreationTimestamp time.Time `json:"creationTimestamp"` | |
| Labels map[string]string `json:"labels"` | |
| Annotations map[string]string `json:"annotations"` |
Copilot uses AI. Check for mistakes.
1 participant
No description provided.