Bug 1717494: Refactor client and cache handling #244
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
size/L
|
/retest |
ironcladlou
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
ironcladlou
changed the title
|
@Miciah @danehans this is ready for review... tomorrow I need to repro https://bugzilla.redhat.com/show_bug.cgi?id=1711373 with the ingress operator and make a new bug to link to this PR. I can do a similar change in dns-operator to fix the existing bug. |
|
/retest |
ironcladlou
changed the title
| @@ -55,9 +55,10 @@ func main() { | |||
| // Collect operator configuration. | |||
| operatorNamespace := os.Getenv("WATCH_NAMESPACE") | |||
| if len(operatorNamespace) == 0 { | |||
| log.Error(fmt.Errorf("missing environment variable"), "'WATCH_NAMESPACE' environment variable must be set") | |||
| os.Exit(1) | |||
| operatorNamespace = "openshift-ingress-operator" | |||
There was a problem hiding this comment.
This looks like a leftover from development that should be reverted.
There was a problem hiding this comment.
It was certainly useful — why not keep it?
pkg/operator/operator.go
Outdated
| NewCache: cache.MultiNamespacedCacheBuilder([]string{ | ||
| config.Namespace, | ||
| "openshift-ingress", | ||
| "openshift-config-managed", |
There was a problem hiding this comment.
This should use a named constant.
| if !o.manager.GetCache().WaitForCacheSync(stop) { | ||
| log.Error(nil, "failed to sync cache before ensuring default ingresscontroller") | ||
| return | ||
| } |
There was a problem hiding this comment.
Is ensureDefaultIngressController actually using the cache?
There was a problem hiding this comment.
Not right now, but generally it seemed like a good idea to wait until things were up and running
There was a problem hiding this comment.
Since the wait.Until uses a 1-minute period, does this mean that the operator will probably take an extra minute to create the default ingress controller on a new cluster?
There was a problem hiding this comment.
Hm, I guess in the normal case, WaitForCacheSync will block and succeed and delay the creation exactly as long as it takes to sync the cache.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, Miciah 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 |
MultiNamespacedCache