✨ add progressing condition by everettraven · Pull Request #1302 · operator-framework/operator-controller

everettraven · 2024-09-23T21:14:08Z

Description

Adds a new Progressing status condition
Removes unpack states that we never expect to be returned from our implementation, makes reconciliation panic if we get an unexpected state since it is a programmer error
Consolidates some unit tests
fixes[v1 API Review] Add Progressing status condition #1292

Reviewer Checklist

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

netlify · 2024-09-23T21:14:24Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`04c1b85`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66f3174339b5ea00088c174e
😎 Deploy Preview	https://deploy-preview-1302--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.

codecov · 2024-09-23T21:20:52Z

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.58%. Comparing base (eea61cf) to head (04c1b85).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	85.71%	2 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #1302 +/- ##
==========================================
+ Coverage 76.44% 76.58% +0.14% 
==========================================
Files 40 40 Lines 2390 2413 +23 ==========================================
+ Hits 1827 1848 +21 - Misses 395 397 +2 
Partials 168 168

Flag	Coverage Δ
e2e	`58.72% <71.87%> (+0.27%)`	⬆️
unit	`53.08% <93.75%> (+0.28%)`	⬆️

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.

everettraven · 2024-09-24T15:03:48Z

-	require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
-}
-
-func TestClusterExtensionResolutionSucceeds(t *testing.T) {


Removed this test function because I don't think we should be using a state we never expect to be returned as part of our standard code paths in a mock to end reconciliation without an error but still have successful resolution. Removing the unused, outside of testing, unpack states resulted in making the mock unpacker return an error. We already had a test (immediately after this one I think) that did this same logic so I renamed it and removed this test.

Note: the github diff view makes this look a tad wonky

everettraven · 2024-09-24T15:18:52Z

-	require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
-}
-
-func TestClusterExtensionUnpackSucceeds(t *testing.T) {


Removed this particular test function because it tests when unpack succeeds but the applier returns an error. We already have a test immediately after this one that tests the same functionality. I updated the name of the remaining test function to capture that it tests both unpack succeeding and applier failing.

everettraven · 2024-09-24T15:27:27Z

-	res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
-	require.Equal(t, ctrl.Result{}, res)
-	require.Error(t, err)
-
-	t.Log("By fetching updated cluster extension after reconcile")
-	require.NoError(t, cl.Get(ctx, extKey, clusterExtension))
-
-	t.Log("By checking the status fields")
-	require.Equal(t, ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Resolution.Bundle)
-	require.Empty(t, clusterExtension.Status.Install)
-
-	t.Log("By checking the expected conditions")
-	resolvedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved)
-	require.NotNil(t, resolvedCond)
-	require.Equal(t, metav1.ConditionTrue, resolvedCond.Status)
-	require.Equal(t, ocv1alpha1.ReasonSuccess, resolvedCond.Reason)
-	require.Equal(t, "resolved to \"quay.io/operatorhubio/prometheus@fake1.0.0\"", resolvedCond.Message)
-
-	t.Log("By checking the expected unpacked conditions")
-	unpackedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeUnpacked)
-	require.NotNil(t, unpackedCond)
-	require.Equal(t, metav1.ConditionFalse, unpackedCond.Status)
-	require.Equal(t, ocv1alpha1.ReasonFailed, unpackedCond.Reason)
-
-	require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
-}


Note: this github diff view is showing weird due to the code that was removed vs added. Since this is related to testing for an unexpected unpack state, which is a programmer error, this test now checks that Reconcile() panics.

everettraven · 2024-09-24T15:34:17Z

-	// StatePending conveys that a request for unpacking a bundle has been
-	// acknowledged, but not yet started.
-	StatePending State = "Pending"
-
-	// StateUnpacking conveys that the source is currently unpacking a bundle.
-	// This state should be used when the bundle contents are being downloaded
-	// and processed.
-	StateUnpacking State = "Unpacking"
-


This is somewhat outside of the scope of the intent of this PR, but while I was identifying where to set the Progressing condition I noted that we don't ever expect these unpack states from being returned. I figured it is a small enough change that it made sense to include it as part of this PR.

skattoju · 2024-09-24T17:29:29Z

- ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, "unpack pending")
- return ctrl.Result{}, nil
 	case rukpaksource.StateUnpacked:
 setStatusUnpacked(ext, unpackResult.Message)


Should we setStatusProgressing here ? or just remove this completely (probably in the removed unpacked status pr) ?

I would anticipate that we remove it completely in the unpacked status PR. There isn't really any value to setting it here since if there are no errors throughout reconciliation we set it to False, Succeeded

joelanford · 2024-09-24T18:09:40Z

+	require.NotNil(t, progressingCond)
+	require.Equal(t, metav1.ConditionTrue, progressingCond.Status)
+	require.Equal(t, ocv1alpha1.ReasonRetrying, progressingCond.Reason)
+


Since we expect the Progressing condition message to be the place that contains details of the resolution (if we progress that far), and there will eventually be no other place with that information, I think we should have some test assertions that look for <name>, <version>, and maybe <resolvedImageRef> in the message.

I think this is reasonable

should be updated in the latest push

tmshort · 2024-09-24T18:12:26Z

This has conflicts with removing the "Healthy" condition in #1304...

Signed-off-by: everettraven <everettraven@gmail.com>

openshift-ciBot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2024

everettraven commented Sep 24, 2024
View reviewed changes

Comment threadinternal/controllers/common_controller_test.go Outdated

everettraven commented Sep 24, 2024
View reviewed changes

Comment threadinternal/controllers/common_controller_test.go Outdated

everettraven commented Sep 24, 2024
View reviewed changes

everettraven force-pushed the status/progressing-condition branch from b2a83b4 to e669088Compare September 24, 2024 15:46

everettraven changed the title ~~WIP: add progressing condition~~✨ add progressing conditionSep 24, 2024

everettraven marked this pull request as ready for review September 24, 2024 15:48

everettraven requested a review from a team as a code owner September 24, 2024 15:48

openshift-ciBot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024

skattoju reviewed Sep 24, 2024
View reviewed changes

joelanford reviewed Sep 24, 2024
View reviewed changes

Comment threadinternal/controllers/common_controller.go

everettraven force-pushed the status/progressing-condition branch from e669088 to 63b56c0Compare September 24, 2024 19:41

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

add progressing condition
53f3d9e
Signed-off-by: everettraven <everettraven@gmail.com>

everettraven force-pushed the status/progressing-condition branch from 63b56c0 to 53f3d9eCompare September 24, 2024 19:44

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

cleanup lingering healthy condition reason
04c1b85
Signed-off-by: everettraven <everettraven@gmail.com>

joelanford approved these changes Sep 25, 2024
View reviewed changes

joelanford added this pull request to the merge queue Sep 25, 2024

Merged via the queue into operator-framework:main with commit 679e4abSep 25, 2024

skattoju mentioned this pull request Sep 25, 2024
✨ Remove unpacked status #1309
Merged
4 tasks

Uh oh!

Conversation

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

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlifyBot commented Sep 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecovBot commented Sep 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanfordSep 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort commented Sep 24, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

everettraven commented Sep 23, 2024•
edited
Loading

netlifyBot commented Sep 23, 2024•
edited
Loading

codecovBot commented Sep 23, 2024•
edited
Loading

joelanfordSep 24, 2024•
edited
Loading