-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix for bug1431655: delete reencrypt routes #14921
Conversation
5ebbbe2
to
7f13ab7
Compare
@@ -510,6 +510,28 @@ func (p *F5Plugin) deleteRoute(routename string) error { | |||
return err | |||
} | |||
} | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should not use "else" here. That means if passthrough route existed, then no chance to delete reencrypt routes, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But aren't they mutually exclusive anyway? For the given routename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw other three type routes are checked in sequence in the function. Seems only one type route will be matched for a given routename.
LGTM. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[merge] |
[test] Last flaked on #14043 (logs: https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2773/ ) |
[test] flake #13271 |
[test] just for funzies this time. |
re-[merge] last flaked on #14043 (logs: https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1172/) |
[test] |
[test] I think we flaked on the rate limiter bug #12558 |
Evaluated for origin test up to 7f13ab7 |
[merge] last flaked on GCE provisioning |
Evaluated for origin merge up to 7f13ab7 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1187/) (Base Commit: 1d1aa8a) (PR Branch Commit: 7f13ab7) (Image: devenv-rhel7_6415) |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2879/) (Base Commit: 916ac6c) (PR Branch Commit: 7f13ab7) |
Fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1431655
bug: 1431655
@lihongan Review please
@pravisankar PTAL
/cc @openshift/networking