controller: Rework boolean logic for consistency #176
Conversation
sgreene570
force-pushed
the
boolean-logic-rework-ensure
branch
from
f60ef55
to
ff375d4
Compare
| } | ||
| desired, err := desiredDNSConfigMap(dns, clusterDomain) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build configmap: %v", err) | ||
| return haveCM, nil, fmt.Errorf("failed to build configmap: %v", err) |
There was a problem hiding this comment.
I would think this should return return haveCM, current, fmt.Errorf(...).
That said, I grepped for return true, nil, in cluster-ingress-operator and found several matches. However, it makes sense that if we return true (indicating that the resource exists), we should return a non-nil pointer value; would you agree?
There was a problem hiding this comment.
I would agree. Will update this, good catch!
There was a problem hiding this comment.
Perhaps some of the return true, nil, ... lines in the cluster-ingress-operator could be cleaned up as well. I'm working a PR for the cluster-ingress-operator similar to this one, so I'll keep that in mind.
| } | ||
| logrus.Infof("created dns cluster role: %s/%s", desired.Namespace, desired.Name) | ||
| case desired != nil && current != nil: | ||
| case haveCR: | ||
| if err := r.updateDNSClusterRole(current, desired); err != nil { |
There was a problem hiding this comment.
Sidenote: A future cleanup could add a Boolean return value to the update methods that don't have one to indicate whether an update was performed. If no update was performed, the caller can return true, current, nil instead of falling through to return r.currentFoo (which incurs an unnecessary API call).
There was a problem hiding this comment.
Good idea! I suspect there are quite a few places that have these extra API calls, not just in the cluster-dns-operator but also in the cluster-ingress-operator, right?
There was a problem hiding this comment.
Probably, yeah. This is an opportunity to make all of this logic more consistent, correct, and efficient across both operators.
Switches all `ensureDNS<thing>` functions in `pkg/operator/controller` to use a `have<thing>` boolean rather than manually checking if `<thing> != nil`.
sgreene570
force-pushed
the
boolean-logic-rework-ensure
branch
from
ff375d4
to
cded466
Compare
|
Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 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-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Switches all
ensureDNS<thing>functions inpkg/operator/controllerto use a
have<thing>boolean rather than manually checking if<thing> != nil.@Miciah mentioned that this is the pattern the cluster-ingress-operator tries to follow, so the cluster-dns-operator should do the same.