operator-framework / operator-controller

A new and improved management framework for extending Kubernetes with Operators
https://operator-framework.github.io/operator-controller/
Apache License 2.0
61 stars 53 forks source link

:sparkles: refactor ClusterExtensionReconciler.reconcile() #1068

Closed everettraven closed 2 months ago

everettraven commented 2 months ago

Description

Makes the reconcile method on the ClusterExtensionReconciler easier to unit test by encapsulating distinct operations as interfaces that can be mocked:

Reviewer Checklist

netlify[bot] commented 2 months ago

Deploy Preview for olmv1 ready!

Name Link
Latest commit dcd37f04daedec62c93ec9592cb45eb22d127481
Latest deploy log https://app.netlify.com/sites/olmv1/deploys/66acee810502b8000801f846
Deploy Preview https://deploy-preview-1068--olmv1.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

joelanford commented 2 months ago

Nice! I like this idea. I'll take a closer look at this tomorrow, but one thing I would suggest off the bat is talking to @thetechnick about the Applier interface. I was chatting with him last week about experimenting with plugging package-operator's apply logic into operator-controller. I figure having a second implementation candidate would help validate our ideas behind the shape of that interface.

thetechnick commented 2 months ago

I do have a very rough first PoC here: https://github.com/thetechnick/operator-controller/pull/1 It directly uses Package Operator APIs to offload bundle reconciliation, so operator-controller just has to create a ClusterPackage, read status from it and lean back, while getting features similar to the OLMv0 Subscription config for free.

everettraven commented 2 months ago

@joelanford Sounds good. Are you thinking to utilize the ClusterPackage similarly to what @thetechnick has done (via the PoC) or more along the lines of importing the apply logic and using it as a library?

I'd be a bit hesitant to move back to an approach where we are creating an underlying resource like the ClusterPackage since we intentionally decided to deviate from that approach relatively recently (~3ish months ago).

everettraven commented 2 months ago

I will squash commits after reviews are fully addressed

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 56.91057% with 53 lines in your changes missing coverage. Please review.

Project coverage is 75.28%. Comparing base (989a3df) to head (dcd37f0).

Files Patch % Lines
internal/applier/helm.go 41.97% 35 Missing and 12 partials :warning:
cmd/manager/main.go 70.00% 2 Missing and 1 partial :warning:
internal/catalogmetadata/filter/successors.go 0.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1068 +/- ## ========================================== - Coverage 75.65% 75.28% -0.38% ========================================== Files 31 33 +2 Lines 1902 1861 -41 ========================================== - Hits 1439 1401 -38 - Misses 320 321 +1 + Partials 143 139 -4 ``` | [Flag](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1068/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | Coverage Δ | | |---|---|---| | [e2e](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `56.85% <48.78%> (-1.20%)` | :arrow_down: | | [unit](https://app.codecov.io/gh/operator-framework/operator-controller/pull/1068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `52.17% <23.57%> (+2.43%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tmshort commented 2 months ago

Looks reasonable to me, but needs a rebase.