Improve uninstall command #29
Conversation
| // If csvObj is nil and crds is not present, it means the package was not found | ||
| if subObj == nil && len(crds) == 0 { |
There was a problem hiding this comment.
| // If csvObj is nil and crds is not present, it means the package was not found | |
| if subObj == nil && len(crds) == 0 { | |
| // If no objects were cleaned up, it means the package was not found | |
| if subObj == nil && csvObj == nil && len(crds) == 0 { |
| subObj = sub | ||
| // CSV name may either be the installed or current name in a subscription's status, | ||
| // depending on installation state. | ||
| csvKey := types.NamespacedName{ | ||
| Name: sub.Status.InstalledCSV, | ||
| Namespace: u.config.Namespace, | ||
| } | ||
| if csvKey.Name == "" { | ||
| csvKey.Name = sub.Status.CurrentCSV | ||
| } | ||
|
|
||
| // This value can be empty which will cause errors. | ||
| if csvKey.Name != "" { | ||
| csv := &v1alpha1.ClusterServiceVersion{} | ||
| if err := u.config.Client.Get(ctx, csvKey, csv); err != nil && !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("error getting installed CSV %q: %v", csvKey.Name, err) | ||
| } else if err == nil { | ||
| crds = getCRDs(csv) | ||
| } | ||
| csvObj = csv |
There was a problem hiding this comment.
(optional) you could move all of this into OperatorUninstall.getInstalledCSV().
Refactor uninstall command to clean up all the resources, and report whether package existed correctly. Co-authored by: Eric Stroczynski <ericstroczynski@gmail.com>
varshaprasad96
force-pushed
the
improve/uninstall
branch
from
090cd7d
to
174ae1b
Compare
| return fmt.Errorf("get installed CSV %q: %v", sub.Status.InstalledCSV, err) | ||
| var subObj, csvObj controllerutil.Object | ||
| var crds []controllerutil.Object | ||
| if sub != nil { |
There was a problem hiding this comment.
By this point, we know that sub is non-nil because we checked it and returned early in lines 64-66, so IIUC there's no need for this conditional, and also therefore no need for the subObj variable.
There was a problem hiding this comment.
You're correct, this condition can be removed due to the above check. This PR is a port of operator-framework/operator-sdk#4303 so there might be some incongruities that should be resolved. subObj was necessary due to some strange nil comparison behavior I experienced when writing the linked PR, and made the code a little cleaner. It might not make sense here given your comments below.
| @@ -107,6 +140,11 @@ func (u *OperatorUninstall) Run(ctx context.Context) error { | |||
| } | |||
| } | |||
|
|
|||
| // If no objects were cleaned up, it means the package was not found | |||
| if subObj == nil && csvObj == nil && len(crds) == 0 { | |||
| return &ErrPackageNotFound{u.Package} | |||
There was a problem hiding this comment.
Since we know subObj is never nil by this point (right?), I think we can completely remove this conditional block and use ErrPackageNotFound in line 65.
| @@ -81,7 +114,7 @@ func (u *OperatorUninstall) Run(ctx context.Context) error { | |||
| // OLM puts an ownerref on every namespaced resource to the CSV, | |||
| // and an owner label on every cluster scoped resource. When CSV is deleted | |||
| // kube and olm gc will remove all the referenced resources. | |||
| if err := u.deleteObjects(ctx, csv); err != nil { | |||
| if err := u.deleteObjects(ctx, csvObj); err != nil { | |||
There was a problem hiding this comment.
If we come out of the CSV fetching with a nil csvObj, that means we will have a nil CSV and no CRDs to delete (since we find the CRDs from the CSV), right?
Do we need to check to see if the CSV is nil before trying to delete it here?
If it is nil, I think that means we'll only be deleting the subscription and possibly the operator group and NOT erroring out on a missing CSV? Seems reasonable since its possible there's a problem with the subscription that prevented the CSV from being created.
There was a problem hiding this comment.
A nil check should be performed in deleteObjects() like in operator-framework/operator-sdk#4303.
|
I finally got around to submitting PR #31 that makes these changes based on my suggestions. |
Refactor uninstall command to clean up all the resources,
and report whether package existed correctly.
Co-authored by: Eric Stroczynski ericstroczynski@gmail.com