✨ refactor ClusterExtensionReconciler.reconcile() #1068

everettraven · 2024-07-18T14:53:46Z

Description

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

(Done before this PR) Resolution is now done via a Resolver interface.
- (Done as part of this PR) Bundle validation is now done by the Resolver. The reasoning behind this is that it reduces the complexity of the reconcile method by making the assumption that the configured Resolver will always return a valid bundle. Validation unit tests that existed in the internal/controller package have been moved and updated in the internal/resolve package
Unpacking was unchanged
Installation of content is now abstracted into a more generic Applier interface. This makes it easier to, in the future, swap out the underlying installation method by implementing a new Applier interface and swapping it out for the existing one. The existing Helm-based installation logic was moved to an implementation of this interface.
Watching of managed resources was moved to a new Watcher interface that was implemented as part of the provided service account work. This was going to be a change that took place in the near future. From a functionality perspective, nothing should have changed.
Unit tests added for the ClusterExtensionReconciler.reconcile() method

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-07-18T14:54:08Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`dcd37f0`
🔍 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... Use your smartphone camera to open QR code link.

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

joelanford · 2024-07-19T00:51:04Z

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 · 2024-07-23T07:59:32Z

I do have a very rough first PoC here:
thetechnick#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.

internal/controllers/clusterextension_controller.go

everettraven · 2024-07-23T18:15:13Z

@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 · 2024-07-26T19:21:22Z

internal/applier/helm.go

+	Preflights         []Preflight
+}
+
+func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.ClusterExtension, labels map[string]string) ([]client.Object, string, error) {


Ideally we make this function more easily unit testable and add explicit unit tests for this function and the individual components but I would put that out of scope of the intention of this PR.

tmshort · 2024-07-26T19:53:20Z

internal/catalogmetadata/filter/successors.go

 	installedBundleVersion, err := mmsemver.NewVersion(installedBundle.Version)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("parsing installed bundle version %q: %w | installedBundle %v", installedBundle.Version, err, installedBundle)


Suggested change

return nil, fmt.Errorf("parsing installed bundle version %q: %w | installedBundle %v", installedBundle.Version, err, installedBundle)

return nil, fmt.Errorf("parsing installed bundle %q version %q: %w | installedBundle %v", installedBundle.Name, installedBundle.Version, err)

Ah shoot, some of that is still debugging changes I didn't rollback - thanks for catching that!

should be updated in 8c12758

everettraven · 2024-07-26T19:56:52Z

I will squash commits after reviews are fully addressed

codecov · 2024-07-26T20:02:20Z

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 ⚠️
cmd/manager/main.go	70.00%	2 Missing and 1 partial ⚠️
internal/catalogmetadata/filter/successors.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            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	Coverage Δ
e2e	`56.85% <48.78%> (-1.20%)`	⬇️
unit	`52.17% <23.57%> (+2.43%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

tmshort · 2024-08-01T19:51:28Z

Looks reasonable to me, but needs a rebase.

…testing by moving bundle validation logic to the resolver, moving installation of content behind a new Applier interface, and using the new contentmanager.Watcher interface for watching managed objects Signed-off-by: everettraven <[email protected]>

Signed-off-by: everettraven <[email protected]>

tmshort

/lgtm

…#1068) * update reconciler with logical interface abstractions to simply unit testing by moving bundle validation logic to the resolver, moving installation of content behind a new Applier interface, and using the new contentmanager.Watcher interface for watching managed objects Signed-off-by: everettraven <[email protected]> * update testing Signed-off-by: everettraven <[email protected]> * update testing Signed-off-by: everettraven <[email protected]> * remove bundle validation unit tests from controller package Signed-off-by: everettraven <[email protected]> * make linter happy Signed-off-by: everettraven <[email protected]> * remove postrenderer, replace with map[string]string for labels Signed-off-by: everettraven <[email protected]> * update debugging error string Signed-off-by: everettraven <[email protected]> * rebase fixes Signed-off-by: everettraven <[email protected]> --------- Signed-off-by: everettraven <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2024

everettraven commented Jul 23, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

everettraven force-pushed the shiftweek/reconciler branch from 9afedde to 9fe7cf0 Compare July 25, 2024 20:02

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2024

everettraven commented Jul 26, 2024

View reviewed changes

everettraven marked this pull request as ready for review July 26, 2024 19:29

everettraven requested a review from a team as a code owner July 26, 2024 19:29

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2024

everettraven changed the title ~~✨ WIP: reconciler refactor~~ ✨ refactor ClusterExtensionReconciler.reconcile() Jul 26, 2024

tmshort reviewed Jul 26, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2024

everettraven added 8 commits August 2, 2024 10:26

update testing

02f1611

Signed-off-by: everettraven <[email protected]>

update testing

c1c1d76

Signed-off-by: everettraven <[email protected]>

remove bundle validation unit tests from controller package

15a74b1

Signed-off-by: everettraven <[email protected]>

make linter happy

b3c6305

Signed-off-by: everettraven <[email protected]>

remove postrenderer, replace with map[string]string for labels

95d620c

Signed-off-by: everettraven <[email protected]>

update debugging error string

89d71a7

Signed-off-by: everettraven <[email protected]>

rebase fixes

dcd37f0

Signed-off-by: everettraven <[email protected]>

everettraven force-pushed the shiftweek/reconciler branch from 8c12758 to dcd37f0 Compare August 2, 2024 14:34

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2024

everettraven enabled auto-merge August 2, 2024 15:09

everettraven added this pull request to the merge queue Aug 2, 2024

tmshort approved these changes Aug 2, 2024

View reviewed changes

openshift-ci bot assigned tmshort Aug 2, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2024

Merged via the queue into operator-framework:main with commit cab9eab Aug 2, 2024

joelanford mentioned this pull request Aug 6, 2024

Improve unit testing of reconcile() function. #879

Closed

skattoju mentioned this pull request Sep 9, 2024

🌱 Descriptive Upgrade Constraint Policy constants #1233

Merged

skattoju mentioned this pull request Sep 25, 2024

✨ Remove unpacked status #1309

Merged

4 tasks

	return nil, fmt.Errorf("parsing installed bundle version %q: %w \| installedBundle %v", installedBundle.Version, err, installedBundle)
	return nil, fmt.Errorf("parsing installed bundle %q version %q: %w \| installedBundle %v", installedBundle.Name, installedBundle.Version, err)

✨ refactor ClusterExtensionReconciler.reconcile() #1068

✨ refactor ClusterExtensionReconciler.reconcile() #1068

Uh oh!

Conversation

everettraven commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlify bot commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

joelanford commented Jul 19, 2024

Uh oh!

thetechnick commented Jul 23, 2024

Uh oh!

Uh oh!

everettraven commented Jul 23, 2024

Uh oh!

everettraven Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

tmshort Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

everettraven Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

everettraven Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

everettraven commented Jul 26, 2024

Uh oh!

codecov bot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tmshort commented Aug 1, 2024

Uh oh!

tmshort left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

everettraven commented Jul 18, 2024 •

edited

Loading

netlify bot commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading