-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NE-1907: Manage OSSM operator subscription manually to ensure a compatible version is installed #1112
base: master
Are you sure you want to change the base?
NE-1907: Manage OSSM operator subscription manually to ensure a compatible version is installed #1112
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,6 +178,7 @@ rules: | |
- operators.coreos.com | ||
resources: | ||
- subscriptions | ||
- installplans | ||
verbs: | ||
- '*' | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |||||
|
||||||
"github.com/google/go-cmp/cmp" | ||||||
"github.com/google/go-cmp/cmp/cmpopts" | ||||||
"sigs.k8s.io/controller-runtime/pkg/client" | ||||||
|
||||||
operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" | ||||||
|
||||||
|
@@ -16,6 +17,11 @@ import ( | |||||
"k8s.io/apimachinery/pkg/types" | ||||||
) | ||||||
|
||||||
var ( | ||||||
serviceMeshOperatorDesiredVersion = "servicemeshoperator.v2.6.2" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is going to work. If we change the version in a future release and need to backport a different change in the same file, won't this get overwritten? I think normally version information is recorded in a manifest yaml. Can you find a way to do this without hardcoding the version here? I'll check with @Miciah because I don't know the answer. |
||||||
serviceMeshOperatorNamespace = "openshift-operators" | ||||||
) | ||||||
|
||||||
// ensureServiceMeshOperatorSubscription attempts to ensure that a subscription | ||||||
// for servicemeshoperator is present and returns a Boolean indicating whether | ||||||
// it exists, the subscription if it exists, and an error value. | ||||||
|
@@ -56,10 +62,11 @@ func desiredSubscription(name types.NamespacedName) (*operatorsv1alpha1.Subscrip | |||||
}, | ||||||
Spec: &operatorsv1alpha1.SubscriptionSpec{ | ||||||
Channel: "stable", | ||||||
InstallPlanApproval: operatorsv1alpha1.ApprovalAutomatic, | ||||||
InstallPlanApproval: operatorsv1alpha1.ApprovalManual, | ||||||
Package: "servicemeshoperator", | ||||||
CatalogSource: "redhat-operators", | ||||||
CatalogSourceNamespace: "openshift-marketplace", | ||||||
StartingCSV: serviceMeshOperatorDesiredVersion, | ||||||
}, | ||||||
} | ||||||
return &subscription, nil | ||||||
|
@@ -115,3 +122,40 @@ func subscriptionChanged(current, expected *operatorsv1alpha1.Subscription) (boo | |||||
|
||||||
return true, updated | ||||||
} | ||||||
|
||||||
// ensureServiceMeshOperatorInstallPlan attempts to ensure that the install plan for the appropriate OSSM operator | ||||||
// version is approved. | ||||||
func (r *reconciler) ensureServiceMeshOperatorInstallPlan(ctx context.Context) (bool, *operatorsv1alpha1.InstallPlan, error) { | ||||||
currentInstallPlan, err := r.getCurrentInstallPlan(ctx) | ||||||
if err != nil { | ||||||
return false, nil, err | ||||||
} else if currentInstallPlan == nil { | ||||||
return false, nil, nil | ||||||
} | ||||||
if !currentInstallPlan.Spec.Approved { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idiomatic way to write an "ensure" function is to get the current object, get the desired object, compare them, and use the "have" and "want" values to determine whether to create or update. Can you update this to use the idiomatic form? |
||||||
currentInstallPlan.Spec.Approved = true | ||||||
if err := r.client.Update(ctx, currentInstallPlan); err != nil { | ||||||
return false, nil, fmt.Errorf("Failed to update %s/%s: %w", currentInstallPlan.Namespace, currentInstallPlan.Name, err) | ||||||
} | ||||||
return true, currentInstallPlan, nil | ||||||
} | ||||||
return false, currentInstallPlan, nil | ||||||
} | ||||||
|
||||||
func (r *reconciler) getCurrentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use the function name we normally use.
Suggested change
|
||||||
InstallPlans := &operatorsv1alpha1.InstallPlanList{} | ||||||
if err := r.client.List(ctx, InstallPlans, client.InNamespace(serviceMeshOperatorNamespace)); err != nil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the installPlan doesn't have a unique name? |
||||||
return nil, err | ||||||
} | ||||||
if InstallPlans == nil || len(InstallPlans.Items) == 0 { | ||||||
return nil, nil | ||||||
} | ||||||
for _, InstallPlan := range InstallPlans.Items { | ||||||
for _, CSVName := range InstallPlan.Spec.ClusterServiceVersionNames { | ||||||
if CSVName == serviceMeshOperatorDesiredVersion { | ||||||
return &InstallPlan, nil | ||||||
} | ||||||
} | ||||||
} | ||||||
return nil, fmt.Errorf("No InstallPlan with cluster service version %s found", serviceMeshOperatorDesiredVersion) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you check to make sure
len(ClusterServiceVersionNames) > 0
first?