-
Notifications
You must be signed in to change notification settings - Fork 191
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
OCPBUGS-15978: Check public DNS zone when reporting status #967
OCPBUGS-15978: Check public DNS zone when reporting status #967
Conversation
@Miciah: This pull request references Jira Issue OCPBUGS-15978, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
9f9e4ef
to
04e74bd
Compare
@Miciah: This pull request references Jira Issue OCPBUGS-15978, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@Miciah: This pull request references Jira Issue OCPBUGS-15978, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign |
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.
Now that you're testing the public zone too, maybe you should adjust the test description prefixes to something other than "[PrivateZone]".
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.
if dnsConfig.Spec.PrivateZone.ID == zone.ID { | ||
return true | ||
} | ||
return zonesMatch(&zone, dnsConfig.Spec.PublicZone) || zonesMatch(&zone, dnsConfig.Spec.PrivateZone) |
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.
Is this guaranteed to run the second half of the conjunction if the first half is true? In other words, will it just stop evaluating if the public zone matches? Also, is it possible for there to be both a Spec.PublicZone and Spec.PrivateZone , so that we'd want to use &&
rather than ||
?
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 get it now. You can disregard this.
It just needs a new description for the unit tests. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita 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 |
Noticed quite alot of ssh failures, try again.
/test e2e-hypershift |
When reporting the "DNSReady" status condition on an IngressController, check the status conditions for both the public zone and the private zone of the associated DNSRecord. Before this commit, only the status condition for the private zone was used to compute the "DNSReady" status condition. If the operator failed to publish a DNS record in the public zone, this failure was not reported in the IngressController's status conditions or the "ingress" clusteroperator status conditions. Follow-up to commit 9f32923. This commit fixes OCPBUGS-15978. https://issues.redhat.com/browse/OCPBUGS-15978 * pkg/operator/controller/ingress/status.go (checkZoneInConfig): Use the new zonesMatch helper function to check both the public zone as well as the private zone. (zonesMatch): New function. Return a Boolean value indicating whether two DNS zones match, based on their respective ID or "Name" tags. * pkg/operator/controller/ingress/status_test.go (Test_checkZoneInConfig): Verify that checkZoneInConfig checks the public zone as well as the private zone. Delete "[PrivateZone]" from test-case descriptions as each test case is now tested as both a public zone and a private zone.
04e74bd
to
5578044
Compare
@Miciah: This pull request references Jira Issue OCPBUGS-15978, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
https://github.com/openshift/cluster-ingress-operator/compare/04e74bd30f2438729d7ed8d22f63b1b8ce25d01b..55780444031714fc931d90af298a4b193888977a deletes "[PrivateZone]" from test-case descriptions as each test case is now tested as both a public zone and a private zone. |
/lgtm |
@Miciah: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
cd97771
into
openshift:master
@Miciah: Jira Issue OCPBUGS-15978: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-15978 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
When reporting the "DNSReady" status condition on an IngressController, check the status conditions for both the public zone and the private zone of the associated DNSRecord.
Before this change, only the status condition for the private zone was used to compute the "DNSReady" status condition. If the operator failed to publish a DNS record in the public zone, this failure was not reported in the IngressController's status conditions or the "ingress" clusteroperator status conditions.
Follow-up to #641.
pkg/operator/controller/ingress/status.go
(checkZoneInConfig
): Use the newzonesMatch
helper function to check both the public zone as well as the private zone.(
zonesMatch
): New function. Return a Boolean value indicating whether two DNS zones match, based on their respective ID or "Name" tags.pkg/operator/controller/ingress/status_test.go
(Test_checkZoneInConfig
): Verify thatcheckZoneInConfig
checks the public zone as well as the private zone. Delete "[PrivateZone]" from test-case descriptions as each test case is now tested as both a public zone and a private zone./holdRebase after #880 merges.