-
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
Routers should be able to override the host value on Routes #5725
Routers should be able to override the host value on Routes #5725
Conversation
// Complete converts string representations of field and label selectors to their parsed equivalent, or | ||
// returns an error. | ||
func (o *RouterSelection) Complete() error { | ||
if len(o.HostnameTemplate) == 0 && o.OverrideHostname { | ||
return fmt.Errorf("--override-hostname requires that --hostname-template be specified") |
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 specified?
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.
"be" is correct (subjunctive mood).
a89f867
to
f2e1f69
Compare
@ramr / @pravisankar can one of you look through this? Still need to add the integration test, but is functional. |
Yeah, some tests would be good - the changes look good. |
[test] |
67429d8
to
4b3adfa
Compare
[test][extended:core] |
Ram, can you help me debug the extended failure? Getting pulled into something else today - would like to close this out. |
@smarterclayton sure - will take a look at it this eve/over the weekend. |
@smarterclayton took a while - this commit ramr@9787405 fixes the test errors but it also needs a rebase from master as there was another fix that went in last week to properly report 503 pages (status headers). |
Add --hostname-template which takes a format like '${name}-${namespace}.cluster.local' for defaulting and add --override-hostname to allow forcing of the route.
4b3adfa
to
35c68f4
Compare
Thanks, much appreciated On Tue, Dec 15, 2015 at 3:44 AM, Ram Ranganathan notifications@github.com
|
@smarterclayton if you are swamped, did you want me to create a new PR from my branch with your code as the base and rebased from master + the above commit (ramr@9787405) added in? Let me know. Thx |
nm - should have reloaded the page before I commented!! :^( ... think me needs that coffee now!! |
[test] |
1 similar comment
[test]
|
Evaluated for origin test up to 35c68f4 |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7856/) (Extended Tests: core) |
Trap is green [merge]
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4358/) (Image: devenv-rhel7_2965) |
Evaluated for origin merge up to 35c68f4 |
Merged by openshift-bot
Add
--hostname-template
which takes a format like${name}-${namespace}.myapps.com
for defaulting and add--override-hostname
to allow forcing of the route.[test]