villebro commented on code in PR #451: URL: https://github.com/apache/superset-kubernetes-operator/pull/451#discussion_r4299287359
########## docs/contributing/development-guidelines.md: ########## @@ -28,7 +28,7 @@ See [Architecture](../architecture/overview.md) for the structural overview (CRD - **Single public CRD**: `Superset` resolves shared spec (top-level + per-component) into parent-owned Kubernetes resources - **6 deployment components + lifecycle tasks**: web server, Celery worker, Celery beat, Flower, websocket, MCP, and task Jobs for seed/migrate/rotate/init -- **3 pure Go packages**: `internal/resolution/` (spec flattening), `internal/config/` (Python rendering), `internal/common/` (shared types) +- **4 pure Go packages**: `internal/resolution/` (spec flattening), `internal/config/` (Python rendering), `internal/common/` (shared types), `internal/schedule/` (cron tick computation) Review Comment: Rather than naming all, I wonder if we should just mention that some are pure Go packages, and list the ones that apply right now, so that when the fifth one is added, this still holds? ########## docs/contributing/development-guidelines.md: ########## @@ -299,7 +299,7 @@ New Deployment/Pod/Container fields go into the template hierarchy: 1. Determine the Kubernetes level: `DeploymentTemplate` (Deployment-level), `PodTemplate` (PodSpec-level), or `ContainerTemplate` (Container-level) 2. Add the field to the appropriate template type in `api/v1alpha1/shared_types.go` -3. Add the merge logic in `internal/resolution/merge.go` (`MergeDeploymentTemplate`, `MergePodTemplate`, or `MergeContainerTemplate`) using the field's natural semantics (scalar → `ResolveOverridableValue`, named slice → `Merge*ByName`, map → `MergeMaps`, unnamed slice → `append`). All merge functions follow the convention `Merge*(topLevel, component[, operatorInjected])`: the top-level value establishes ordering, the component value overrides by name in place, and where applicable (env vars, volume mounts, labels) operator-injected values are applied last so they cannot be overridden by user configuration. Any code consuming resolved ports, env vars, or other merged collections must respect this same ordering. +3. Add the merge logic in `internal/resolution/merge.go` (`MergeDeploymentTemplate`, `MergePodTemplate`, or `MergeContainerTemplate`) using the field's natural semantics (scalar → `ResolveOverridableValue`, named slice → the matching helper such as `MergeEnvVars` or `MergeVolumes`, map → `MergeMaps`, unnamed slice → `append`). The collection merge helpers take their inputs in the order `(topLevel, component[, operatorInjected])`: the top-level value establishes ordering, the component value overrides by name in place, and where applicable (env vars, volume mounts, labels) operator-injected values are applied last so they cannot be overridden by user configuration. Any code consuming resolved ports, env vars, or other merged collections must respect this same ordering. Review Comment: Same here - as the codebase becomes more complex, maybe we should treat these more as general guidelines rather than listing each component explicitly. This may require some refactoring, so we may want to reconsider doing this work as a follow-up.. ########## docs/contributing/development-setup.md: ########## @@ -28,7 +28,7 @@ This guide walks through setting up a local development environment with Kind, P | Docker daemon | 20+ | [Docker Desktop](https://docs.docker.com/get-docker/), [Colima](https://github.com/abiosoft/colima) (`brew install colima`), or [Podman](https://podman.io) | | kubectl | 1.27+ | [kubernetes.io](https://kubernetes.io/docs/tasks/tools/) | | Kind | 0.30+ | `brew install kind` or `go install sigs.k8s.io/kind@latest` | -| Go | 1.26+ | `brew install go` or [go.dev](https://go.dev/dl/) | +| Go | 1.27+ | `brew install go` or [go.dev](https://go.dev/dl/) | Review Comment: Hmm, I wonder if Renovate supports bumping this to match the major.minor.patch golang version we're pinning elsewhere? ########## AGENTS.md: ########## @@ -21,7 +21,7 @@ under the License. ## Project Overview -Kubernetes operator for Apache Superset, built with the Go-based Operator SDK and Go 1.26+. +Kubernetes operator for Apache Superset, built with the Go-based Operator SDK and Go 1.27+. Review Comment: Same ########## docs/contributing/development-guidelines.md: ########## @@ -340,7 +341,7 @@ internal/ | File | Purpose | |------|---------| | `api/v1alpha1/shared_types.go` | ImageSpec, MetastoreSpec, DeploymentTemplate, PodTemplate, ContainerTemplate, FlatComponentSpec | -| `api/v1alpha1/superset_types.go` | Parent SupersetSpec, component specs, InitSpec, CEL validation rules, status | +| `api/v1alpha1/superset_types.go` | Parent SupersetSpec, component specs, LifecycleSpec, CEL validation rules, status | Review Comment: Oof, this was OLD 😬 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
