From 0689c451c1740fe7d16478d68164a60d0f1254b9 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Thu, 27 Aug 2026 11:03:18 +0200 Subject: [PATCH] style: tighten comments Trim redundant/restating comments and cut wording in the shouldInjectBarmanSidecar doc-comment without dropping any of the non-obvious reasoning it documents. Signed-off-by: Marco Nenciarini --- internal/cnpgi/instance/identity_test.go | 3 +- internal/cnpgi/operator/config/config.go | 4 +-- internal/cnpgi/operator/lifecycle.go | 35 +++++++++++------------ internal/cnpgi/operator/lifecycle_test.go | 1 - 4 files changed, 18 insertions(+), 25 deletions(-) diff --git a/internal/cnpgi/instance/identity_test.go b/internal/cnpgi/instance/identity_test.go index 621145a..b4ab5bb 100644 --- a/internal/cnpgi/instance/identity_test.go +++ b/internal/cnpgi/instance/identity_test.go @@ -39,8 +39,7 @@ var _ = Describe("IdentityImplementation", func() { serviceTypes = append(serviceTypes, capability.GetService().GetType()) } - // The instance sidecar now runs the phase-0 restore in-process, so it must - // advertise TYPE_RESTORE_JOB alongside the services it already served. + // Runs the phase-0 restore in-process now, hence TYPE_RESTORE_JOB below. Expect(serviceTypes).To(ConsistOf( identity.PluginCapability_Service_TYPE_WAL_SERVICE, identity.PluginCapability_Service_TYPE_BACKUP_SERVICE, diff --git a/internal/cnpgi/operator/config/config.go b/internal/cnpgi/operator/config/config.go index ba596cc..db9ca41 100644 --- a/internal/cnpgi/operator/config/config.go +++ b/internal/cnpgi/operator/config/config.go @@ -102,9 +102,7 @@ func (config *PluginConfiguration) GetReplicaSourceBarmanObjectKey() types.Names } } -// HasAnyBarmanObjectStore returns true if the configuration references at least -// one barman object store, be it for backup/archiving, recovery, or as a -// replica source. +// HasAnyBarmanObjectStore reports whether any barman object store is configured. func (config *PluginConfiguration) HasAnyBarmanObjectStore() bool { return len(config.BarmanObjectName) > 0 || len(config.RecoveryBarmanObjectName) > 0 || diff --git a/internal/cnpgi/operator/lifecycle.go b/internal/cnpgi/operator/lifecycle.go index 04e3c9a..18730fd 100644 --- a/internal/cnpgi/operator/lifecycle.go +++ b/internal/cnpgi/operator/lifecycle.go @@ -325,27 +325,24 @@ func (impl LifecycleImplementation) collectAdditionalInstanceArgs( // shouldInjectBarmanSidecar decides whether an instance pod needs the // plugin-barman-cloud sidecar. // -// A cluster doing backup/archiving or serving as a replica source needs the -// sidecar in every instance pod for as long as the cluster exists, so those -// two cases always inject it. A recovery-only cluster (only -// RecoveryBarmanObjectName set, mirroring what pluginConfiguration.Validate() -// accepts) only ever needs the sidecar for its one-time bootstrap restore, so -// it's gated on cluster.Status.CurrentPrimary instead. +// Backup/archiving and replica-source configs need it for as long as the +// cluster exists, so those always inject it. A recovery-only cluster (only +// RecoveryBarmanObjectName set, mirroring pluginConfiguration.Validate()) +// only needs it for the one-time bootstrap restore, so it's gated on +// cluster.Status.CurrentPrimary instead. // -// CurrentPrimary is set by the instance manager itself, from inside the -// primary pod, only once it's up and has completed its own bootstrap (see -// instance_startup.go in cloudnative-pg) - unlike cluster.Status.Instances / -// IsInitialized(), which flips as soon as the instance's PVC exists, well -// before the pod is even created. Using IsInitialized() here would mean the -// sidecar is never added to the one pod that needs it to perform its restore. +// CurrentPrimary is set by the instance manager itself, from inside the pod, +// only once bootstrap completes (see instance_startup.go in cloudnative-pg). +// cluster.Status.Instances / IsInitialized() looks equivalent but flips as +// soon as the instance's PVC exists, before the pod is even created - using +// it here would mean the sidecar never reaches the pod that needs it. // -// Once CurrentPrimary is set, this makes the operator's own drift-check -// (checkPodSpecIsOutdated) see the running pod's spec as outdated and roll it -// out to drop the sidecar. That's deliberately accepted rather than -// engineered around: it's one deterministic rollout using the same machinery -// the operator already uses for every other pod-spec change (a switchover if -// a replica is available, an in-place restart otherwise), not a new or -// fragile risk. +// Once CurrentPrimary is set, the operator's drift-check +// (checkPodSpecIsOutdated) sees the running pod's spec as outdated and rolls +// it out to drop the sidecar. Accepted deliberately: one deterministic +// rollout via the same machinery used for any other pod-spec change +// (switchover if a replica exists, in-place restart otherwise), not a new +// risk. func shouldInjectBarmanSidecar( cluster *cnpgv1.Cluster, pluginConfiguration *config.PluginConfiguration, diff --git a/internal/cnpgi/operator/lifecycle_test.go b/internal/cnpgi/operator/lifecycle_test.go index 2aedf44..5b3264e 100644 --- a/internal/cnpgi/operator/lifecycle_test.go +++ b/internal/cnpgi/operator/lifecycle_test.go @@ -302,7 +302,6 @@ var _ = Describe("LifecycleImplementation", func() { response, err := reconcileInstancePod(ctx, cluster, request, emptyConfig, sidecarConfiguration{}) Expect(err).NotTo(HaveOccurred()) Expect(response).NotTo(BeNil()) - // An empty patch means the pod was left untouched: no sidecar injected. Expect(response.JsonPatch).To(BeEmpty()) })