Use of Unpack conditions

[varshaprasad96]

The direct registry client during unpacking exits only when it is successful or when it encounters an error: https://github.com/operator-framework/rukpak/blob/352d42f1e390177aed8fedf0cfb0212d5e71bc71/pkg/source/image_registry.go#L34. Which means the Unpacking and UnpackPending statuses would never be reached.

These were added initially, to update the client with the status when an unpack pod was being created (https://github.com/operator-framework/rukpak/blob/352d42f1e390177aed8fedf0cfb0212d5e71bc71/pkg/source/image.go#L54-L65). Given that implementation is no longer being used, having these statuses can also be technically removed.

However the caveat here is that having the pending status with a non-error nil return during reconcile, is extremely useful in unit-tests, where we use a mock unpacker and return with a pending state (

operator-controller/internal/controllers/clusterextension_controller_test.go

Lines 132 to 136 in aa48e70

	cl, reconciler := newClientAndReconciler(t, nil)
	mockUnpacker := unpacker.(*MockUnpacker)
	// Set up the Unpack method to return a result with StateUnpacked
	mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*v1alpha2.BundleDeployment")).Return(&source.Result{
	State: source.StatePending,

). Removing this, would force us to go through the next steps in reconcile, and implement a mock storer and loader.

#928 removes the Unpacking state, leaving UnpackPending behind. It would be helpful to evaluate its requirements, and if needed modify the tests to mock other steps in the reconciler.

[joelanford]

Removing this, would force us to go through the next steps in reconcile, and implement a mock storer and loader.

But I think we need to do that if we actually want to fully unit test our Reconciler, right?

[varshaprasad96]

But I think we need to do that if we actually want to fully unit test our Reconciler, right?

Yes definitely. Which is why this issue to fix the unit tests and remove unpackPending status.

I just wanted to explain why I didn't remove it in #928