Refactor to level driven design #49
Conversation
|
/hold |
openshift-ci-robot
added
do-not-merge/hold
|
Holy cow, not sure why the dep update dragged in so much new junk. I want to revert that and try again. |
ironcladlou
force-pushed
the
level-driven
branch
from
f1e84dd to
0e16731
Compare
|
Okay, I decided to copy and paste a slightly modified |
openshift-ci-robot
added
size/L
|
Tested end-to-end in a 4.0 AWS cluster. Ensured defaulting and delete handling works. |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
Refactor the handler to be level driven.
ironcladlou
force-pushed
the
level-driven
branch
from
0e16731 to
03eb5e3
Compare
|
nice! |
| logrus.Infof("reconciling for update to clusteringress %q", o.Name) | ||
| } | ||
| return nil | ||
| return h.reconcile() |
There was a problem hiding this comment.
perform h.reconcole() only when we are dealing with ingressv1alpha1.ClusterIngress? This seems to call this reconciler for non cluster ingress objects.
There was a problem hiding this comment.
It'll trigger for any object we're actively watching in our entry point, which for now is just ClusterIngress, but in the future should be other types of resources that we might want to trigger a reconciliation.
There was a problem hiding this comment.
Why do we log conditionally and reconcile unconditionally?
There was a problem hiding this comment.
No reason, logging can be tweaked
pkg/stub/handler.go
Outdated
| } | ||
| // Clean up the finalizer to allow the clusteringress to be deleted. | ||
| ingress.Finalizers = RemoveString(ingress.Finalizers, "ingress.openshift.io/default-cluster-ingress") | ||
| err = sdk.Update(&ingress) |
There was a problem hiding this comment.
Once we update ingress object, don't we see another event for the same object? if yes, that will have DeletionTimestamp but no finalizers section?
There was a problem hiding this comment.
Yes, and although that still works now, we're doing an additional pointless update. So I think we're at least still correct/reentrant/idempotent... I'll fix
| func (h *Handler) reconcile() error { | ||
| // Ensure we have all the necessary scaffolding on which to place router | ||
| // instances. | ||
| err := h.ensureRouterNamespace() |
There was a problem hiding this comment.
Do we really want to do this for every event? Seems like one time op.
There was a problem hiding this comment.
Probably not, but it seems advantageous to keep it simple for now. We could do a lookup before bothering to try creating the namespace rather than relying on a superfluous create attempt.
pkg/stub/handler.go
Outdated
|
|
||
| // RemoveString returns a newly created []string that contains all items from slice that | ||
| // are not equal to s. | ||
| func RemoveString(slice []string, s string) []string { |
There was a problem hiding this comment.
removeString() if we don't expect to be used in other files.
There was a problem hiding this comment.
I wish the upstream slice package were in a better place so we could use it and some other nice stuff I'll have to copy in 😞
| manifestFactory *manifests.Factory | ||
| } | ||
|
|
||
| func (h *Handler) Handle(ctx context.Context, event sdk.Event) error { |
There was a problem hiding this comment.
What does ctx hold with this new scheme (not useful at this time)?
There was a problem hiding this comment.
Need to look at operator-sdk code, not sure. Nothing important for now AFAIK.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, pravisankar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I think |
What line is this in reference to? |
|
/retest |
Line 199. I would have commented inline but it's outside of the diff context. |
6 participants
Refactor the handler to be level driven.
/cc @openshift/sig-network-edge