Skip to content

Commit 3e5f5a0

Browse files
bentitotmshort
bentito
authored and
tmshort
committed
✨ OPRUN-3293 Fixes several unit tests - not cache ones (#831)
* Fix, rm or skip BundleDeployment related tests Signed-off-by: Brett Tofel <[email protected]> * Address lint errors Signed-off-by: Brett Tofel <[email protected]> --------- Signed-off-by: Brett Tofel <[email protected]>
1 parent ebdd1f1 commit 3e5f5a0

File tree

4 files changed

+103
-500
lines changed

4 files changed

+103
-500
lines changed

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ require (
99
github.com/blang/semver/v4 v4.0.0
1010
github.com/go-logr/logr v1.4.1
1111
github.com/google/go-cmp v0.6.0
12+
github.com/operator-framework/api v0.23.0
1213
github.com/operator-framework/catalogd v0.12.0
1314
github.com/operator-framework/helm-operator-plugins v0.2.1
1415
github.com/operator-framework/operator-registry v1.40.0
@@ -147,7 +148,6 @@ require (
147148
github.com/opencontainers/go-digest v1.0.0 // indirect
148149
github.com/opencontainers/image-spec v1.1.0 // indirect
149150
github.com/opencontainers/runtime-spec v1.2.0 // indirect
150-
github.com/operator-framework/api v0.23.0 // indirect
151151
github.com/operator-framework/operator-lib v0.12.0 // indirect
152152
github.com/otiai10/copy v1.14.0 // indirect
153153
github.com/peterbourgon/diskv v2.0.1+incompatible // indirect

‎internal/controllers/clusterextension_controller.go‎

Lines changed: 89 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"sort"
2626
"strings"
2727
"sync"
28+
"time"
2829

2930
mmsemver "github.com/Masterminds/semver/v3"
3031
bsemver "github.com/blang/semver/v4"
@@ -59,6 +60,7 @@ import (
5960
"sigs.k8s.io/controller-runtime/pkg/reconcile"
6061
"sigs.k8s.io/controller-runtime/pkg/source"
6162

63+
"github.com/operator-framework/api/pkg/operators/v1alpha1"
6264
catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
6365
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
6466
"github.com/operator-framework/operator-registry/alpha/declcfg"
@@ -72,6 +74,7 @@ import (
7274
"github.com/operator-framework/operator-controller/internal/catalogmetadata"
7375
catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter"
7476
catalogsort "github.com/operator-framework/operator-controller/internal/catalogmetadata/sort"
77+
"github.com/operator-framework/operator-controller/internal/conditionsets"
7578
"github.com/operator-framework/operator-controller/internal/handler"
7679
"github.com/operator-framework/operator-controller/internal/labels"
7780
"github.com/operator-framework/operator-controller/internal/packageerrors"
@@ -111,14 +114,13 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
111114

112115
var existingExt = &ocv1alpha1.ClusterExtension{}
113116
if err := r.Client.Get(ctx, req.NamespacedName, existingExt); err != nil {
114-
return ctrl.Result{}, client.IgnoreNotFound(err)
117+
return ctrl.Result{}, utilerrors.NewAggregate([]error{client.IgnoreNotFound(err), nil})
115118
}
116119

117120
reconciledExt := existingExt.DeepCopy()
118121
res, reconcileErr := r.reconcile(ctx, reconciledExt)
119-
if reconcileErr != nil {
120-
return ctrl.Result{}, reconcileErr
121-
}
122+
123+
var updateErrors []error
122124

123125
// Do checks before any Update()s, as Update() may modify the resource structure!
124126
updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status)
@@ -127,7 +129,7 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
127129

128130
if updateStatus {
129131
if updateErr := r.Client.Status().Update(ctx, reconciledExt); updateErr != nil {
130-
return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr})
132+
updateErrors = append(updateErrors, updateErr)
131133
}
132134
}
133135

@@ -137,11 +139,35 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
137139

138140
if updateFinalizers {
139141
if updateErr := r.Client.Update(ctx, reconciledExt); updateErr != nil {
140-
return res, utilerrors.NewAggregate([]error{reconcileErr, updateErr})
142+
updateErrors = append(updateErrors, updateErr)
141143
}
142144
}
143145

144-
return res, reconcileErr
146+
if reconcileErr != nil {
147+
updateErrors = append(updateErrors, reconcileErr)
148+
}
149+
150+
return res, utilerrors.NewAggregate(updateErrors)
151+
}
152+
153+
// ensureAllConditionsWithReason checks that all defined condition types exist in the given ClusterExtension,
154+
// and assigns a specified reason and custom message to any missing condition.
155+
func ensureAllConditionsWithReason(ext *ocv1alpha1.ClusterExtension, reason v1alpha1.ConditionReason, message string) {
156+
for _, condType := range conditionsets.ConditionTypes {
157+
cond := apimeta.FindStatusCondition(ext.Status.Conditions, condType)
158+
if cond == nil {
159+
// Create a new condition with a valid reason and add it
160+
newCond := metav1.Condition{
161+
Type: condType,
162+
Status: metav1.ConditionFalse,
163+
Reason: string(reason),
164+
Message: message,
165+
ObservedGeneration: ext.GetGeneration(),
166+
LastTransitionTime: metav1.NewTime(time.Now()),
167+
}
168+
ext.Status.Conditions = append(ext.Status.Conditions, newCond)
169+
}
170+
}
145171
}
146172

147173
// Compare resources - ignoring status & metadata.finalizers
@@ -151,6 +177,34 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool {
151177
return !equality.Semantic.DeepEqual(a, b)
152178
}
153179

180+
func (r *ClusterExtensionReconciler) handleResolutionErrors(ext *ocv1alpha1.ClusterExtension, err error) (ctrl.Result, error) {
181+
var aggErrs utilerrors.Aggregate
182+
if errors.As(err, &aggErrs) {
183+
for _, err := range aggErrs.Errors() {
184+
errorMessage := err.Error()
185+
if strings.Contains(errorMessage, "no package") {
186+
// Handle no package found errors, potentially setting status conditions
187+
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
188+
ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage)
189+
} else if strings.Contains(errorMessage, "invalid version range") {
190+
// Handle invalid version range errors, potentially setting status conditions
191+
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
192+
ensureAllConditionsWithReason(ext, "ResolutionFailed", errorMessage)
193+
} else {
194+
// General error handling
195+
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
196+
ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "")
197+
}
198+
}
199+
} else {
200+
// If the error is not an aggregate, handle it as a general error
201+
errorMessage := err.Error()
202+
setResolvedStatusConditionFailed(&ext.Status.Conditions, errorMessage, ext.Generation)
203+
ensureAllConditionsWithReason(ext, "InstallationStatusUnknown", "")
204+
}
205+
return ctrl.Result{}, err
206+
}
207+
154208
// Helper function to do the actual reconcile
155209
//
156210
// Today we always return ctrl.Result{} and an error.
@@ -159,13 +213,12 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool {
159213
//
160214
//nolint:unparam
161215
func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (ctrl.Result, error) {
162-
// Lookup the bundle that corresponds to the ClusterExtension's desired package.
163-
bundle, err := r.resolve(ctx, ext)
216+
l := log.FromContext(ctx).WithName("operator-controller")
217+
// run resolution
218+
bundle, err := r.resolve(ctx, *ext)
164219
if err != nil {
165-
ext.Status.ResolvedBundle = nil
166-
ext.Status.InstalledBundle = nil
167-
setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", "unable to get resolved bundle version", err), ext.Generation)
168-
return ctrl.Result{}, err
220+
l.V(1).Info("bundle resolve error", "error", err)
221+
return r.handleResolutionErrors(ext, err)
169222
}
170223

171224
bundleVersion, err := bundle.Version()
@@ -330,13 +383,12 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
330383
return ctrl.Result{}, nil
331384
}
332385

333-
func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) {
386+
func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) {
334387
allBundles, err := r.BundleProvider.Bundles(ctx)
335388
if err != nil {
336-
return nil, err
389+
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("error fetching bundles: %w", err)})
337390
}
338391

