From 5ce1615726292b567c66d5715c3f12c62b94e0ad Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Wed, 4 Nov 2026 10:05:27 -0500 Subject: [PATCH] refactor the FromProto methods to return structs Refactors the `FromProto` methods on the Namespace-related structs in some important ways: 1. Use a value receiver and return a struct or pointer to a struct created by reading the supplied proto message. This gets rid of a bunch of nil checks and repetitive code, changing things from this: ```go // FromProto sets the spec from the supplied Temporal Cloud NamespaceSpec. // Project and tags are not part of the Temporal Cloud NamespaceSpec, so they // are left unchanged; [Namespace.FromProto] sets them. func (s *NamespaceSpec) FromProto(in *tcloudnamespacev1.NamespaceSpec) { s.Replicas = ReplicasFromProto(in.GetReplicas()) s.RetentionDays = in.GetRetentionDays() if in.GetMtlsAuth() == nil { s.MTLSAuth = nil } else { s.MTLSAuth = &MTLSAuthSpec{} s.MTLSAuth.FromProto(in.GetMtlsAuth()) } if in.GetApiKeyAuth() == nil { s.APIKeyAuth = nil } else { s.APIKeyAuth = &APIKeyAuthSpec{} s.APIKeyAuth.FromProto(in.GetApiKeyAuth()) } s.SearchAttributes = SearchAttributesFromProto(in.GetSearchAttributes()) if in.GetCodecServer() == nil { s.CodecServer = nil } else { s.CodecServer = &CodecServerSpec{} s.CodecServer.FromProto(in.GetCodecServer()) } s.Lifecycle = LifecycleFromProto(in.GetLifecycle()) if in.GetHighAvailability() == nil { s.HighAvailability = nil } else { s.HighAvailability = &HighAvailabilitySpec{} s.HighAvailability.FromProto(in.GetHighAvailability()) } s.ConnectivityRuleIDs = slices.Clone(in.GetConnectivityRuleIds()) // A capacity spec with no mode set carries nothing the CR can represent, so // it is treated as unset. if c := in.GetCapacitySpec(); c.GetOnDemand() == nil && c.GetProvisioned() == nil { s.Capacity = nil } else { s.Capacity = &CapacitySpec{} s.Capacity.FromProto(c) } if in.GetFairness() == nil { s.Fairness = nil } else { s.Fairness = &FairnessSpec{} s.Fairness.FromProto(in.GetFairness()) } if in.GetEncryptionValidation() == nil { s.EncryptionValidation = nil } else { s.EncryptionValidation = &EncryptionValidationSpec{} s.EncryptionValidation.FromProto(in.GetEncryptionValidation()) } s.Description = in.GetDescription() } ``` to this: ```go // FromProto returns a NamespaceSpec constructed from the supplied Temporal // Cloud NamespaceSpec. func (s NamespaceSpec) FromProto(in *tcloudnamespacev1.NamespaceSpec) NamespaceSpec { if in == nil { return NamespaceSpec{} } out := NamespaceSpec{ Description: in.GetDescription(), Replicas: ReplicasFromProto(in.GetReplicas()), RetentionDays: in.GetRetentionDays(), SearchAttributes: SearchAttributesFromProto(in.GetSearchAttributes()), Lifecycle: LifecycleFromProto(in.GetLifecycle()), ConnectivityRuleIDs: slices.Clone(in.GetConnectivityRuleIds()), } var mtlsAuthSpec MTLSAuthSpec out.MTLSAuth = mtlsAuthSpec.FromProto(in.GetMtlsAuth()) var apiKeyAuthSpec APIKeyAuthSpec out.APIKeyAuth = apiKeyAuthSpec.FromProto(in.GetApiKeyAuth()) var codecSpec CodecServerSpec out.CodecServer = codecSpec.FromProto(in.GetCodecServer()) var haSpec HighAvailabilitySpec out.HighAvailability = haSpec.FromProto(in.GetHighAvailability()) var capSpec CapacitySpec out.Capacity = capSpec.FromProto(in.GetCapacitySpec()) var fairSpec FairnessSpec out.Fairness = fairSpec.FromProto(in.GetFairness()) var encValSpec EncryptionValidationSpec out.EncryptionValidation = encValSpec.FromProto(in.GetEncryptionValidation()) return out } ``` 2. Updates the `Reconciler.ResourceSetLatest` method to construct the `latest` CR from only the returned results from the call to Temporal Cloud Ops `GetNamespace` API method. I'm considering changing the calling interface for `ResourceSetLatest` to *not* mutate the supplied `latest` parameter but rather return an updated CR with the latest read information from the external API call. This will make the `types.Reconciler` interface methods that work with `types.Resource` parameters consistent: none will ever mutate parameters but will instead return a copy of the parameter updated with whatever the method changed about the CR's representation. Signed-off-by: Jay Pipes --- api/cloud/v1alpha1/namespace.go | 314 ++++++++++++++----------- api/cloud/v1alpha1/namespace_test.go | 56 +++-- controller/namespace/reconcile.go | 22 +- controller/namespace/reconcile_test.go | 68 +++--- 4 files changed, 243 insertions(+), 217 deletions(-) diff --git a/api/cloud/v1alpha1/namespace.go b/api/cloud/v1alpha1/namespace.go index 38b6580..7e101d2 100644 --- a/api/cloud/v1alpha1/namespace.go +++ b/api/cloud/v1alpha1/namespace.go @@ -34,21 +34,35 @@ type Namespace struct { Status NamespaceStatus `json:"status,omitempty"` } -// FromProto sets the spec and status from the supplied Temporal Cloud -// Namespace. Project, tags and the status fields live on the outer Namespace -// message rather than its spec, so they are set here. Object metadata is left +// FromProto returns a Namespace from the supplied Temporal Cloud Namespace +// proto message. +// +// Project, tags and the status fields live on the outer Namespace message +// rather than its spec, so they are set here. Object metadata is left // unchanged. -func (n *Namespace) FromProto(in *tcloudnamespacev1.Namespace) { - n.Spec.FromProto(in.GetSpec()) - n.Spec.Project = in.GetProjectId() - n.Spec.Tags = maps.Clone(in.GetTags()) +func (n Namespace) FromProto(in *tcloudnamespacev1.Namespace) *Namespace { + out := Namespace{ + TypeMeta: n.TypeMeta, + ObjectMeta: n.ObjectMeta, + } + + if in == nil { + return &out + } - n.Status.ID = in.GetNamespace() - n.Status.ResourceVersion = in.GetResourceVersion() - n.Status.State.FromProto(in.GetState()) - n.Status.AsyncOperationID = in.GetAsyncOperationId() - n.Status.CreatedTime = transform.TimeFromProto(in.GetCreatedTime()) - n.Status.LastModifiedTime = transform.TimeFromProto(in.GetLastModifiedTime()) + var spec NamespaceSpec + out.Spec = spec.FromProto(in.GetSpec()) + out.Spec.Project = in.GetProjectId() + out.Spec.Tags = maps.Clone(in.GetTags()) + + out.Status.ID = in.GetNamespace() + out.Status.ResourceVersion = in.GetResourceVersion() + out.Status.State.FromProto(in.GetState()) + out.Status.AsyncOperationID = in.GetAsyncOperationId() + out.Status.CreatedTime = transform.TimeFromProto(in.GetCreatedTime()) + out.Status.LastModifiedTime = transform.TimeFromProto(in.GetLastModifiedTime()) + + return &out } // ToProto returns the Temporal Cloud Namespace that represents the spec. It @@ -145,71 +159,44 @@ type NamespaceSpec struct { Description string `json:"description,omitempty"` } -// FromProto sets the spec from the supplied Temporal Cloud NamespaceSpec. -// Project and tags are not part of the Temporal Cloud NamespaceSpec, so they -// are left unchanged; [Namespace.FromProto] sets them. -func (s *NamespaceSpec) FromProto(in *tcloudnamespacev1.NamespaceSpec) { - s.Replicas = ReplicasFromProto(in.GetReplicas()) - s.RetentionDays = in.GetRetentionDays() - - if in.GetMtlsAuth() == nil { - s.MTLSAuth = nil - } else { - s.MTLSAuth = &MTLSAuthSpec{} - s.MTLSAuth.FromProto(in.GetMtlsAuth()) +// FromProto returns a NamespaceSpec constructed from the supplied Temporal +// Cloud NamespaceSpec. +func (s NamespaceSpec) FromProto(in *tcloudnamespacev1.NamespaceSpec) NamespaceSpec { + if in == nil { + return NamespaceSpec{} } - if in.GetApiKeyAuth() == nil { - s.APIKeyAuth = nil - } else { - s.APIKeyAuth = &APIKeyAuthSpec{} - s.APIKeyAuth.FromProto(in.GetApiKeyAuth()) + out := NamespaceSpec{ + Description: in.GetDescription(), + Replicas: ReplicasFromProto(in.GetReplicas()), + RetentionDays: in.GetRetentionDays(), + SearchAttributes: SearchAttributesFromProto(in.GetSearchAttributes()), + Lifecycle: LifecycleFromProto(in.GetLifecycle()), + ConnectivityRuleIDs: slices.Clone(in.GetConnectivityRuleIds()), } - s.SearchAttributes = SearchAttributesFromProto(in.GetSearchAttributes()) + var mtlsAuthSpec MTLSAuthSpec + out.MTLSAuth = mtlsAuthSpec.FromProto(in.GetMtlsAuth()) - if in.GetCodecServer() == nil { - s.CodecServer = nil - } else { - s.CodecServer = &CodecServerSpec{} - s.CodecServer.FromProto(in.GetCodecServer()) - } + var apiKeyAuthSpec APIKeyAuthSpec + out.APIKeyAuth = apiKeyAuthSpec.FromProto(in.GetApiKeyAuth()) - s.Lifecycle = LifecycleFromProto(in.GetLifecycle()) + var codecSpec CodecServerSpec + out.CodecServer = codecSpec.FromProto(in.GetCodecServer()) - if in.GetHighAvailability() == nil { - s.HighAvailability = nil - } else { - s.HighAvailability = &HighAvailabilitySpec{} - s.HighAvailability.FromProto(in.GetHighAvailability()) - } + var haSpec HighAvailabilitySpec + out.HighAvailability = haSpec.FromProto(in.GetHighAvailability()) - s.ConnectivityRuleIDs = slices.Clone(in.GetConnectivityRuleIds()) + var capSpec CapacitySpec + out.Capacity = capSpec.FromProto(in.GetCapacitySpec()) - // A capacity spec with no mode set carries nothing the CR can represent, so - // it is treated as unset. - if c := in.GetCapacitySpec(); c.GetOnDemand() == nil && c.GetProvisioned() == nil { - s.Capacity = nil - } else { - s.Capacity = &CapacitySpec{} - s.Capacity.FromProto(c) - } + var fairSpec FairnessSpec + out.Fairness = fairSpec.FromProto(in.GetFairness()) - if in.GetFairness() == nil { - s.Fairness = nil - } else { - s.Fairness = &FairnessSpec{} - s.Fairness.FromProto(in.GetFairness()) - } - - if in.GetEncryptionValidation() == nil { - s.EncryptionValidation = nil - } else { - s.EncryptionValidation = &EncryptionValidationSpec{} - s.EncryptionValidation.FromProto(in.GetEncryptionValidation()) - } + var encValSpec EncryptionValidationSpec + out.EncryptionValidation = encValSpec.FromProto(in.GetEncryptionValidation()) - s.Description = in.GetDescription() + return out } // toProto returns the Temporal Cloud NamespaceSpec that represents the spec, @@ -311,10 +298,14 @@ type MTLSAuthSpec struct { CertificateFilters []CertificateFilterSpec `json:"certificateFilters,omitempty"` } -// FromProto sets the spec from the supplied Temporal Cloud MtlsAuthSpec. m must -// not be nil; callers allocate it when the proto field is set. -func (m *MTLSAuthSpec) FromProto(in *tcloudnamespacev1.MtlsAuthSpec) { - *m = MTLSAuthSpec{ +// FromProto returns an MTLSAuthSpec constructed from the supplied Temporal +// Cloud MtlsAuthSpec. +func (m MTLSAuthSpec) FromProto(in *tcloudnamespacev1.MtlsAuthSpec) *MTLSAuthSpec { + if in == nil { + return nil + } + + out := MTLSAuthSpec{ Enabled: in.GetEnabled(), // TODO: confirm Temporal Cloud returns raw PEM here, not base64. AcceptedClientCA: string(in.GetAcceptedClientCa()), @@ -322,8 +313,9 @@ func (m *MTLSAuthSpec) FromProto(in *tcloudnamespacev1.MtlsAuthSpec) { for _, f := range in.GetCertificateFilters() { var filter CertificateFilterSpec filter.FromProto(f) - m.CertificateFilters = append(m.CertificateFilters, filter) + out.CertificateFilters = append(out.CertificateFilters, filter) } + return &out } // ToProto returns the Temporal Cloud MtlsAuthSpec that represents the spec. It @@ -396,10 +388,15 @@ type APIKeyAuthSpec struct { Enabled bool `json:"enabled"` } -// FromProto sets the spec from the supplied Temporal Cloud ApiKeyAuthSpec. a -// must not be nil; callers allocate it when the proto field is set. -func (a *APIKeyAuthSpec) FromProto(in *tcloudnamespacev1.ApiKeyAuthSpec) { - *a = APIKeyAuthSpec{Enabled: in.GetEnabled()} +// FromProto returns an APIKeyAuthSpec from the supplied Temporal Cloud ApiKeyAuthSpec. +func (a APIKeyAuthSpec) FromProto(in *tcloudnamespacev1.ApiKeyAuthSpec) *APIKeyAuthSpec { + if in == nil { + return nil + } + + return &APIKeyAuthSpec{ + Enabled: in.GetEnabled(), + } } // ToProto returns the Temporal Cloud ApiKeyAuthSpec that represents the spec. @@ -542,18 +539,22 @@ type CodecServerSpec struct { CustomErrorMessage *CodecServerCustomErrorMessage `json:"customErrorMessage,omitempty"` } -// FromProto sets the spec from the supplied Temporal Cloud CodecServerSpec. c -// must not be nil; callers allocate it when the proto field is set. -func (c *CodecServerSpec) FromProto(in *tcloudnamespacev1.CodecServerSpec) { - *c = CodecServerSpec{ +// FromProto returns a CodecServerSpec from the supplied Temporal Cloud +// CodecServerSpec. +func (c CodecServerSpec) FromProto(in *tcloudnamespacev1.CodecServerSpec) *CodecServerSpec { + if in == nil { + return nil + } + out := CodecServerSpec{ Endpoint: in.GetEndpoint(), PassAccessToken: in.GetPassAccessToken(), IncludeCrossOriginCredentials: in.GetIncludeCrossOriginCredentials(), } - if cem := in.GetCustomErrorMessage(); cem != nil { - c.CustomErrorMessage = &CodecServerCustomErrorMessage{} - c.CustomErrorMessage.FromProto(cem) - } + + var cem CodecServerCustomErrorMessage + out.CustomErrorMessage = cem.FromProto(in.GetCustomErrorMessage()) + + return &out } // ToProto returns the Temporal Cloud CodecServerSpec that represents the spec. @@ -580,17 +581,21 @@ type CodecServerCustomErrorMessage struct { Default *CodecServerErrorMessage `json:"default,omitempty"` } -// FromProto sets the custom error message from the supplied Temporal Cloud -// CodecServerSpec_CustomErrorMessage. c must not be nil; callers allocate it -// when the proto field is set. -func (c *CodecServerCustomErrorMessage) FromProto( +// FromProto returns a CodecServerCustomErrorMessage from the supplied Temporal +// Cloud CodecServerSpec_CustomErrorMessage. +func (c CodecServerCustomErrorMessage) FromProto( in *tcloudnamespacev1.CodecServerSpec_CustomErrorMessage, -) { - *c = CodecServerCustomErrorMessage{} - if def := in.GetDefault(); def != nil { - c.Default = &CodecServerErrorMessage{} - c.Default.FromProto(def) +) *CodecServerCustomErrorMessage { + if in == nil { + return nil } + + out := CodecServerCustomErrorMessage{} + + var def CodecServerErrorMessage + out.Default = def.FromProto(in.GetDefault()) + + return &out } // ToProto returns the Temporal Cloud CodecServerSpec_CustomErrorMessage that @@ -617,13 +622,15 @@ type CodecServerErrorMessage struct { Link string `json:"link,omitempty"` } -// FromProto sets the error message from the supplied Temporal Cloud -// CodecServerSpec_CustomErrorMessage_ErrorMessage. e must not be nil; callers -// allocate it when the proto field is set. -func (e *CodecServerErrorMessage) FromProto( +// FromProto returns a CodecServerErrorMessage constructed from the supplied +// Temporal Cloud CodecServerSpec_CustomErrorMessage_ErrorMessage. +func (e CodecServerErrorMessage) FromProto( in *tcloudnamespacev1.CodecServerSpec_CustomErrorMessage_ErrorMessage, -) { - *e = CodecServerErrorMessage{ +) *CodecServerErrorMessage { + if in == nil { + return nil + } + return &CodecServerErrorMessage{ Message: in.GetMessage(), Link: in.GetLink(), } @@ -694,11 +701,13 @@ type HighAvailabilitySpec struct { DisablePassivePollerForwarding bool `json:"disablePassivePollerForwarding,omitempty"` } -// FromProto sets the spec from the supplied Temporal Cloud -// HighAvailabilitySpec. h must not be nil; callers allocate it when the proto -// field is set. -func (h *HighAvailabilitySpec) FromProto(in *tcloudnamespacev1.HighAvailabilitySpec) { - *h = HighAvailabilitySpec{ +// FromProto returns a HighAvailabilitySpec created from the supplied Temporal +// Cloud HighAvailabilitySpec. +func (h HighAvailabilitySpec) FromProto(in *tcloudnamespacev1.HighAvailabilitySpec) *HighAvailabilitySpec { + if in == nil { + return nil + } + return &HighAvailabilitySpec{ DisableManagedFailover: in.GetDisableManagedFailover(), DisablePassivePollerForwarding: in.GetDisablePassivePollerForwarding(), } @@ -730,18 +739,26 @@ type CapacitySpec struct { Provisioned *ProvisionedCapacity `json:"provisioned,omitempty"` } -// FromProto sets the spec from the supplied Temporal Cloud CapacitySpec. c -// must not be nil; callers allocate it when the proto sets a capacity mode. -func (c *CapacitySpec) FromProto(in *tcloudnamespacev1.CapacitySpec) { - *c = CapacitySpec{} - switch { - case in.GetOnDemand() != nil: - c.OnDemand = &OnDemandCapacity{} - c.OnDemand.FromProto(in.GetOnDemand()) - case in.GetProvisioned() != nil: - c.Provisioned = &ProvisionedCapacity{} - c.Provisioned.FromProto(in.GetProvisioned()) +// FromProto returns a CapacitySpec created from the supplied Temporal Cloud +// CapacitySpec. +func (c CapacitySpec) FromProto(in *tcloudnamespacev1.CapacitySpec) *CapacitySpec { + if in == nil { + return nil + } + // A capacity spec with no mode set carries nothing the CR can represent, so + // it is treated as unset. + if in.GetOnDemand() == nil && in.GetProvisioned() == nil { + return nil } + out := CapacitySpec{} + + var odCap OnDemandCapacity + out.OnDemand = odCap.FromProto(in.GetOnDemand()) + + var provCap ProvisionedCapacity + out.Provisioned = provCap.FromProto(in.GetProvisioned()) + + return &out } // ToProto returns the Temporal Cloud CapacitySpec that represents the spec. It @@ -771,10 +788,13 @@ func (c *CapacitySpec) ToProto() *tcloudnamespacev1.CapacitySpec { // OnDemandCapacity selects the on-demand capacity mode. type OnDemandCapacity struct{} -// FromProto sets the capacity from the supplied Temporal Cloud -// CapacitySpec_OnDemand. The proto has no fields, so there is nothing to copy. -func (o *OnDemandCapacity) FromProto(*tcloudnamespacev1.CapacitySpec_OnDemand) { - *o = OnDemandCapacity{} +// FromProto returns an OnDemandCapacity from the supplied Temporal Cloud +// CapacitySpec_OnDemand. +func (o OnDemandCapacity) FromProto(in *tcloudnamespacev1.CapacitySpec_OnDemand) *OnDemandCapacity { + if in == nil { + return nil + } + return &OnDemandCapacity{} } // ToProto returns the Temporal Cloud CapacitySpec_OnDemand that represents the @@ -794,10 +814,13 @@ type ProvisionedCapacity struct { Value int32 `json:"value"` } -// FromProto sets the capacity from the supplied Temporal Cloud +// FromProto returns the ProvisionedCapacity from the supplied Temporal Cloud // CapacitySpec_Provisioned. -func (p *ProvisionedCapacity) FromProto(in *tcloudnamespacev1.CapacitySpec_Provisioned) { - *p = ProvisionedCapacity{ +func (p ProvisionedCapacity) FromProto(in *tcloudnamespacev1.CapacitySpec_Provisioned) *ProvisionedCapacity { + if in == nil { + return nil + } + return &ProvisionedCapacity{ // The proto carries TRUs as a float64; the CR uses whole TRUs. Value: int32(in.GetValue()), } @@ -820,10 +843,13 @@ type FairnessSpec struct { TaskQueueFairnessEnabled bool `json:"taskQueueFairnessEnabled,omitempty"` } -// FromProto sets the spec from the supplied Temporal Cloud FairnessSpec. f -// must not be nil; callers allocate it when the proto field is set. -func (f *FairnessSpec) FromProto(in *tcloudnamespacev1.FairnessSpec) { - *f = FairnessSpec{ +// FromProto returns a FairnessSpec from the supplied Temporal Cloud +// FairnessSpec. +func (f FairnessSpec) FromProto(in *tcloudnamespacev1.FairnessSpec) *FairnessSpec { + if in == nil { + return nil + } + return &FairnessSpec{ TaskQueueFairnessEnabled: in.GetTaskQueueFairnessEnabled(), } } @@ -867,17 +893,23 @@ type EncryptionValidationSpec struct { InspectFailure bool `json:"inspectFailure,omitempty"` } -// FromProto sets the spec from the supplied Temporal Cloud -// EncryptionValidationSpec. e must not be nil; callers allocate it when the -// proto field is set. -func (e *EncryptionValidationSpec) FromProto(in *tcloudnamespacev1.EncryptionValidationSpec) { - *e = EncryptionValidationSpec{ +// FromProto returns an EncryptionValidationSpec from the supplied Temporal +// Cloud EncryptionValidationSpec. +func (e EncryptionValidationSpec) FromProto(in *tcloudnamespacev1.EncryptionValidationSpec) *EncryptionValidationSpec { + if in == nil { + return nil + } + out := EncryptionValidationSpec{ MetadataKey: in.GetMetadataKey(), MetadataValues: slices.Clone(in.GetMetadataValues()), InspectHeader: in.GetInspectHeader(), InspectFailure: in.GetInspectFailure(), } - e.Mode.FromProto(in.GetMode()) + + var mode EncryptionValidationMode + out.Mode = mode.FromProto(in.GetMode()) + + return &out } // ToProto returns the Temporal Cloud EncryptionValidationSpec that represents @@ -910,22 +942,22 @@ const ( EncryptionValidationModeDeny EncryptionValidationMode = "Deny" ) -// FromProto sets the mode from the supplied Temporal Cloud encryption -// validation mode. Unknown modes keep their proto enum name. -func (m *EncryptionValidationMode) FromProto( +// FromProto returns the EncryptionValidationMode from the supplied Temporal +// Cloud encryption validation mode. +func (m EncryptionValidationMode) FromProto( in tcloudnamespacev1.EncryptionValidationSpec_EncryptionValidationMode, -) { +) EncryptionValidationMode { switch in { case tcloudnamespacev1.EncryptionValidationSpec_ENCRYPTION_VALIDATION_MODE_UNSPECIFIED: - *m = "" + return EncryptionValidationMode("") case tcloudnamespacev1.EncryptionValidationSpec_ENCRYPTION_VALIDATION_MODE_DISABLED: - *m = EncryptionValidationModeDisabled + return EncryptionValidationModeDisabled case tcloudnamespacev1.EncryptionValidationSpec_ENCRYPTION_VALIDATION_MODE_WARN: - *m = EncryptionValidationModeWarn + return EncryptionValidationModeWarn case tcloudnamespacev1.EncryptionValidationSpec_ENCRYPTION_VALIDATION_MODE_DENY: - *m = EncryptionValidationModeDeny + return EncryptionValidationModeDeny default: - *m = EncryptionValidationMode(in.String()) + return EncryptionValidationMode(in.String()) } } diff --git a/api/cloud/v1alpha1/namespace_test.go b/api/cloud/v1alpha1/namespace_test.go index 5163ab0..f5017c3 100644 --- a/api/cloud/v1alpha1/namespace_test.go +++ b/api/cloud/v1alpha1/namespace_test.go @@ -136,22 +136,18 @@ func namespaceWithSpec(spec v1alpha1.NamespaceSpec) *v1alpha1.Namespace { } func TestNamespaceFromProto_Spec(t *testing.T) { - minimal := v1alpha1.NamespaceSpec{ - Replicas: []v1alpha1.ReplicaSpec{{Region: "aws-us-east-1"}}, - RetentionDays: 7, - } cases := []struct { name string in *tcloudnamespacev1.Namespace exp v1alpha1.NamespaceSpec }{ { - name: "nil namespace", + name: "nil proto yields empty spec", in: nil, exp: v1alpha1.NamespaceSpec{}, }, { - name: "nil spec keeps outer fields", + name: "nil proto spec keeps outer fields", in: &tcloudnamespacev1.Namespace{ ProjectId: "proj-1", Tags: map[string]string{"a": "1"}, @@ -170,7 +166,10 @@ func TestNamespaceFromProto_Spec(t *testing.T) { RetentionDays: 7, }, }, - exp: minimal, + exp: v1alpha1.NamespaceSpec{ + Replicas: []v1alpha1.ReplicaSpec{{Region: "aws-us-east-1"}}, + RetentionDays: 7, + }, }, { name: "every field", @@ -180,8 +179,10 @@ func TestNamespaceFromProto_Spec(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(tt *testing.T) { - var got v1alpha1.Namespace - got.FromProto(tc.in) + var ns v1alpha1.Namespace + + got := ns.FromProto(tc.in) + require.Equal(tt, tc.exp, got.Spec) }) } @@ -223,8 +224,9 @@ func TestNamespaceFromProto_Status(t *testing.T) { t.Run(tc.name, func(tt *testing.T) { require := require.New(tt) - got := namespaceWithSpec(v1alpha1.NamespaceSpec{}) - got.FromProto(tc.in) + ns := namespaceWithSpec(v1alpha1.NamespaceSpec{}) + + got := ns.FromProto(tc.in) require.Equal(tc.exp, got.Status.ResourceStatus) // The qualified ID goes to status, never to metadata.name. @@ -237,8 +239,9 @@ func TestNamespaceFromProto_CopiesCollections(t *testing.T) { require := require.New(t) in := fullProtoNamespace() - var got v1alpha1.Namespace - got.FromProto(in) + + var ns v1alpha1.Namespace + got := ns.FromProto(in) in.Tags["team"] = "changed" in.Spec.ConnectivityRuleIds[0] = "changed" @@ -329,9 +332,13 @@ func TestNamespace_RoundTrip(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(tt *testing.T) { - var got v1alpha1.Namespace - got.FromProto(namespaceWithSpec(tc.spec).ToProto()) - require.Equal(tt, tc.spec, got.Spec) + orig := namespaceWithSpec(tc.spec) + tp := orig.ToProto() + + var ns v1alpha1.Namespace + fp := ns.FromProto(tp) + + require.Equal(tt, tc.spec, fp.Spec) }) } } @@ -372,10 +379,9 @@ func TestCapacityFromProto(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(tt *testing.T) { - // Go through v1alpha1.NamespaceSpec, which decides when Capacity is nil. - var got v1alpha1.NamespaceSpec - got.FromProto(&tcloudnamespacev1.NamespaceSpec{CapacitySpec: tc.in}) - require.Equal(tt, tc.exp, got.Capacity) + var spec v1alpha1.CapacitySpec + got := spec.FromProto(tc.in) + require.Equal(tt, tc.exp, got) }) } } @@ -423,12 +429,12 @@ func TestCapacityToProto(t *testing.T) { } func TestCodecServerFromProto_CustomErrorMessageWithoutDefault(t *testing.T) { - var got v1alpha1.CodecServerSpec - got.FromProto(&tcloudnamespacev1.CodecServerSpec{ + var spec v1alpha1.CodecServerSpec + got := spec.FromProto(&tcloudnamespacev1.CodecServerSpec{ Endpoint: "https://codec.example.com", CustomErrorMessage: &tcloudnamespacev1.CodecServerSpec_CustomErrorMessage{}, }) - require.Equal(t, v1alpha1.CodecServerSpec{ + require.Equal(t, &v1alpha1.CodecServerSpec{ Endpoint: "https://codec.example.com", CustomErrorMessage: &v1alpha1.CodecServerCustomErrorMessage{}, }, got) @@ -514,8 +520,8 @@ func TestSearchAttributeTypeConversion(t *testing.T) { func TestEncryptionValidationModeConversion(t *testing.T) { fromProto := func(in tcloudnamespacev1.EncryptionValidationSpec_EncryptionValidationMode) v1alpha1.EncryptionValidationMode { var m v1alpha1.EncryptionValidationMode - m.FromProto(in) - return m + got := m.FromProto(in) + return got } pairs := []struct { diff --git a/controller/namespace/reconcile.go b/controller/namespace/reconcile.go index b5f7101..231df22 100644 --- a/controller/namespace/reconcile.go +++ b/controller/namespace/reconcile.go @@ -149,23 +149,11 @@ func (r *Reconciler) ResourceSetLatest( return nil } - cr.FromProto(tns) - - // cr := latest.(*res).cr - // _ = cr - - // // TODO: call the Temporal Cloud API to fetch Namespace spec and status and - // // mutate the supplied resource to set the spec and status fields - // // accordingly. - // req := tcloudservicev1.GetNamespaceRequest{ - // Namespace: cr.Name, - // } - // resp, err := tcc.GetNamespace(ctx, &req) - // if err != nil { - // return fmt.Errorf("failed getting tcloud namespace: %w", err) - // } - // ns := resp.GetNamespace() - // cr.Spec.Project = ns.GetProjectId() + // Construct a new Namespace from the proto we got back for the Cloud + // resource and mutate the latest types.Resource by dereferencing the cr + // variable to the value of the Namespace returned from FromProto() + fromCloud := cr.FromProto(tns) + *cr = *fromCloud return err } diff --git a/controller/namespace/reconcile_test.go b/controller/namespace/reconcile_test.go index 7abe685..c5a22e4 100644 --- a/controller/namespace/reconcile_test.go +++ b/controller/namespace/reconcile_test.go @@ -110,10 +110,10 @@ func TestReconciler_ResourceSetLatest(t *testing.T) { tnsSynced.Status.ID = tnsID tnsSynced.Status.ResourceVersion = "rv-0" - // cloudNS is the namespace as Temporal Cloud reports it. Its project + // protoOutOfSync is the namespace as Temporal Cloud reports it. Its project // differs from the CR's so we can see that latest is mutated to match // Temporal Cloud. - cloudNS := &tcloudnamespacev1.Namespace{ + protoOutOfSync := &tcloudnamespacev1.Namespace{ Namespace: tnsID, ProjectId: projectOther, Tags: map[string]string{"team": "core"}, @@ -127,36 +127,28 @@ func TestReconciler_ResourceSetLatest(t *testing.T) { AsyncOperationId: "op-1", } - // withLatest returns a copy of the supplied CR as a types.Resource after - // applying mutate, for building the expected latest resource. - withLatest := func( - cr *tcloudv1alpha1.Namespace, - mutate func(*tcloudv1alpha1.Namespace), - ) types.Resource { - ns := cr.DeepCopy() - mutate(ns) - return namespace.NewResource(ns) + nsOutOfSync := tns.DeepCopy() + nsOutOfSync.Spec = tcloudv1alpha1.NamespaceSpec{ + Project: projectOther, + Tags: map[string]string{"team": "core"}, + Replicas: []tcloudv1alpha1.ReplicaSpec{{Region: "aws-us-east-1"}}, + RetentionDays: 30, } - // mappedFromCloud sets the spec and status that cloudNS maps to. - mappedFromCloud := func(ns *tcloudv1alpha1.Namespace) { - ns.Spec = tcloudv1alpha1.NamespaceSpec{ - Project: projectOther, - Tags: map[string]string{"team": "core"}, - Replicas: []tcloudv1alpha1.ReplicaSpec{{Region: "aws-us-east-1"}}, - RetentionDays: 30, - } - ns.Status.ResourceStatus = cloudcommon.ResourceStatus{ + nsOutOfSync.Status = tcloudv1alpha1.NamespaceStatus{ + ResourceStatus: cloudcommon.ResourceStatus{ ID: tnsID, ResourceVersion: "rv-1", State: cloudcommon.ResourceStateActive, AsyncOperationID: "op-1", - } + }, } + // cleared empties the spec and status, which is what ResourceSetLatest // does when the namespace does not exist in Temporal Cloud. - cleared := func(ns *tcloudv1alpha1.Namespace) { - ns.Spec = tcloudv1alpha1.NamespaceSpec{} - ns.Status.ResourceStatus = cloudcommon.ResourceStatus{} + cleared := tns.DeepCopy() + cleared.Spec = tcloudv1alpha1.NamespaceSpec{} + cleared.Status = tcloudv1alpha1.NamespaceStatus{ + ResourceStatus: cloudcommon.ResourceStatus{}, } cases := []struct { @@ -199,11 +191,13 @@ func TestReconciler_ResourceSetLatest(t *testing.T) { name: "latest is mutated from get namespace", serverOpts: []fakeserver.NewOption{ fakeserver.OnGetNamespaceReturn( - &tcloudservicev1.GetNamespaceResponse{Namespace: cloudNS}, nil, + &tcloudservicev1.GetNamespaceResponse{Namespace: protoOutOfSync}, nil, ), }, - res: namespace.NewResource(tnsSynced), - latest: withLatest(tnsSynced, mappedFromCloud), + res: namespace.NewResource(tnsSynced), + latest: func() types.Resource { + return namespace.NewResource(nsOutOfSync) + }(), }, { // The namespace was deleted outside of the operator, so latest is @@ -214,8 +208,10 @@ func TestReconciler_ResourceSetLatest(t *testing.T) { nil, status.Error(codes.NotFound, "namespace not found"), ), }, - res: namespace.NewResource(tnsSynced), - latest: withLatest(tnsSynced, cleared), + res: namespace.NewResource(tnsSynced), + latest: func() types.Resource { + return namespace.NewResource(cleared) + }(), }, { name: "fail if tcloud list namespaces errors", @@ -247,13 +243,15 @@ func TestReconciler_ResourceSetLatest(t *testing.T) { Namespace: "other-ns.acct1", Spec: &tcloudnamespacev1.NamespaceSpec{Name: "other-ns"}, }, - cloudNS, + protoOutOfSync, }, }, nil, ), }, - res: namespace.NewResource(tns), - latest: withLatest(tns, mappedFromCloud), + res: namespace.NewResource(tns), + latest: func() types.Resource { + return namespace.NewResource(nsOutOfSync) + }(), }, { name: "latest is cleared if not found by name", @@ -262,8 +260,10 @@ func TestReconciler_ResourceSetLatest(t *testing.T) { &tcloudservicev1.GetNamespacesResponse{}, nil, ), }, - res: namespace.NewResource(tns), - latest: withLatest(tns, cleared), + res: namespace.NewResource(tns), + latest: func() types.Resource { + return namespace.NewResource(cleared) + }(), }, } for _, tc := range cases {