Added comments and tidy ups with test case to Delete

internal/controller/bucket/delete.go

@@ -120,29 +120,36 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error {
 		return err
 	}
 
-	// If an error occurred during deletion, we must return for requeue.
 	if deleteErr != nil {
 		c.log.Info("Failed to delete bucket on one or more backends", "error", deleteErr.Error())
 		traces.SetAndRecordError(span, deleteErr)
 
-		// If the error is BucketNotEmpty error, the DeleteBucket operation should be failed
-		// and the client should be able to use the bucket with non-empty buckends.
 		if errors.Is(deleteErr, rgw.ErrBucketNotEmpty) {
+			c.log.Info("Cannot deleted non-empty bucket - this error will not be requeued", consts.KeyBucketName, bucket.Name)
+			// An error occured attempting to delete the bucket because it is not empty.
+			// If this Delete operation was triggered because the Bucket CR was "Disabled",
+			// we need to unset this value so as not to continue attempting Delete.
+			// Otherwise we can return no error as we do not wish to requeue the Delete.
+			if !bucket.Spec.Disabled {
+				return nil
+			}
 			if err := c.updateBucketCR(ctx, bucket, func(bucketDeepCopy, bucketLatest *v1alpha1.Bucket) UpdateRequired {
-				c.log.Info("Change 'disabled' flag to false", consts.KeyBucketName, bucket.Name)
+				c.log.Info("Non-empty Buckets should not be disabled - setting 'disabled' flag to false", consts.KeyBucketName, bucket.Name)
 
 				bucketLatest.Spec.Disabled = false
 
 				return NeedsObjectUpdate
 			}); err != nil {
 				err = errors.Wrap(err, errUpdateBucketCR)
-				c.log.Info("Failed to change 'disabled' flag to false", consts.KeyBackendName, bucket.Name, "error", err.Error())
+				c.log.Info("Failed to set 'disabled' flag to false", consts.KeyBackendName, bucket.Name, "error", err.Error())
 				traces.SetAndRecordError(span, err)
 
 				return err
 			}
-		}
 
+			return nil
+		}
+		// In all other cases we should return the deletion error for requeue.
 		return deleteErr
 	}
 

internal/controller/bucket/delete_test.go

@@ -682,7 +682,111 @@ func TestDelete(t *testing.T) {
 				},
 			},
 			want: want{
-				err: rgw.ErrBucketNotEmpty,
+				err: nil,
+				statusDiff: func(t *testing.T, mg resource.Managed) {
+					t.Helper()
+					bucket, _ := mg.(*v1alpha1.Bucket)
+
+					// s3-backend-1 failed so is stuck in Deleting status.
+					assert.True(t,
+						bucket.Status.AtProvider.Backends[s3Backend1].BucketCondition.Equal(xpv1.Deleting().WithMessage(fmt.Errorf("%w: %w", rgw.ErrBucketNotEmpty, rgw.BucketNotEmptyError{}).Error())),
+						"unexpected bucket condition on s3-backend-1")
+
+					// s3-backend-2 was successfully deleted so was removed from status.
+					assert.False(t,
+						func(b v1alpha1.Backends) bool {
+							if _, ok := b[s3Backend2]; ok {
+								return true
+							}
+
+							return false
+						}(bucket.Status.AtProvider.Backends),
+						"s3-backend-2 should not exist in backends")
+
+					// If backend deletion fails due to BucketNotEmpty error, Disabled flag should be false.
+					assert.False(t,
+						bucket.Spec.Disabled,
+						"Disabled flag should be false",
+					)
+				},
+				finalizerDiff: func(t *testing.T, mg resource.Managed) {
+					t.Helper()
+					bucket, _ := mg.(*v1alpha1.Bucket)
+
+					assert.Equal(t,
+						[]string{v1alpha1.InUseFinalizer},
+						bucket.Finalizers,
+						"unexpeceted finalizers",
+					)
+				},
+			},
+		},
+		"Error deleting disabled bucket because one specified bucket is not empty": {
+			fields: fields{
+				backendStore: func() *backendstore.BackendStore {
+					// DeleteBucket first calls HeadBucket to establish
+					// if a bucket exists, so return a random error
+					// to mimic a failed delete.
+					fakeClient := &backendstorefakes.FakeS3Client{}
+					fakeClient.HeadBucketReturns(
+						&s3.HeadBucketOutput{},
+						nil,
+					)
+					fakeClient.DeleteBucketReturns(
+						nil,
+						rgw.BucketNotEmptyError{},
+					)
+
+					// DeleteBucket first calls HeadBucket to establish
+					// if a bucket exists, so return not found
+					// error to short circuit a successful delete.
+					var notFoundError *s3types.NotFound
+					fakeClientOK := &backendstorefakes.FakeS3Client{}
+					fakeClientOK.HeadBucketReturns(
+						&s3.HeadBucketOutput{},
+						notFoundError,
+					)
+
+					bs := backendstore.NewBackendStore()
+					bs.AddOrUpdateBackend(s3Backend1, fakeClient, nil, true, apisv1alpha1.HealthStatusHealthy)
+					bs.AddOrUpdateBackend(s3Backend2, fakeClientOK, nil, true, apisv1alpha1.HealthStatusHealthy)
+
+					return bs
+				}(),
+			},
+			args: args{
+				mg: &v1alpha1.Bucket{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "bucket",
+						Finalizers: []string{v1alpha1.InUseFinalizer},
+						Labels: map[string]string{
+							v1alpha1.BackendLabelPrefix + s3Backend1: "true",
+							v1alpha1.BackendLabelPrefix + s3Backend2: "true",
+						},
+					},
+					Spec: v1alpha1.BucketSpec{
+						Providers: []string{
+							s3Backend1,
+							s3Backend2,
+						},
+						Disabled: true,
+					},
+					Status: v1alpha1.BucketStatus{
+						AtProvider: v1alpha1.BucketObservation{
+							Backends: v1alpha1.Backends{
+								s3Backend1: &v1alpha1.BackendInfo{
+									BucketCondition: xpv1.Available(),
+								},
+								s3Backend2: &v1alpha1.BackendInfo{
+									BucketCondition: xpv1.Available(),
+								},
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				err: nil,
 				statusDiff: func(t *testing.T, mg resource.Managed) {
 					t.Helper()
 					bucket, _ := mg.(*v1alpha1.Bucket)