(feat): Communicate "Ready" Status of CatalogSource CR#17
(feat): Communicate "Ready" Status of CatalogSource CR#17joelanford merged 6 commits intooperator-framework:mainfrom
CatalogSource CR#17Conversation
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
varshaprasad96
left a comment
varshaprasad96
left a comment
There was a problem hiding this comment.
Looks good! We could handle others in follow up.
/lgtm
| const ( | ||
| TypeReady = "Ready" | ||
|
|
||
| ReasonContentsAvailable = "ContentsAvailable" |
There was a problem hiding this comment.
We could do in a follow up too. But having all the conditions and reasons added to an array eventually would help in testing condition invariants as in here in future.
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
|
New changes are detected. LGTM label has been removed. |
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
| func (r *CatalogSourceReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&corev1beta1.CatalogSource{}). | ||
| For(&corev1beta1.CatalogSource{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). |
There was a problem hiding this comment.
Don't do this. If someone/something changes the status out from under us, we should go back and re-reconcile.
There was a problem hiding this comment.
Currently since we don't have good error handling, not having this results in the controller getting into an error state because once we update the status it requeues and then errors out when trying to create all the Packages again even though they already exist.
Would you be okay with this going in for now and fixing it along with the changes needed for #6 ?
There was a problem hiding this comment.
Sure, can you add a TODO comment, summarizing that in the code?
There was a problem hiding this comment.
And maybe it makes sense to have a separate issue about just that particular problem?
| if err := r.Client.Status().Update(ctx, &catalogSource); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating catalogsource status: %v", err) | ||
| } |
There was a problem hiding this comment.
For follow-up: we should check to see if the status changed to avoid unecessary update calls. I think this may be covered by #5
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
For follow-up: We should go ahead and add controller Reconcile unit tests in the near future so that we get in the habit of adding/updating these tests and we update functionality.
| func updateStatusReady(catalogSource *corev1beta1.CatalogSource) { | ||
| meta.SetStatusCondition(&catalogSource.Status.Conditions, metav1.Condition{ | ||
| Type: corev1beta1.TypeReady, | ||
| Reason: corev1beta1.ReasonContentsAvailable, | ||
| Status: metav1.ConditionTrue, | ||
| Message: "catalog contents have been unpacked and are available on cluster", | ||
| }) | ||
| } |
There was a problem hiding this comment.
In all of the other scenarios when we return an error from Reconcile, we need to set this condition somehow. For now, I think it's fine to always say Status: false with the error message.
But eventually we'll need to consider what happens when an old version of the catalog is available but we had a problem unpacking/syncing the new version.
There was a problem hiding this comment.
That is partially the goal of #6 but if we want it loosely implemented in this PR I am happy to update it
There was a problem hiding this comment.
Yeah, I'd propose we at least do the big picture invariant of "all conditions are updated somehow in every Reconcile pass"
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
4 participants
Description
CatalogSourceStatustype to include aConditionsfield for communicating status conditionsReadyReadyat the end of the reconciliation loop (once all contents are on cluster)Motivation
CatalogSourceCatalogSourceReady status #8