Skip to content
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

Override routes from certain domains with the hostname template #19418

Merged

Conversation

ramr
Copy link
Contributor

@ramr ramr commented Apr 18, 2018

Allow routes from certain domains (ala defaulted domains) to be replaced with the subdomain/hostname-template the router environment is running under.

Fixes issue #16797

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 18, 2018
@ramr
Copy link
Contributor Author

ramr commented Apr 18, 2018

Still to add tests but @cldmnky can you please check if this works for you? Thx

@cldmnky
Copy link
Contributor

cldmnky commented Apr 19, 2018

Will test it, looks like it will solve our issue!

@ramr ramr force-pushed the override-defaulted-hosts branch from 61938aa to 570ef0f Compare April 20, 2018 01:33
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 20, 2018
@ramr ramr changed the title [DO-NOT-MERGE] Override routes from certain domains with the hostname template Override routes from certain domains with the hostname template Apr 20, 2018
@cldmnky
Copy link
Contributor

cldmnky commented Apr 20, 2018

@ramr It works perfectly for us!

Thanks!

@ramr
Copy link
Contributor Author

ramr commented Apr 20, 2018

flake #19058

@ramr
Copy link
Contributor Author

ramr commented Apr 20, 2018

/retest

@ramr
Copy link
Contributor Author

ramr commented Apr 23, 2018

/test gcp

@ramr ramr force-pushed the override-defaulted-hosts branch from 570ef0f to fdb3458 Compare April 24, 2018 20:42
@ramr
Copy link
Contributor Author

ramr commented Apr 24, 2018

/test integration

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just spelling nits. Thanks @ramr

/approve

@@ -165,6 +165,65 @@ var _ = g.Describe("[Conformance][Area:Networking][Feature:Router]", func() {
o.Expect(status).To(o.Equal(kapi.ConditionTrue))
o.Expect(condition.LastTransitionTime).NotTo(o.BeNil())
})

g.It("should override the route host for overriden domains with a custom value", func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overriden -> overridden

execPodName := exutil.CreateExecPodOrFail(oc.AdminKubeClient().CoreV1(), ns, "execpod")
defer func() { oc.AdminKubeClient().CoreV1().Pods(ns).Delete(execPodName, metav1.NewDeleteOptions(1)) }()

g.By(fmt.Sprintf("creating a scoped router with overriden domains from a config file %q", configPath))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@@ -104,6 +132,36 @@ objects:
ports:
- targetPort: http

# routes that contain overriden domains
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, ramr

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2018
@knobunc knobunc removed the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2018
@ramr ramr force-pushed the override-defaulted-hosts branch 2 times, most recently from 2bc100a to de9bf61 Compare April 30, 2018 19:19
@ramr
Copy link
Contributor Author

ramr commented Apr 30, 2018

@knobunc made the requested changes + squashed the commits. PTAL

@ramr
Copy link
Contributor Author

ramr commented Apr 30, 2018

/retest
jenkins job failed syncing initial repositories

with the subdomain/hostname-template that the router environment is
running under. And fix typos as per @knobunc review
Fixes issue openshift#16797
@ramr ramr force-pushed the override-defaulted-hosts branch from de9bf61 to e2b68bc Compare April 30, 2018 23:03
@ramr
Copy link
Contributor Author

ramr commented May 1, 2018

/test gcp

@knobunc
Copy link
Contributor

knobunc commented May 1, 2018

/lgtm

Thanks @ramr

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 1, 2018
@ramr
Copy link
Contributor Author

ramr commented May 1, 2018

/test gcp
timeouts waiting on deployment/pods

@knobunc
Copy link
Contributor

knobunc commented May 2, 2018

/retest

@ramr
Copy link
Contributor Author

ramr commented May 2, 2018

/test gcp
timed out on deployment

@ramr
Copy link
Contributor Author

ramr commented May 2, 2018

/test unit
flake #19599

@knobunc
Copy link
Contributor

knobunc commented May 3, 2018

/retest

@openshift-merge-robot openshift-merge-robot merged commit 4df76d9 into openshift:master May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants