Return a subscription from handleSubscription
#198
Conversation
JustinKuli
force-pushed
the
useful-subscription-return
branch
from
ef38ec5
to
72eeb42
Compare
| } | ||
|
|
||
| desiredSub.SetGroupVersionKind(subscriptionGVK) // Create stripped this information | ||
|
|
||
| // Now it should match, so report Compliance | ||
| err = r.updateStatus(ctx, policy, createdCond("Subscription"), createdObj(desiredSub)) | ||
| if err != nil { | ||
| return fmt.Errorf("error updating the status for a created Subscription: %w", err) | ||
| return desiredSub, fmt.Errorf("error updating the status for a created Subscription: %w", err) | ||
| } |
There was a problem hiding this comment.
I think returning a subscription inside of the logic that handles the enforce remediationAction type would be useful for future handler functions. It would make sense for this function to return a subscription only if it was created successfully on the cluster.
| } | ||
| } | ||
|
|
||
| return nil | ||
| return desiredSub, nil |
There was a problem hiding this comment.
I think returning something like nil, nil would be better here if we can return the subscription in the enforce case. Then in my PR, I could use the return value to determine if the desired subscription exists on the cluster (nil vs a subscription object) and skip a call to r.DynamicWatcher.Get to retrieve the subscription.
There was a problem hiding this comment.
It's possible... but I think that if inform mode returned nil, nil here, it would artificially limit some of the information we can get. For example, it is generally possible to check if the CatalogSource specified in the policy exists on the cluster, even if the Subscription doesn't exist.
There was a problem hiding this comment.
I might be overlooking something but is the point of this to reduce calls to buildSubscription?
There was a problem hiding this comment.
Mostly. This would reduce calls to buildSubscription and dynamicWatcher.Get for the Subscription.
This can also return the "merged" subscription, regardless of inform/enforce, which hypothetically could be useful for some other healthcheck... I don't see a need for that yet, but it feels like it's easy enough to "do it right" now, so we don't have to worry about it later. For example if some other check used the channel in the subscription, this function would always return the desired setting based on the policy, which is what I think the user would be expecting.
There was a problem hiding this comment.
I see, I think that would be useful in general. For my specific case I'm only calling dynamicWatcher.Get on a catalogsource when the subscription doesn't exist yet, so I might need to end up calling dynamicWatcher.Get on the subscription anyways in handleCatalogSource because the return values of handleSubscription doesn't seem to indicate whether or not a subscription exists on the cluster. I will look into ways to work around this on my end.
There was a problem hiding this comment.
Should mention that I'm specifically thinking about the inform case.
Some of the additional checks will need pieces of information from the Subscription. Converting it before returning it (as opposed to using an Unstructured) should help reduce other error checking. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
force-pushed
the
useful-subscription-return
branch
from
72eeb42
to
b45e55c
Compare
|
/lgtm |
| } | ||
|
|
||
| mergedSub := new(operatorv1alpha1.Subscription) | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(merged.Object, mergedSub); err != nil { |
There was a problem hiding this comment.
I just checked and I don't believe this will produce an error if there are unknown fields (e.g. version drift of different OLM versions). So this looks to be safe if you are just reading from it.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 934c69b
into
open-cluster-management-io:main
Some of the additional checks will need pieces of information from the Subscription. Converting it before returning it (as opposed to using an Unstructured) should help reduce other error checking.