Refactor secret generator controller

[lburgazzoli]

Description

The PR is meant to:

• make the code easier to understand, reducing complexity caused by nested if/else and error conditions (align happy path to the left)
• remove shadowed error vars to avoid reporting misleading errors
• add some basic unit test for the reconcile loop

How Has This Been Tested?

• This does not change the behavior, the default test suite should not be affected.
• Additional unit test case have been added

Screenshot or short clip

Merge criteria

• You have read the contributors guide.
• Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
• Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

[ykaliuta]

So, you combined 2 changes in the patch: 1) combined err and IsNotFound checks in one condition; 2) changed the Get error variable name.
Due to presents of the first one the diff is unreviewable basically due to change in the indentation level (I had to open both sources separately). It is unrelated unexpected change, draws attention of the intended change, and it is not mentioned in the commit message at all. Hint: if you negate the condition and do early return, you will save one more indentation level. UPD: and it also fixes the original problem.

The second one is the case Bartosz likes to struggle :) But it is also solvable introducing if scope only variable. Matter of taste. Of course, there would be probably no such problem, if the logic was in a separate function.

[lburgazzoli]

The second one is the case Bartosz likes to struggle :) But it is also solvable introducing if scope only variable. Matter of taste. Of course, there would be probably no such problem, if the logic was in a separate function.

Yep, I made this as a draft as I'm still improving it a bit after finding I didn't like it much

[codecov]

Codecov Report

Attention: Patch coverage is 56.60377% with 23 lines in your changes missing coverage. Please review.

Please upload report for BASE (incubation@b91bd29). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...lers/secretgenerator/secretgenerator_controller.go	56.60%	21 Missing and 2 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             incubation    #1311   +/-   ##
=============================================
  Coverage              ?   18.72%           
=============================================
  Files                 ?       30           
  Lines                 ?     3364           
  Branches              ?        0           
=============================================
  Hits                  ?      630           
  Misses                ?     2668           
  Partials              ?       66           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zdtsw]

small comments (cannot do in code....)

• log:
  

  opendatahub-operator/controllers/secretgenerator/secretgenerator_controller.go

  Line 242 in 45a5ee6

  return fmt.Errorf("failed to get DataScienceCluster custom resource data: %w", err)

  etc move to log.Error
• label:
  

  opendatahub-operator/controllers/secretgenerator/secretgenerator_controller.go

  Line 119 in 45a5ee6

  generatedSecret := &corev1.Secret{

  why still need generatedSecret.Labels = foundSecret.Labels

[lburgazzoli]

@zdtsw

• log: I have not changed it

  opendatahub-operator/controllers/secretgenerator/secretgenerator_controller.go

  Line 231 in b91bd29

  return fmt.Errorf("failed to get DataScienceCluster custom resource data: %w", err)

• labels: added in the generateSecret function, no need to add them when looking up the secret

[ykaliuta]

looks good to me, will leave to @zdtsw due to the conversation.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zdtsw

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:

• OWNERS [zdtsw]

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