From a128c1d90eaa27bd6b0a8e3f0684f4f106098cbb Mon Sep 17 00:00:00 2001 From: whg517 Date: Thu, 25 Jul 2024 18:19:49 +0800 Subject: [PATCH] feat: remove builder.RoleGroupInfo to avoid confusion with reconciler (#89) * feat: remove builder.RoleGroupInfo to avoid confusion with reconciler * feat: make Builder.name to public --- pkg/builder/config.go | 2 +- pkg/builder/deployment.go | 2 +- pkg/builder/deployment_test.go | 33 ++++++++++++------------ pkg/builder/interfaces.go | 4 +-- pkg/builder/job.go | 4 +-- pkg/builder/options.go | 17 +++---------- pkg/builder/rbac.go | 10 ++++---- pkg/builder/resource.go | 40 ++++++++++++++++++------------ pkg/builder/service.go | 2 +- pkg/builder/statefulset.go | 2 +- pkg/builder/workload.go | 30 +++++++++------------- pkg/reconciler/deployment_test.go | 2 +- pkg/reconciler/role_test.go | 12 ++++----- pkg/reconciler/statefulset_test.go | 2 +- 14 files changed, 77 insertions(+), 85 deletions(-) diff --git a/pkg/builder/config.go b/pkg/builder/config.go index 8040265..6b087cc 100644 --- a/pkg/builder/config.go +++ b/pkg/builder/config.go @@ -35,7 +35,7 @@ func NewBaseConfigBuilder( return &BaseConfigBuilder{ BaseResourceBuilder: BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: labels, annotations: annotations, }, diff --git a/pkg/builder/deployment.go b/pkg/builder/deployment.go index 11a164e..d662780 100644 --- a/pkg/builder/deployment.go +++ b/pkg/builder/deployment.go @@ -24,7 +24,7 @@ func NewDeployment( name string, replicas *int32, image *util.Image, - options *WorkloadOptions, + options WorkloadOptions, ) *Deployment { return &Deployment{ BaseWorkloadReplicasBuilder: *NewBaseWorkloadReplicasBuilder( diff --git a/pkg/builder/deployment_test.go b/pkg/builder/deployment_test.go index d5db00f..fcb6771 100644 --- a/pkg/builder/deployment_test.go +++ b/pkg/builder/deployment_test.go @@ -63,18 +63,19 @@ var _ = Describe("DeploymentBuilder test", func() { "sample-trinocluster-default", &[]int32{1}[0], util.NewImage("trino", "485", "1.0.0"), - &builder.WorkloadOptions{ - Labels: map[string]string{ - util.AppKubernetesInstanceName: ownerName, - util.AppKubernetesManagedByName: "trino.zncdata.dev", - util.AppKubernetesComponentName: "coordinator", - util.AppKubernetesNameName: "TrinoCluster", - util.AppKubernetesRoleGroupName: "default", - }, - RoleGroupInfo: &builder.RoleGroupInfo{ + builder.WorkloadOptions{ + Options: builder.Options{ RoleName: "coordinator", RoleGroupName: "default", + Labels: map[string]string{ + util.AppKubernetesInstanceName: ownerName, + util.AppKubernetesManagedByName: "trino.zncdata.dev", + util.AppKubernetesComponentName: "coordinator", + util.AppKubernetesNameName: "TrinoCluster", + util.AppKubernetesRoleGroupName: "default", + }, }, + Resource: &commonsv1alpha1.ResourcesSpec{ CPU: &commonsv1alpha1.CPUResource{ Max: resource.MustParse("100m"), @@ -151,7 +152,11 @@ var _ = Describe("DeploymentBuilder test", func() { "sample-trinocluster-default", &[]int32{1}[0], util.NewImage("trino", "485", "1.0.0"), - &builder.WorkloadOptions{ + builder.WorkloadOptions{ + Options: builder.Options{ + RoleName: "coordinator", // EnvOverrides and CommandOverrides will only applied to the container, which it name eq RoleName + RoleGroupName: "default", + }, CommandOverrides: []string{ "bin/launcher", "start", @@ -160,10 +165,6 @@ var _ = Describe("DeploymentBuilder test", func() { "foo": "test", "bar": "test", }, - RoleGroupInfo: &builder.RoleGroupInfo{ - RoleName: "coordinator", // EnvOverrides and CommandOverrides will only applied to the container, which it name eq RoleName - RoleGroupName: "default", - }, }, ), } @@ -210,7 +211,7 @@ var _ = Describe("DeploymentBuilder test", func() { "sample-trinocluster-default", &[]int32{1}[0], util.NewImage("trino", "485", "1.0.0"), - &builder.WorkloadOptions{ + builder.WorkloadOptions{ EnvOverrides: map[string]string{ "foo": "test", }, @@ -221,7 +222,7 @@ var _ = Describe("DeploymentBuilder test", func() { }, }, }, - RoleGroupInfo: &builder.RoleGroupInfo{ + Options: builder.Options{ RoleName: "coordinator", // EnvOverrides will only applied to the container, which it name eq RoleName RoleGroupName: "default", }, diff --git a/pkg/builder/interfaces.go b/pkg/builder/interfaces.go index 68ad32b..119ac82 100644 --- a/pkg/builder/interfaces.go +++ b/pkg/builder/interfaces.go @@ -95,7 +95,7 @@ type WorkloadVolumes interface { } type WorkloadAffinity interface { - AddAffinity(affinity *corev1.Affinity) + SetAffinity(affinity *corev1.Affinity) GetAffinity() *corev1.Affinity } @@ -105,7 +105,7 @@ type WorkloadReplicas interface { } type WorkloadTerminationGracePeriodSeconds interface { - AddTerminationGracePeriod(duration *time.Duration) + SetTerminationGracePeriod(duration *time.Duration) GetTerminationGracePeriod() *time.Duration } diff --git a/pkg/builder/job.go b/pkg/builder/job.go index f9c2bcd..f10c8e8 100644 --- a/pkg/builder/job.go +++ b/pkg/builder/job.go @@ -20,9 +20,9 @@ type Job struct { func NewGenericJobBuilder( client *resourceClient.Client, - name string, + name string, // this is resource name when creating image *util.Image, - options *WorkloadOptions, + options WorkloadOptions, ) JobBuilder { return &Job{ BaseWorkloadBuilder: *NewBaseWorkloadBuilder( diff --git a/pkg/builder/options.go b/pkg/builder/options.go index cf8b36d..a9eb862 100644 --- a/pkg/builder/options.go +++ b/pkg/builder/options.go @@ -8,31 +8,22 @@ import ( commonsv1alpha1 "github.com/zncdatadev/operator-go/pkg/apis/commons/v1alpha1" ) -type RoleGroupInfo struct { +type Options struct { ClusterName string RoleName string RoleGroupName string -} - -func (i *RoleGroupInfo) String() string { - return i.ClusterName + "-" + i.RoleName + "-" + i.RoleGroupName -} - -type ResourceOptions struct { Labels map[string]string Annotations map[string]string - RoleGroupInfo *RoleGroupInfo } type WorkloadOptions struct { - Labels map[string]string - Annotations map[string]string + Options + Affinity *corev1.Affinity PodOverrides *corev1.PodTemplateSpec EnvOverrides map[string]string CommandOverrides []string TerminationGracePeriod *time.Duration // Workload cpu and memory resource limits and requests - Resource *commonsv1alpha1.ResourcesSpec - RoleGroupInfo *RoleGroupInfo + Resource *commonsv1alpha1.ResourcesSpec } diff --git a/pkg/builder/rbac.go b/pkg/builder/rbac.go index 74ea4e1..c81ea30 100644 --- a/pkg/builder/rbac.go +++ b/pkg/builder/rbac.go @@ -26,7 +26,7 @@ func NewGenericServiceAccountBuilder( return &GenericServiceAccountBuilder{ BaseResourceBuilder: BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: labels, annotations: annotations, }, @@ -63,7 +63,7 @@ func NewGenericRoleBuilder( return &GenericRoleBuilder{ BaseResourceBuilder: BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: labels, annotations: annotations, }, @@ -100,7 +100,7 @@ func NewGenericRoleBindingBuilder( return &GenericRoleBindingBuilder{ BaseResourceBuilder: BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: labels, annotations: annotations, }, @@ -137,7 +137,7 @@ func NewGenericClusterRoleBuilder( return &GenericClusterRoleBuilder{ BaseResourceBuilder: BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: labels, annotations: annotations, }, @@ -176,7 +176,7 @@ func NewGenericClusterRoleBindingBuilder( return &GenericClusterRoleBindingBuilder{ BaseResourceBuilder: BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: labels, annotations: annotations, }, diff --git a/pkg/builder/resource.go b/pkg/builder/resource.go index 054be26..fd1ecef 100644 --- a/pkg/builder/resource.go +++ b/pkg/builder/resource.go @@ -19,23 +19,28 @@ var _ ResourceBuilder = &BaseResourceBuilder{} type BaseResourceBuilder struct { Client *client.Client - name string + Name string // this is resource name when creating labels map[string]string annotations map[string]string - roleGroupInfo *RoleGroupInfo + clusterName string + roleName string + roleGroupName string } func NewBaseResourceBuilder( client *client.Client, - name string, - options *ResourceOptions, + name string, // this is resource name when creating + options *Options, ) *BaseResourceBuilder { return &BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: options.Labels, - roleGroupInfo: options.RoleGroupInfo, + annotations: options.Annotations, + clusterName: options.ClusterName, + roleName: options.RoleName, + roleGroupName: options.RoleGroupName, } } @@ -44,11 +49,11 @@ func (b *BaseResourceBuilder) GetClient() *client.Client { } func (b *BaseResourceBuilder) SetName(name string) { - b.name = name + b.Name = name } func (b *BaseResourceBuilder) GetName() string { - return b.name + return b.Name } func (b *BaseResourceBuilder) AddLabels(labels map[string]string) { @@ -67,20 +72,23 @@ func (b *BaseResourceBuilder) GetLabels() map[string]string { util.AppKubernetesManagedByName: util.StackDomain, } - if b.roleGroupInfo != nil { - if b.roleGroupInfo.RoleName != "" { - b.labels[util.AppKubernetesComponentName] = b.roleGroupInfo.RoleName - } - if b.roleGroupInfo.RoleGroupName != "" { - b.labels[util.AppKubernetesRoleGroupName] = b.roleGroupInfo.RoleGroupName - } + if b.clusterName != "" { + b.labels[util.AppKubernetesInstanceName] = b.clusterName + } + + if b.roleName != "" { + b.labels[util.AppKubernetesComponentName] = b.roleName + } + + if b.roleGroupName != "" { + b.labels[util.AppKubernetesRoleGroupName] = b.roleGroupName } } + return b.labels } func (o *BaseResourceBuilder) filterLabels(labels map[string]string) map[string]string { - matchingLabels := make(map[string]string) for _, label := range util.AppMatchingLabelsNames { if value, ok := labels[label]; ok { diff --git a/pkg/builder/service.go b/pkg/builder/service.go index 552c9cd..11ecf8d 100644 --- a/pkg/builder/service.go +++ b/pkg/builder/service.go @@ -79,7 +79,7 @@ func NewServiceBuilder( return &BaseServiceBuilder{ BaseResourceBuilder: BaseResourceBuilder{ Client: client, - name: name, + Name: name, labels: labels, }, ports: servicePorts, diff --git a/pkg/builder/statefulset.go b/pkg/builder/statefulset.go index 9d345f3..e8284ac 100644 --- a/pkg/builder/statefulset.go +++ b/pkg/builder/statefulset.go @@ -23,7 +23,7 @@ func NewStatefulSetBuilder( name string, replicas *int32, image *util.Image, - options *WorkloadOptions, + options WorkloadOptions, ) *StatefulSet { return &StatefulSet{ BaseWorkloadReplicasBuilder: *NewBaseWorkloadReplicasBuilder( diff --git a/pkg/builder/workload.go b/pkg/builder/workload.go index 3241061..2748e54 100644 --- a/pkg/builder/workload.go +++ b/pkg/builder/workload.go @@ -54,22 +54,16 @@ type BaseWorkloadBuilder struct { func NewBaseWorkloadBuilder( client *client.Client, - name string, + name string, // this is resource name when creating image *util.Image, - options *WorkloadOptions, + options WorkloadOptions, ) *BaseWorkloadBuilder { - resourceOptions := &ResourceOptions{ - Labels: options.Labels, - Annotations: options.Annotations, - RoleGroupInfo: options.RoleGroupInfo, - } - return &BaseWorkloadBuilder{ BaseResourceBuilder: *NewBaseResourceBuilder( client, name, - resourceOptions, + &options.Options, ), image: image, @@ -138,18 +132,18 @@ func (b *BaseWorkloadBuilder) GetSecurityContext() *corev1.PodSecurityContext { func (b *BaseWorkloadBuilder) OverrideCommand() { containers := b.GetContainers() - if len(containers) == 0 || b.commandOverrides == nil || len(b.commandOverrides) == 0 || b.roleGroupInfo == nil || b.roleGroupInfo.RoleName == "" { + if len(containers) == 0 || b.commandOverrides == nil || len(b.commandOverrides) == 0 || b.roleName == "" { containersName := []string{} for _, container := range containers { containersName = append(containersName, container.Name) } - logger.V(5).Info("Sikpping command override", "containers", containersName, "commandOverrides", b.commandOverrides, "roleGroupInfo", b.roleGroupInfo) + logger.V(5).Info("Sikpping command override", "containers", containersName, "commandOverrides", b.commandOverrides, "roleName", b.roleName) return } for i := range containers { container := &containers[i] - if container.Name == b.roleGroupInfo.RoleName { + if container.Name == b.roleName { // Override the command, clear the args container.Command = b.commandOverrides container.Args = []string{} @@ -164,18 +158,18 @@ func (b *BaseWorkloadBuilder) OverrideCommand() { func (b *BaseWorkloadBuilder) OverrideEnv() { containers := b.GetContainers() - if len(containers) == 0 || b.envOverrides == nil || len(b.envOverrides) == 0 || b.roleGroupInfo == nil || b.roleGroupInfo.RoleName == "" { + if len(containers) == 0 || b.envOverrides == nil || len(b.envOverrides) == 0 || b.roleName == "" { containersName := []string{} for _, container := range containers { containersName = append(containersName, container.Name) } - logger.V(5).Info("Sikpping env override", "containers", containersName, "envOverrides", b.envOverrides, "roleGroupInfo", b.roleGroupInfo) + logger.V(5).Info("Sikpping env override", "containers", containersName, "envOverrides", b.envOverrides, "roleName", b.roleName) return } for i := range containers { container := &containers[i] - if container.Name == b.roleGroupInfo.RoleName { + if container.Name == b.roleName { // Override the env for key, value := range b.envOverrides { container.Env = append(container.Env, corev1.EnvVar{ @@ -223,7 +217,7 @@ func (b *BaseWorkloadBuilder) GetVolumes() []corev1.Volume { return b.volumes } -func (b *BaseWorkloadBuilder) AddAffinity(affinity *corev1.Affinity) { +func (b *BaseWorkloadBuilder) SetAffinity(affinity *corev1.Affinity) { b.affinity = affinity } @@ -231,7 +225,7 @@ func (b *BaseWorkloadBuilder) GetAffinity() *corev1.Affinity { return b.affinity } -func (b *BaseWorkloadBuilder) AddTerminationGracePeriod(duration *time.Duration) { +func (b *BaseWorkloadBuilder) SetTerminationGracePeriod(duration *time.Duration) { b.terminationGracePeriod = duration } @@ -336,7 +330,7 @@ func NewBaseWorkloadReplicasBuilder( name string, replicas *int32, image *util.Image, - options *WorkloadOptions, + options WorkloadOptions, ) *BaseWorkloadReplicasBuilder { return &BaseWorkloadReplicasBuilder{ BaseWorkloadBuilder: *NewBaseWorkloadBuilder( diff --git a/pkg/reconciler/deployment_test.go b/pkg/reconciler/deployment_test.go index 6142ca0..166f29b 100644 --- a/pkg/reconciler/deployment_test.go +++ b/pkg/reconciler/deployment_test.go @@ -79,7 +79,7 @@ var _ = Describe("Deloyment reconciler", func() { name, &replcias, util.NewImage("trino", "458", "1.0.0"), - &builder.WorkloadOptions{}, + builder.WorkloadOptions{}, ), } }) diff --git a/pkg/reconciler/role_test.go b/pkg/reconciler/role_test.go index 7c47362..b7b9f8f 100644 --- a/pkg/reconciler/role_test.go +++ b/pkg/reconciler/role_test.go @@ -122,15 +122,13 @@ func (r *RoleReconciler) getResourceWithRoleGroup(_ context.Context, info reconc func (r *RoleReconciler) getDeployment(info reconciler.RoleGroupInfo, roleGroup *TrinoRoleGroupSpec) (reconciler.Reconciler, error) { - options := &builder.WorkloadOptions{ - Labels: info.GetLabels(), - Annotations: info.GetAnnotations(), + options := builder.WorkloadOptions{ + Options: builder.Options{ + Labels: info.GetLabels(), + Annotations: info.GetAnnotations(), + }, EnvOverrides: roleGroup.EnvOverrides, CommandOverrides: roleGroup.CommandOverrides, - RoleGroupInfo: &builder.RoleGroupInfo{ - RoleName: info.RoleName, - RoleGroupName: info.RoleGroupName, - }, } if roleGroup.Config != nil { diff --git a/pkg/reconciler/statefulset_test.go b/pkg/reconciler/statefulset_test.go index 7515415..52414fc 100644 --- a/pkg/reconciler/statefulset_test.go +++ b/pkg/reconciler/statefulset_test.go @@ -81,7 +81,7 @@ var _ = Describe("Statefulset reconciler", func() { ProductVersion: "458", ProductName: "nginx", }, - &builder.WorkloadOptions{}, + builder.WorkloadOptions{}, ), } })