feat(HACBS-2381): increase consistency

[davidmogar]

This change improves consistency across the project, following operator-toolkit best practices.

Depends on #213

[codecov]

Codecov Report

Patch coverage: 98.33% and project coverage change: -0.06% ⚠️

Comparison is base (5a8ba0d) 85.50% compared to head (45d4c39) 85.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
- Coverage   85.50%   85.45%   -0.06%     
==========================================
  Files          23       23              
  Lines        1442     1437       -5     
==========================================
- Hits         1233     1228       -5     
  Misses        150      150              
  Partials       59       59              

Files Changed	Coverage Δ
loader/loader.go	72.78% <ø> (ø)
controllers/releaseplan/controller.go	72.72% <88.88%> (-5.85%)	⬇️
controllers/release/controller.go	60.00% <90.00%> (-4.29%)	⬇️
api/v1alpha1/webhooks/author/webhook.go	79.54% <100.00%> (ø)
api/v1alpha1/webhooks/release/webhook.go	100.00% <100.00%> (ø)
api/v1alpha1/webhooks/releaseplan/webhook.go	100.00% <100.00%> (ø)
.../v1alpha1/webhooks/releaseplanadmission/webhook.go	100.00% <100.00%> (ø)
controllers/release/adapter.go	86.33% <100.00%> (ø)
controllers/releaseplan/adapter.go	55.55% <100.00%> (ø)
syncer/syncer.go	88.46% <100.00%> (ø)

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

[johnbieren]

This depends on #213 and also includes all of its changes. Given that, I am not really sure how this is ready for review

[johnbieren]

I am -1 to this change. I don't think it is worth merging 833 lines of new code with no new functionality or bug fixes, just renaming variables and moving stuff around

[davidmogar]

I am -1 to this change. I don't think it is worth merging 833 lines of new code with no new functionality or bug fixes, just renaming variables and moving stuff around

This is a change for increasing consistency, which is the main reason to write Operator Toolkit and the main reason behind the presentation we did. This change is following the best practices set by the framework and by the sample operator wrote using Operator Toolkit.

Given that it doesn't change functionality it should be easy to review and merge. Happy to discuss those best practices and take changes over to the framework itself.

Btw, same changes have been applied already to our other operator konflux-ci/internal-services#22

[davidmogar]

/retest

[johnbieren]

I am -1 to this change. I don't think it is worth merging 833 lines of new code with no new functionality or bug fixes, just renaming variables and moving stuff around

This is a change for increasing consistency, which is the main reason to write Operator Toolkit and the main reason behind the presentation we did. This change is following the best practices set by the framework and by the sample operator wrote using Operator Toolkit.

Given that it doesn't change functionality it should be easy to review and merge. Happy to discuss those best practices and take changes over to the framework itself.

Btw, same changes have been applied already to our other operator redhat-appstudio/internal-services#22

I am not seeing how changing function receivers from authorWebhook to webhook is part of operator-toolkit. And we did not already apply those changes to our other operator.. that PR you linked is open and unapproved

[davidmogar]

I am -1 to this change. I don't think it is worth merging 833 lines of new code with no new functionality or bug fixes, just renaming variables and moving stuff around

This is a change for increasing consistency, which is the main reason to write Operator Toolkit and the main reason behind the presentation we did. This change is following the best practices set by the framework and by the sample operator wrote using Operator Toolkit.
Given that it doesn't change functionality it should be easy to review and merge. Happy to discuss those best practices and take changes over to the framework itself.
Btw, same changes have been applied already to our other operator redhat-appstudio/internal-services#22

I am not seeing how changing function receivers from authorWebhook to webhook is part of operator-toolkit. And we did not already apply those changes to our other operator.. that PR you linked is open and unapproved

Saying that I applied the changes and I'm waiting for them to be reviewed.

Regarding consistency and best practices, this change makes every single webhook to be called webhook, which for new developers is easier to read. Most of the lines in this change go to putting in place a suite file (356 of them), avoid exposing structs that should not be exposed like the adapter, which right now is accessible from outside the controller package (which is wrong) and unifying the names (having just controller and webhook across all our controllers and webhooks).

[davidmogar]

/retest

[happybhati]

I do not disagree with anyone, but I think if the effort is already done and it makes no change to functionality but just the consistency with naming and refinements for better alignment with the framework I think we should move forward, but same time, such wide changes are concerning some time as Johhny told.
I am approving this PR because the code looks good to me. But the decision to merge this can be determined by the team.

[theflockers]

Generally speaking I am ok with the changes. Just on thing I noticed as this one is for consistency, in some places log
is declared as *logr.Logger and in others to logr.Logger. Any special reason or just that the functions depending needs it to be different for each case?

[davidmogar]

Generally speaking I am ok with the changes. Just on thing I noticed as this one is for consistency, in some places log is declared as *logr.Logger and in others to logr.Logger. Any reason besides go itself complaining on run time?

Controllers get a copy but anything else within the controller (adapter, syncer, etc) get a reference.

[mmalina]

I am not seeing how changing function receivers from authorWebhook to webhook is part of operator-toolkit.

Regarding consistency and best practices, this change makes every single webhook to be called webhook, which for new developers is easier to read. Most of the lines in this change go to putting in place a suite file (356 of them), avoid exposing structs that should not be exposed like the adapter, which right now is accessible from outside the controller package (which is wrong) and unifying the names (having just controller and webhook across all our controllers and webhooks).

Personally I'm not sure if this is better. Is there some convention for operators that we're now closer to? Or is this just your idea that this is better? I kind of liked the old structure more. And also, before, you could search the code base for something like authorWebhook, but now it's just webhook which is more generic, so not easy to search for.

But I'm fine with either way.

[davidmogar]

Good comments. Let me address them individually.

Personally I'm not sure if this is better. Is there some convention for operators that we're now closer to?

There's no convention as you can see by checking any RHTAP operator. The only "conventions" are the interfaces used like the the one containing the Reconcile method.

Or is this just your idea that this is better? I kind of liked the old structure more.

It kinda is my idea but for example, the controllers/adapters thing comes from another Red Hat operator. I built Operator Toolkit from there, trying to share as much code as possible and trying to keep a clean and readable structure.

And also, before, you could search the code base for something like authorWebhook, but now it's just webhook which is more generic, so not easy to search for.

There are multiple mentions of author in the very same file. What we get with the webhooks structure changes is:

• To have a api/v1alpha1 directory containing no webhook stuff but only types, tests for those types and conditions (less clutered)
• To be able to use stuff that requires imports to the api. That is the case of the loader, which can be used now in any webhook.
• To have an structure similar to the Controllers one. In fact, take a look to how enabled controllers and webhooks are declared:

var EnabledControllers = []controller.Controller{
	&release.Controller{},
	&releaseplan.Controller{},
}

And for webhooks:

var EnabledWebhooks = []webhook.Webhook{
	&author.Webhook{},
	&release.Webhook{},
	&releaseplan.Webhook{},
	&releaseplanadmission.Webhook{},
}

You can see that searching for author would point you to the Webhook import.

[mmalina]

@davidmogar thanks for the additional details. It makes sense.

[kasemAlem]

Sure thing is that those changes are turning the project to easier to read and follow up, at least for me .
Also I understand the others who seeing it as a huge jump and lot of changes on one take
if the functionality is not broken and performance is not reduced then I'm good with decision taken by team.

[openshift-ci]

New changes are detected. LGTM label has been removed.