339-
// TODO: change ext spec to contain a source field.
340392
packageName := ext.Spec.PackageName
341393
channelName := ext.Spec.Channel
342394
versionRange := ext.Spec.Version
@@ -352,47 +404,53 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext *ocv1alpha
352404
if versionRange != "" {
353405
vr, err := mmsemver.NewConstraint(versionRange)
354406
if err != nil {
355-
return nil, fmt.Errorf("invalid version range %q: %w", versionRange, err)
407+
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("invalid version range '%s': %w", versionRange, err)})
356408
}
357409
predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr))
358410
}
359411

360412
var installedVersion string
361-
// Do not include bundle versions older than currently installed unless UpgradeConstraintPolicy = 'Ignore'
413+
var upgradeErrorPrefix string
362414
if ext.Spec.UpgradeConstraintPolicy != ocv1alpha1.UpgradeConstraintPolicyIgnore {
363-
installedVersionSemver, err := r.getInstalledVersion(ctx, *ext)
364-
if err != nil && !apierrors.IsNotFound(err) {
365-
return nil, err
415+
installedBundle, err := r.getInstalledVersion(ctx, ext)
416+
if err != nil {
417+
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("error fetching installed version: %w", err)})
366418
}
367-
if installedVersionSemver != nil {
368-
installedVersion = installedVersionSemver.String()
369-
370-
// Based on installed version create a caret range comparison constraint
371-
// to allow only minor and patch version as successors.
419+
if installedBundle != nil {
420+
installedVersion = installedBundle.String()
421+
upgradeErrorPrefix = fmt.Sprintf("error upgrading from currently installed version %q: ", installedVersion)
372422
wantedVersionRangeConstraint, err := mmsemver.NewConstraint(fmt.Sprintf("^%s", installedVersion))
373423
if err != nil {
374-
return nil, err
424+
return nil, utilerrors.NewAggregate([]error{fmt.Errorf("%serror creating version constraint: %w", upgradeErrorPrefix, err)})
375425
}
376426
predicates = append(predicates, catalogfilter.InMastermindsSemverRange(wantedVersionRangeConstraint))
377427
}
378428
}
379429

