NE-882: Set ROUTER_DOMAIN to enable route subdomain field
#674
Conversation
openshift-ci
bot
added
the
approved
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
1 similar comment
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
Miciah
changed the title
ROUTER_DOMAIN to enable route subdomain fieldROUTER_DOMAIN to enable route subdomain field
| @@ -232,6 +232,7 @@ func TestDesiredRouterDeployment(t *testing.T) { | |||
|
|
|||
| checkDeploymentHasEnvVar(t, deployment, "ROUTER_USE_PROXY_PROTOCOL", false, "") | |||
|
|
|||
| checkDeploymentHasEnvVar(t, deployment, "ROUTER_DOMAIN", false, "") | |||
There was a problem hiding this comment.
If you control-f in this function, there are 4 desiredRouterDeployment, so 4 different generations of a deployment object. Looks like you did the private, loadbalancing, but there's another loadbalancing and the hostnetwork ingress controller tests.
To be thorough, would it make sense to just add the checks after the other two new deployment objects on what you expect? Specifically, I remember working a bug in which private and loadbalancing did one thing and hostNetwork did something different.
There was a problem hiding this comment.
Oh and I'll throw another plug that this function is super confusing and needs to be written into different tests or something more readable (for another day I suppose...)
There was a problem hiding this comment.
If you control-f in this function, there are 4
desiredRouterDeployment, so 4 different generations of a deployment object. Looks like you did the private, loadbalancing, but there's another loadbalancing and the hostnetwork ingress controller tests.To be thorough, would it make sense to just add the checks after the other two new deployment objects on what you expect? Specifically, I remember working a bug in which private and loadbalancing did one thing and hostNetwork did something different.
I suppose I could check ROUTER_DOMAIN after each call to desiredRouterDeployment. However, the intention with TestDesiredRouterDeployment was really that for each configuration option, we exercise every code path related to that option. In some cases, there are only two possible code paths, so we only need to check the effect of two different settings to cover both code paths (that's the case with status.domain and ROUTER_DOMAIN: either the domain is present or it's empty, and we set the environment variable or omit it accordingly). In other cases, we have one option with many code paths (such as spec.endpointPublishingStrategy.type, which has 4 possible settings with different code paths; we also have some settings that interact in interesting ways, which we need to test). It just happens that we mashed everything into a single test function, so we end up calling desiredRouterDeployment` four times in the same test, which is needed to cover the possible code paths for some options, even if it isn't needed for other code paths and options.
Oh and I'll throw another plug that this function is super confusing and needs to be written into different tests or something more readable (for another day I suppose...)
Coming soon to a Git clone near you (courtesy of Candace): #712
(Which means I'm going to need to rebase once that PR merges. * grin *.)
There was a problem hiding this comment.
Sure, so you are basically saying you aren't exercising a different code path for the other desiredRouterDeployment. That's fair, I can buy that. I think it comes back to how large and confusing this function is, so I'm excited to see Candace's update.
There was a problem hiding this comment.
It's merged! I don't know who's more excited, you or me!
Set ROUTER_DOMAIN to the IngressController's domain in the router deployment so that the router can use the domain to construct a default host for routes that specify an empty value for spec.host and a nonempty value for spec.subdomain. * pkg/operator/controller/ingress/deployment.go (desiredRouterDeployment): Set ROUTER_DOMAIN. * pkg/operator/controller/ingress/deployment_test.go (TestDesiredRouterDeployment): Verify that ROUTER_DOMAIN is not set. (TestDesiredRouterDeploymentSpecAndNetwork): Verify that ROUTER_DOMAIN is set to the expected value.
Miciah
force-pushed
the
set-ROUTER_DOMAIN-to-enable-route-subdomain-field
branch
from
9435046 to
f8b71b1
Compare
|
Rebased for #712. |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
/label docs-approved |
openshift-ci
bot
added
the
docs-approved
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278, 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 |
|
@Miciah: all tests passed! 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. |
Set
ROUTER_DOMAINto the IngressController's domain in the router deployment so that the router can use the domain to construct a default host for routes that specify an empty value forspec.hostand a nonempty value forspec.subdomain.pkg/operator/controller/ingress/deployment.go(desiredRouterDeployment): SetROUTER_DOMAIN.pkg/operator/controller/ingress/deployment_test.go(TestDesiredRouterDeployment): Verify thatROUTER_DOMAINis set as expected.