Register aggregator resources into scheme before starting components #15226
Conversation
|
[test] |
|
[severity:blocker] |
|
@deads2k PTAL |
| testserver "github.com/openshift/origin/test/util/server" | ||
| ) | ||
|
|
||
| func TestAggregator(t *testing.T) { |
There was a problem hiding this comment.
I did this upstream, but unless you create a service running an actual API server behind it, this test never catches the breaks you're interested in. Also, this would be removed as soon as we merge the rebase and make the aggregator non-optional.
There was a problem hiding this comment.
unless you create a service running an actual API server behind it, this test never catches the breaks you're interested in
I was interested in the "can the aggregator start without causing a panic" break
pkg/cmd/server/origin/aggregator.go
Outdated
| // install our types into the scheme so that "normal" RESTOptionsGetters can work for us. | ||
| // done in init() so we don't mutate the scheme after it is being used by clients in other goroutines. | ||
| // TODO: move to a scheme-per-process like upstream and register into that instead. | ||
| install.Install(kapi.GroupFactoryRegistry, kapi.Registry, kapi.Scheme) |
There was a problem hiding this comment.
We really don't want to do this. If your integration tests requires it, do it there. You shouldn't be relying on a side effect of loading the package and if anyone gets used to it, we'll be chasing problems around as people who transitively depend on this package but never starts a server will accidentally work.
There was a problem hiding this comment.
this wasn't for the integration test, it was the fix. I can put it in an isolated install package and import it just for our server wiring, but it's safer to do this in an init() in 3.6
|
@deads2k moved to Master#Start() instead of init() block, PTAL |
pkg/cmd/server/start/start_master.go
Outdated
| // install aggregator types into the scheme so that "normal" RESTOptionsGetters can work for us. | ||
| // done in Start() prior to doing any other initialization so we don't mutate the scheme after it is being used by clients in other goroutines. | ||
| // TODO: make scheme threadsafe and do this as part of aggregator config building | ||
| aggregatorinstall.Install(kapi.GroupFactoryRegistry, kapi.Registry, kapi.Scheme) |
There was a problem hiding this comment.
ought to be able to gate this on the presence of the aggregator proxy client cert, right?
There was a problem hiding this comment.
gated the same way the config building is gated
|
Evaluated for origin test up to fea9842 |
|
lgtm [merge] |
|
Evaluated for origin merge up to fea9842 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1296/) (Base Commit: 6c3acd2) (PR Branch Commit: fea9842) (Extended Tests: blocker) (Image: devenv-rhel7_6448) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3208/) (Base Commit: 271fb1c) (PR Branch Commit: fea9842) |
liggitt
changed the title
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1469923