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

Jump to bottom

Simplify if else statements #222

Merged
merged 1 commit into from
Aug 10, 2023
Merged

Simplify if else statements #222

merged 1 commit into from
Aug 10, 2023

Conversation

bshephar
Copy link
Collaborator

@bshephar bshephar commented Aug 3, 2023

If we're returning from a if statement, we really don't need an else. These can be simplified and made more legible by simply removing the else and having a seperate if block.

This is really just a quality of life patch. IMO, the use of else can often make code a bit harder to follow and read.

If this is just something we're doing for consistency with the wider org's sake, I'm happy to raise it to the wider group for broader perspective. But at least here, I think it makes things a little nicer to read and follow.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Aug 3, 2023
@bshephar bshephar force-pushed the simply-if-statements branch 2 times, most recently from d89e672 to af17297 Compare August 4, 2023 02:28
@kajinamit
Copy link
Contributor

/test functional

Is this a consistent failure ?

@bshephar
Copy link
Collaborator Author

bshephar commented Aug 5, 2023

Hmm yeah, this is reproducing locally as well actually.

@bshephar
Copy link
Collaborator Author

bshephar commented Aug 5, 2023 •
edited
Loading

Interesting, seems to be this one:
https://github.com/openstack-k8s-operators/heat-operator/compare/af1729757e111269defb133988a2474d9a4f3b7d..1dc91656281bd84f234487ca4a95c6610afeb5ab

	//
	// get admin authentication OpenStack
	//
	os, ctrlResult, err := keystonev1.GetAdminServiceClient(
		ctx,
		helper,
		keystoneAPI,
	)
	fmt.Printf("bshephar-debug: err: %+v\n ctrlResult: %+v \n comp ctrl.Result: %+v\n", err, ctrlResult, ctrl.Result{})
	if err != nil || (ctrlResult != ctrl.Result{}) {
		return ctrl.Result{}, err
	}

There's a diff in ctrlResult vs ctrl.Result{}:

  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}

But not sure just yet as to why it would make a difference in the consolidated if this or this, vs the two seperate if statements.

It's exactly the same except it doesn't go on for as long when I do the same thing with seperate if statements:

  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}

Maybe some timing thing?

@kajinamit
Copy link
Contributor

Interesting, seems to be this one: https://github.com/openstack-k8s-operators/heat-operator/compare/af1729757e111269defb133988a2474d9a4f3b7d..1dc91656281bd84f234487ca4a95c6610afeb5ab

	//
	// get admin authentication OpenStack
	//
	os, ctrlResult, err := keystonev1.GetAdminServiceClient(
		ctx,
		helper,
		keystoneAPI,
	)
	fmt.Printf("bshephar-debug: err: %+v\n ctrlResult: %+v \n comp ctrl.Result: %+v\n", err, ctrlResult, ctrl.Result{})
	if err != nil || (ctrlResult != ctrl.Result{}) {
		return ctrl.Result{}, err
	}

There's a diff in ctrlResult vs ctrl.Result{}:

  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}

But not sure just yet as to why it would make a difference in the consolidated if this or this, vs the two seperate if statements.

It's exactly the same except it doesn't go on for as long when I do the same thing with seperate if statements:

  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}
  bshephar-debug: err: <nil>
   ctrlResult: {Requeue:false RequeueAfter:10s} 
   comp ctrl.Result: {Requeue:false RequeueAfter:0s}

Maybe some timing thing?

It looks you returned ctrl.Result{} instead of ctrlResult and I guess that is the problem.

@bshephar
Copy link
Collaborator Author

bshephar commented Aug 9, 2023

/test heat-operator-build-deploy-kuttl

@bshephar
Simplify if else statements
e0defb1 
If we're returning from a if statement, we really don't need an else.
These can be simplified and made more legible by simply removing the
else and having a seperate if block.

Signed-off-by: Brendan Shephard <bshephar@redhat.com>
kajinamit
kajinamit reviewed Aug 9, 2023
View reviewed changes
controllers/heatcfnapi_controller.go
@@ -336,7 +337,7 @@ func (r *HeatCfnAPIReconciler) reconcileInit(

ksSvcObj := keystonev1.NewKeystoneService(ksSvcSpec, instance.Namespace, serviceLabels, time.Duration(10)*time.Second)
ctrlResult, err = ksSvcObj.CreateOrPatch(ctx, helper)
if err != nil {
if err != nil || (ctrlResult != ctrl.Result{}) {
Copy link
Contributor

@kajinamit kajinamit Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I thought we'd need this logic kept after condition mirror but it turned out this method returns non-emptry ctrlResult only when KeystoneService is not found... so skipping the condition mirror makes more sense.

@bshephar
Copy link
Collaborator Author

bshephar commented Aug 9, 2023

/test heat-operator-build-deploy-kuttl

@kajinamit
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 10, 2023
@openshift-merge-robot openshift-merge-robot merged commit f3690dc into openstack-k8s-operators:main Aug 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kajinamit kajinamit kajinamit left review comments

@Sandeepyadav93 Sandeepyadav93 Awaiting requested review from Sandeepyadav93

Assignees

@kajinamit kajinamit

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bshephar @kajinamit @openshift-merge-robot