chore: remove duplicated code and fix minor bugs

[dejanzele]

Pull Request Template

Description

Please include a summary of the changes and the related issue (if any). Describe your changes in detail to help reviewers understand your contribution.

• What is the purpose of this PR?
• Cleanup of duplicated logic and minor bugfixes
• What was changed?
• Nothing feature-wise
• Why was it changed?
• Improve stability and reusability
• Does this address any existing issues or enhancement requests?
• No

Fixes # (issue)

Type of change

Please select the type of change your PR introduces:

• Bugfix
• Feature
• Code Style Update (formatting, renaming)
• Refactor (code changes that do not fix a bug or add a feature)
• Documentation Update
• Other (please describe):

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions to reproduce these tests. List any relevant details for your test configuration.

• Test Configuration:

  • Kubernetes Version:
  • Helm Version:
  • OS:

• Test Steps:

  1. Step 1
  2. Step 2
  3. ...

Checklist:

• My code follows the style guidelines of this project.
• I have performed a self-review of my code.
• I have commented on my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• Any dependent changes have been merged and published in downstream modules.

[codecov]

Codecov Report

Attention: Patch coverage is 59.68992% with 104 lines in your changes missing coverage. Please review.

Project coverage is 84.67%. Comparing base (d82cf81) to head (e249792).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/controller/install/lookout_controller.go	45.94%	11 Missing and 9 partials ⚠️
...ternal/controller/install/binoculars_controller.go	41.37%	9 Missing and 8 partials ⚠️
internal/controller/install/executor_controller.go	42.85%	8 Missing and 8 partials ⚠️
...nternal/controller/install/scheduler_controller.go	42.85%	6 Missing and 6 partials ⚠️
...rnal/controller/install/armadaserver_controller.go	65.51%	6 Missing and 4 partials ⚠️
internal/controller/install/common_helpers.go	85.93%	6 Missing and 3 partials ⚠️
...controller/install/scheduleringester_controller.go	61.90%	4 Missing and 4 partials ⚠️
...nal/controller/install/eventingester_controller.go	53.84%	3 Missing and 3 partials ⚠️
...l/controller/install/lookoutingester_controller.go	62.50%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #326      +/-   ##
==========================================
+ Coverage   84.30%   84.67%   +0.36%     
==========================================
  Files          15       15              
  Lines        3467     3327     -140     
==========================================
- Hits         2923     2817     -106     
+ Misses        381      353      -28     
+ Partials      163      157       -6     

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

[nikola-jokic]

Left few comments ☺️ LMKWYT

[nikola-jokic]

I would personally prefer to leave the refactored code and structure it slightly differently. The checkandHandleResourceDeletion is misleading, since it also adds the finalizer if one doesn't exist. This helper function is doing too much for something that is easy to read. As a suggestion, I'd prefer the following:

if !server.ObjectMeta.DeletionTimestamp.IsZero() {
	// The object is being deleted
	if !controllerutil.ContainsFinalizer(&as, operatorFinalizer) {
        return ctrl.Result{}, nil
    }
	logger.Info("Running cleanup function for ArmadaServer cluster-scoped components", "finalizer", operatorFinalizer)
    if err := r.deleteExternalResources(ctx, components, logger); err != nil {
		// if fail to delete the external dependency here, return with error
		// so that it can be retried
		return ctrl.Result{}, err
	}

	// remove our finalizer from the list and update it.
	logger.Info("Removing finalizer from ArmadaServer object", "finalizer", operatorFinalizer)
	controllerutil.RemoveFinalizer(&as, operatorFinalizer)
	err = r.Update(ctx, &as)
    return ctrl.Result{}, err
}

if !controllerutil.ContainsFinalizer(&as, operatorFinalizer) {
	logger.Info("Attaching finalizer to As object", "finalizer", operatorFinalizer)
	controllerutil.AddFinalizer(&as, operatorFinalizer)
	if err := r.Update(ctx, &as); err != nil {
		return ctrl.Result{}, err
	}
}

To me, this is much easier to read, you can easily follow the state machine and what is being applied, and the added benefit is when you decide to add another finalizer, the generic cleanup function would have to be refactored to cover that case, which would be more complicated than reading this code.

[dejanzele]

So this is a snippet which is duplicated in 8 contollers with around 99% similarity.

I'd rather have it in a single function which is well-tested and reused in all controllers.

I refactored it a bit so the function seems smaller and a bit cleaner, maybe checkAndHandleResourceDeletion can be renamed a bit better but I don't have a better idea currently.

[dave-gantenbein]

Nice refactor - ship it!