380430
resultSet := catalogfilter.Filter(allBundles, catalogfilter.And(predicates...))
381-
382431
if len(resultSet) == 0 {
383-
return nil, packageerrors.GenerateFullError(packageName, versionRange, channelName, installedVersion)
432+
switch {
433+
case versionRange != "" && channelName != "":
434+
return nil, packageerrors.GenerateVersionChannelError(packageName, versionRange, channelName)
435+
case versionRange != "":
436+
return nil, packageerrors.GenerateVersionError(packageName, versionRange)
437+
case channelName != "":
438+
return nil, packageerrors.GenerateChannelError(packageName, channelName)
439+
default:
440+
return nil, packageerrors.GenerateError(packageName)
441+
}
384442
}
385-
386443
sort.SliceStable(resultSet, func(i, j int) bool {
387444
return catalogsort.ByVersion(resultSet[i], resultSet[j])
388445
})
389446
sort.SliceStable(resultSet, func(i, j int) bool {
390447
return catalogsort.ByDeprecated(resultSet[i], resultSet[j])
391448
})
449+
392450
return resultSet[0], nil
393451
}
394452

395-
// setDeprecationStatus will set the appropriate deprecation statuses for a ClusterExtension
453+
// SetDeprecationStatus will set the appropriate deprecation statuses for a ClusterExtension
396454
// based on the provided bundle
397455
func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetadata.Bundle) {
398456
// reset conditions to false

0 commit comments

Comments
 (0)