Updates ensureIngressDeleted() to check for removal of deps #330
Conversation
openshift-ci-robot
added
size/M
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to get current wildcard dnsrecord for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) | ||
| } | ||
| if record != nil { |
There was a problem hiding this comment.
Shouldn't use this return value if err is not nil
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to get deployment for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) | ||
| } | ||
| if deploy != nil { |
There was a problem hiding this comment.
Shouldn't use this return value if err is not nil
| errs := []error{} | ||
| if err := r.finalizeLoadBalancerService(ingress); err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to finalize load balancer service for %s/%s: %v", ingress.Namespace, ingress.Name, err)) | ||
| svc, err := r.finalizeLoadBalancerService(ingress) |
There was a problem hiding this comment.
Shouldn't read svc is err is not nil... how about just returning a "service still exist" error and when the service is not found return no error?
There was a problem hiding this comment.
retryable.New could be used for such resource-still-exists errors.
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to finalize load balancer service for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) | ||
| } | ||
| if svc != nil { |
There was a problem hiding this comment.
Better to use else if. It should not matter in this case since finalizeLoadBalancerService returns a nil service value when it returns a non-nil error value, but generally non-error return values should be ignored in the case of a non-nil error value.
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to get current wildcard dnsrecord for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) | ||
| } | ||
| if record != nil { |
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to get deployment for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) | ||
| } | ||
| if deploy != nil { |
| // on existing resources. | ||
| // TODO: How can we delete this code? | ||
| func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressController) error { | ||
| func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressController) (*corev1.Service, error) { |
There was a problem hiding this comment.
Instead of a pointer, how about returning a Boolean value to indicate presence of the service?
| func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressController) error { | ||
| func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressController) (*corev1.Service, error) { | ||
| service, err := r.currentLoadBalancerService(ci) | ||
| if err != nil { |
There was a problem hiding this comment.
if not found, return nil? No reason to return the service
There was a problem hiding this comment.
currentLoadBalancerService returns nil if the service does not exist.
danehans
force-pushed
the
bz_1766141_v2
branch
from
a20493d to
8f1465d
Compare
|
@ironcladlou @Miciah commit |
|
/lgtm |
openshift-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, Miciah 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 |
|
Great, thanks /hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
5 participants
Previously,
ensureIngressDeleted()was not ensuringingresscontrollerdependent resources no longer existed. This PR updatesensureIngressDeleted()to check for the existence of eachingresscontrollerdependent resource. If a dependent resource still exists,ensureIngressDeleted()will add an error to the error list. Theingresscontrollerfinalizer is only removed when no errors exist in the error list./assign @ironcladlou @Miciah