Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor for tautological condition non-nil not equal to nil #126

Merged
merged 2 commits into from
Oct 10, 2024
Merged

refactor for tautological condition non-nil not equal to nil #126

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54a8cdb
refactor condition
sejima1105 Sep 6, 2024
60bbeb0
add nil check
sejima1105 Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 33 additions & 44 deletions controllers/gatling_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,12 @@ func doNotRequeue(err error) (ctrl.Result, error) {
}

func (r *GatlingReconciler) createVolumesForCR(ctx context.Context, gatling *gatlingv1alpha1.Gatling, namespace string, log logr.Logger) error {
if gatling == nil {
return errors.New("gatling is nil")
}

// Create Simulation Data ConfigMap if defined to create in CR
if &gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
if gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When gatling is nil, the nil pointer access is panicked, so here is the case

if gatling ! = nil && gatling.Spec.TestScenarioSpec.SimulationData && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0

is a good choice here.
The zero value of map is nil, so the nil check of SimulationData is left as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review!

It is indeed a very good idea.
However, I thought that using this approach might change the existing behavior if gatling is nil.
For example, if gatling is nil in the createVolumesForCR function, no processing is performed and error is returned as nil.

To prevent this, I think it would be better to check for nil at the beginning of the function and return an error at that point.
What do you think?

Here's what I have in mind to add at the beginning of the function.

if gatling == nil {
    return errors.New("gatling is nil")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kane8n
Sorry for the delay in making the corrections.
I have addressed it with 60bbeb0.
Please check it.

configMapName := gatling.Name + "-simulations-data"
foundConfigMap := &corev1.ConfigMap{}
if err := r.Get(ctx, client.ObjectKey{Name: configMapName, Namespace: namespace}, foundConfigMap); err != nil {
Expand All @@ -388,7 +392,7 @@ func (r *GatlingReconciler) createVolumesForCR(ctx context.Context, gatling *gat
}
}
// Create Resource Data ConfigMap if defined to create in CR
if &gatling.Spec.TestScenarioSpec.ResourceData != nil && len(gatling.Spec.TestScenarioSpec.ResourceData) > 0 {
if gatling.Spec.TestScenarioSpec.ResourceData != nil && len(gatling.Spec.TestScenarioSpec.ResourceData) > 0 {
configMapName := gatling.Name + "-resources-data"
foundConfigMap := &corev1.ConfigMap{}
if err := r.Get(ctx, client.ObjectKey{Name: configMapName, Namespace: namespace}, foundConfigMap); err != nil {
Expand All @@ -403,7 +407,7 @@ func (r *GatlingReconciler) createVolumesForCR(ctx context.Context, gatling *gat
}
}
// Create GatlingConf ConfigMap if defined to create in CR
if &gatling.Spec.TestScenarioSpec.GatlingConf != nil && len(gatling.Spec.TestScenarioSpec.GatlingConf) > 0 {
if gatling.Spec.TestScenarioSpec.GatlingConf != nil && len(gatling.Spec.TestScenarioSpec.GatlingConf) > 0 {
configMapName := gatling.Name + "-gatling-conf"
foundConfigMap := &corev1.ConfigMap{}
if err := r.Get(ctx, client.ObjectKey{Name: configMapName, Namespace: namespace}, foundConfigMap); err != nil {
Expand All @@ -418,7 +422,7 @@ func (r *GatlingReconciler) createVolumesForCR(ctx context.Context, gatling *gat
}
}
// Create PersistentVolume if defined to create in CR
if &gatling.Spec.PersistentVolumeSpec != nil && gatling.Spec.PersistentVolumeSpec.Name != "" {
if gatling.Spec.PersistentVolumeSpec.Name != "" {
gold-kou marked this conversation as resolved.
Show resolved Hide resolved
volumeName := gatling.Spec.PersistentVolumeSpec.Name
foundVolume := &corev1.PersistentVolume{}
if err := r.Get(ctx, client.ObjectKey{Name: volumeName, Namespace: namespace}, foundVolume); err != nil {
Expand All @@ -433,7 +437,7 @@ func (r *GatlingReconciler) createVolumesForCR(ctx context.Context, gatling *gat
}
}
// Create PersistentVolumeClaim if defined to create in CR
if &gatling.Spec.PersistentVolumeClaimSpec != nil && gatling.Spec.PersistentVolumeClaimSpec.Name != "" {
if gatling.Spec.PersistentVolumeClaimSpec.Name != "" {
claimName := gatling.Spec.PersistentVolumeClaimSpec.Name
foundClaim := &corev1.PersistentVolumeClaim{}
if err := r.Get(ctx, client.ObjectKey{Name: claimName, Namespace: namespace}, foundClaim); err != nil {
Expand Down Expand Up @@ -768,13 +772,13 @@ func (r *GatlingReconciler) newGatlingReporterJobForCR(gatling *gatlingv1alpha1.
// VolumeMounts for Galling Job
func (r *GatlingReconciler) getGatlingRunnerJobVolumeMounts(gatling *gatlingv1alpha1.Gatling) []corev1.VolumeMount {
volumeMounts := make([]corev1.VolumeMount, 0)
if &gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
if gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
volumeMounts = append(volumeMounts, corev1.VolumeMount{
Name: "simulations-data-volume",
MountPath: r.getTempSimulationsDirectoryPath(gatling),
})
}
if &gatling.Spec.TestScenarioSpec.ResourceData != nil && len(gatling.Spec.TestScenarioSpec.ResourceData) > 0 {
if gatling.Spec.TestScenarioSpec.ResourceData != nil && len(gatling.Spec.TestScenarioSpec.ResourceData) > 0 {
volumeMounts = append(volumeMounts, corev1.VolumeMount{
Name: "resources-data-volume",
MountPath: r.getResourcesDirectoryPath(gatling),
Expand All @@ -795,7 +799,7 @@ func (r *GatlingReconciler) getGatlingRunnerJobVolumeMounts(gatling *gatlingv1al
// Volume Source
func (r *GatlingReconciler) getGatlingRunnerJobVolumes(gatling *gatlingv1alpha1.Gatling) []corev1.Volume {
volumes := make([]corev1.Volume, 0)
if &gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
if gatling.Spec.TestScenarioSpec.SimulationData != nil && len(gatling.Spec.TestScenarioSpec.SimulationData) > 0 {
volumes = append(volumes, corev1.Volume{
Name: "simulations-data-volume",
VolumeSource: corev1.VolumeSource{
Expand All @@ -807,7 +811,7 @@ func (r *GatlingReconciler) getGatlingRunnerJobVolumes(gatling *gatlingv1alpha1.
},
})
}
if &gatling.Spec.TestScenarioSpec.ResourceData != nil && len(gatling.Spec.TestScenarioSpec.ResourceData) > 0 {
if gatling.Spec.TestScenarioSpec.ResourceData != nil && len(gatling.Spec.TestScenarioSpec.ResourceData) > 0 {
volumes = append(volumes, corev1.Volume{
Name: "resources-data-volume",
VolumeSource: corev1.VolumeSource{
Expand Down Expand Up @@ -968,127 +972,115 @@ func (r *GatlingReconciler) isGatlingCompleted(gatling *gatlingv1alpha1.Gatling)

func (r *GatlingReconciler) getCloudStorageProvider(gatling *gatlingv1alpha1.Gatling) string {
provider := ""
if &gatling.Spec.CloudStorageSpec != nil && gatling.Spec.CloudStorageSpec.Provider != "" {
if gatling.Spec.CloudStorageSpec.Provider != "" {
provider = gatling.Spec.CloudStorageSpec.Provider
}
return provider
}

func (r *GatlingReconciler) getCloudStorageRegion(gatling *gatlingv1alpha1.Gatling) string {
region := ""
if &gatling.Spec.CloudStorageSpec != nil && gatling.Spec.CloudStorageSpec.Region != "" {
if gatling.Spec.CloudStorageSpec.Region != "" {
region = gatling.Spec.CloudStorageSpec.Region
}
return region
}

func (r *GatlingReconciler) getCloudStorageBucket(gatling *gatlingv1alpha1.Gatling) string {
bucket := ""
if &gatling.Spec.CloudStorageSpec != nil && gatling.Spec.CloudStorageSpec.Bucket != "" {
if gatling.Spec.CloudStorageSpec.Bucket != "" {
bucket = gatling.Spec.CloudStorageSpec.Bucket
}
return bucket
}

func (r *GatlingReconciler) getNotificationServiceProvider(gatling *gatlingv1alpha1.Gatling) string {
provider := defaultNotificationServiceProvider
if &gatling.Spec.NotificationServiceSpec != nil && gatling.Spec.NotificationServiceSpec.Provider != "" {
if gatling.Spec.NotificationServiceSpec.Provider != "" {
provider = gatling.Spec.NotificationServiceSpec.Provider
}
return provider
}

func (r *GatlingReconciler) getNotificationServiceSecretName(gatling *gatlingv1alpha1.Gatling) string {
secretName := ""
if &gatling.Spec.NotificationServiceSpec != nil && gatling.Spec.NotificationServiceSpec.SecretName != "" {
if gatling.Spec.NotificationServiceSpec.SecretName != "" {
secretName = gatling.Spec.NotificationServiceSpec.SecretName
}
return secretName
}

func (r *GatlingReconciler) getGatlingRunnerJobStartTime(gatling *gatlingv1alpha1.Gatling) string {
var startTime string
if &gatling.Spec.TestScenarioSpec != nil && gatling.Spec.TestScenarioSpec.StartTime != "" {
if gatling.Spec.TestScenarioSpec.StartTime != "" {
startTime = gatling.Spec.TestScenarioSpec.StartTime
}
return startTime
}

func (r *GatlingReconciler) getGatlingRunnerJobParallelism(gatling *gatlingv1alpha1.Gatling) *int32 {
var parallelism int32
if &gatling.Spec.TestScenarioSpec != nil && gatling.Spec.TestScenarioSpec.Parallelism != 0 {
if gatling.Spec.TestScenarioSpec.Parallelism != 0 {
parallelism = gatling.Spec.TestScenarioSpec.Parallelism
}
return &parallelism
}

func (r *GatlingReconciler) getGatlingContainerImage(gatling *gatlingv1alpha1.Gatling) string {
image := defaultGatlingImage
if &gatling.Spec.PodSpec != nil && gatling.Spec.PodSpec.GatlingImage != "" {
if gatling.Spec.PodSpec.GatlingImage != "" {
image = gatling.Spec.PodSpec.GatlingImage
}
return image
}

func (r *GatlingReconciler) getRcloneContainerImage(gatling *gatlingv1alpha1.Gatling) string {
image := defaultRcloneImage
if &gatling.Spec.PodSpec != nil && gatling.Spec.PodSpec.RcloneImage != "" {
if gatling.Spec.PodSpec.RcloneImage != "" {
image = gatling.Spec.PodSpec.RcloneImage
}
return image
}

func (r *GatlingReconciler) getPodResources(gatling *gatlingv1alpha1.Gatling) corev1.ResourceRequirements {
resources := corev1.ResourceRequirements{}
gold-kou marked this conversation as resolved.
Show resolved Hide resolved
if &gatling.Spec.PodSpec != nil && &gatling.Spec.PodSpec.Resources != nil {
resources = gatling.Spec.PodSpec.Resources
}
return resources
return gatling.Spec.PodSpec.Resources
}

func (r *GatlingReconciler) getObjectMeta(gatling *gatlingv1alpha1.Gatling) *metav1.ObjectMeta {
objectmeta := metav1.ObjectMeta{}
if &gatling != nil && &gatling.ObjectMeta != nil {
if gatling != nil {
objectmeta = gatling.ObjectMeta
}
return &objectmeta
}

func (r *GatlingReconciler) getPodAffinity(gatling *gatlingv1alpha1.Gatling) *corev1.Affinity {
affinity := corev1.Affinity{}
if &gatling.Spec.PodSpec != nil && &gatling.Spec.PodSpec.Affinity != nil {
affinity = gatling.Spec.PodSpec.Affinity
}
return &affinity
return &gatling.Spec.PodSpec.Affinity
}

func (r *GatlingReconciler) getPodTolerations(gatling *gatlingv1alpha1.Gatling) []corev1.Toleration {
tolerations := []corev1.Toleration{}
if &gatling.Spec.PodSpec != nil && &gatling.Spec.PodSpec.Tolerations != nil {
if gatling.Spec.PodSpec.Tolerations != nil {
tolerations = gatling.Spec.PodSpec.Tolerations
}
return tolerations
}

func (r *GatlingReconciler) getPodServiceAccountName(gatling *gatlingv1alpha1.Gatling) string {
serviceAccountName := ""
if &gatling.Spec.PodSpec != nil && &gatling.Spec.PodSpec.ServiceAccountName != nil {
serviceAccountName = gatling.Spec.PodSpec.ServiceAccountName
}
return serviceAccountName
return gatling.Spec.PodSpec.ServiceAccountName
}

func (r *GatlingReconciler) getSimulationFormat(gatling *gatlingv1alpha1.Gatling) string {
format := defaultSimulationFormat
if &gatling.Spec.TestScenarioSpec != nil && gatling.Spec.TestScenarioSpec.SimulationsFormat != "" {
if gatling.Spec.TestScenarioSpec.SimulationsFormat != "" {
format = gatling.Spec.TestScenarioSpec.SimulationsFormat
}
return format
}

func (r *GatlingReconciler) getSimulationsDirectoryPath(gatling *gatlingv1alpha1.Gatling) string {
path := defaultSimulationsDirectoryPath
if &gatling.Spec.TestScenarioSpec != nil && gatling.Spec.TestScenarioSpec.SimulationsDirectoryPath != "" {
if gatling.Spec.TestScenarioSpec.SimulationsDirectoryPath != "" {
path = gatling.Spec.TestScenarioSpec.SimulationsDirectoryPath
}
return path
Expand All @@ -1100,40 +1092,37 @@ func (r *GatlingReconciler) getTempSimulationsDirectoryPath(gatling *gatlingv1al

func (r *GatlingReconciler) getResourcesDirectoryPath(gatling *gatlingv1alpha1.Gatling) string {
path := defaultResourcesDirectoryPath
if &gatling.Spec.TestScenarioSpec != nil && gatling.Spec.TestScenarioSpec.ResourcesDirectoryPath != "" {
if gatling.Spec.TestScenarioSpec.ResourcesDirectoryPath != "" {
path = gatling.Spec.TestScenarioSpec.ResourcesDirectoryPath
}
return path
}

func (r *GatlingReconciler) getResultsDirectoryPath(gatling *gatlingv1alpha1.Gatling) string {
path := defaultResultsDirectoryPath
if &gatling.Spec.TestScenarioSpec != nil && gatling.Spec.TestScenarioSpec.ResultsDirectoryPath != "" {
if gatling.Spec.TestScenarioSpec.ResultsDirectoryPath != "" {
path = gatling.Spec.TestScenarioSpec.ResultsDirectoryPath
}
return path
}

func (r *GatlingReconciler) getPodSecurityContext(gatling *gatlingv1alpha1.Gatling) *corev1.PodSecurityContext {
securityContext := &corev1.PodSecurityContext{}
if &gatling.Spec.PodSpec != nil && &gatling.Spec.PodSpec.SecurityContext != nil {
if gatling.Spec.PodSpec.SecurityContext != nil {
securityContext = gatling.Spec.PodSpec.SecurityContext
}
return securityContext
}

func (r *GatlingReconciler) getRunnerContainerSecurityContext(gatling *gatlingv1alpha1.Gatling) *corev1.SecurityContext {
securityContext := &corev1.SecurityContext{}
if &gatling.Spec.PodSpec != nil && &gatling.Spec.PodSpec.RunnerContainerSecurityContext != nil {
if gatling.Spec.PodSpec.RunnerContainerSecurityContext != nil {
securityContext = gatling.Spec.PodSpec.RunnerContainerSecurityContext
}
return securityContext
}

func (r *GatlingReconciler) getGenerateLocalReport(gatling *gatlingv1alpha1.Gatling) bool {
if &gatling.Spec.GenerateLocalReport == nil {
return false
}
return gatling.Spec.GenerateLocalReport
}

Expand Down
Loading