Disconnected #162
Disconnected #162
Conversation
eks-distro-bot
added
approved
size/L
|
/hold until other pr merges |
| pbc.Status.State = api.BundleControllerStateDisconnected | ||
| err = r.Client.Status().Update(ctx, pbc, &client.UpdateOptions{}) | ||
| if err != nil { | ||
| return result, fmt.Errorf("updating status to active: %s", err) |
There was a problem hiding this comment.
It would seem there might have been a cut and paste error with this log message?
| } | ||
|
|
||
| latestBundle, err := r.bundleManager.LatestBundle(ctx, pbc.Spec.Source.BaseRef()) | ||
| if err != nil { | ||
| r.Log.Error(err, "Unable to get latest bundle") | ||
| if pbc.Status.State != api.BundleControllerStateDisconnected { |
There was a problem hiding this comment.
I'm unclear on the intention here. If we fail (for any reason) to retrieve a bundle, we mark the bundle controller as disconnected? Maybe I don't understand what is meant by disconnected in this case, but that doesn't seem right. Could you give me more insight into the "why" behind this please?
There was a problem hiding this comment.
This is related to #109 and it was a request during our high level design review. If for whatever reason we cannot download new bundles, we should notify the customer that we are in an error state. They can tell they are in an error state using kubectl instead of looking at logs
There was a problem hiding this comment.
Hmm, thanks. Shoot, I see now that you linked that issue in your PR description, and I just missed it cuz of the nice table there.
There was a problem hiding this comment.
I might need a little time to get used to disconnected as the term. It doesn't feel right to me, but I can't think of anything better. :)
| if changed { | ||
| err = r.Client.Status().Update(ctx, pbc, &client.UpdateOptions{}) | ||
| if err != nil { | ||
| return result, fmt.Errorf("updating status to active: %s", err) |
There was a problem hiding this comment.
Another cut and paste error?
| @@ -113,20 +113,31 @@ func (r *PackageBundleControllerReconciler) Reconcile(ctx context.Context, req c | |||
| return withoutRequeue(result), nil | |||
| } | |||
|
|
|||
| changed := false | |||
There was a problem hiding this comment.
Github won't let me comment on the top of this method, but it seems it's growing a lot. We should review it and see if it follows our design intents and/or how we can logically break it into smaller, more bite-sized chunks.
(Not for this PR of course)
There was a problem hiding this comment.
Yeh, totally agree, some of this code should be in manager.go
There was a problem hiding this comment.
Created a issue to track
eks-distro-bot
added
size/S
|
/unhold |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ewollesen, jonahjon, TerryHowe 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 |
Closes: #109