BZ 1331303 allocate route host on update if host is empty in spec #8677
Conversation
| @@ -31,7 +32,7 @@ func (a *testAllocator) GenerateHostname(*api.Route, *api.RouterShard) string { | |||
| return a.Hostname | |||
| } | |||
|
|
|||
| func newStorage(t *testing.T, allocator *testAllocator) (*REST, *etcdtesting.EtcdTestServer) { | |||
| func newStorage(t *testing.T, allocator routetypes.RouteAllocator) (*REST, *etcdtesting.EtcdTestServer) { | |||
There was a problem hiding this comment.
@ramr I changed this to the interface because we were not actually testing nil allocators correctly
|
We want to remove server side allocation going forward, since the routers do that now. |
|
I'm not opposed to fixing the gap, but spec = "" is "let the routers assign it" |
|
@pweil- changes look good - I did start one out as well but this is good. @smarterclayton yeah its in 2 places - this can be removed once we set a default value to the router subdomain so that the routers do assign it. Edited I think this will need one more change. |
|
[test] |
|
I'm hoping with oc start we'll be in the spot to do that.
|
|
Looks like strategy is set correctly for update. So changes LGTM |
|
So there was an interesting use case for online that this actually helps. They wanted to be able to allow admins to set route hosts on routes but not regular users. Since we don't have field level authorization I think they're going to check groups in admission and clear out route hosts from there. I guess they could do that in a customized router impl but the userInfo isn't available at that point. The override in the router seems good and useful but it seems to me that this works well with the shard allocation (which is server side) which would allow you to set host overrides per shard. Just something to consider. |
|
I was talking to Miciah about not doing that though :). Since routers can On Apr 29, 2016, at 8:43 AM, Paul Weil [email protected] wrote: So there was an interesting use case for online that this actually helps. The override in the router seems good and useful but it seems to me that — |
|
[test] |
|
re[test] @ramr if there are no objections I'll merge after tests pass again |
|
@pweil- no objections to merging and re [test] |
|
Evaluated for origin test up to 1e92fe5 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4614/) |
|
[merge] On Wed, Jun 8, 2016 at 3:35 PM, OpenShift Bot [email protected]
|
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4640/) (Image: devenv-rhel7_4337) |
|
Evaluated for origin merge up to 1e92fe5 |
@ramr since I had this code open I went ahead and made a PR for it.