-
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
clean up construction to make creating types more obvious #20777
clean up construction to make creating types more obvious #20777
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
/retest |
1 similar comment
/retest |
|
||
var cloudConfig []byte | ||
if kubeOptions.CloudProvider.CloudConfigFile != "" { | ||
if cloudConfigFile != "" { |
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.
nit: len(cloudConfigFile) == 0
@@ -147,31 +144,3 @@ func NewPluginInitializer( | |||
|
|||
return admission.PluginInitializers{genericInitializer, webhookInitializer, kubePluginInitializer, openshiftPluginInitializer}, nil | |||
} | |||
|
|||
type DefaultInformerAccess struct { |
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.
where has this moved?
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.
where has this moved?
unused.
@@ -22,6 +21,7 @@ import ( | |||
// install all APIs | |||
_ "github.com/openshift/origin/pkg/api/install" | |||
"github.com/openshift/origin/pkg/image/apiserver/registryhostname" | |||
"k8s.io/apiserver/pkg/registry/generic" |
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.
order
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.
also everywhere below.
@@ -42,13 +42,15 @@ func setup(t *testing.T) (etcd.KV, *etcdtesting.EtcdTestServer, *REST) { | |||
server, etcdStorage := etcdtesting.NewUnsecuredEtcd3TestClientServer(t) | |||
etcdStorage.Codec = legacyscheme.Codecs.LegacyCodec(schema.GroupVersion{Group: "image.openshift.io", Version: "v1"}) | |||
etcdClient := etcd.NewKV(server.V3Client) | |||
restOptions := generic.RESTOptions{StorageConfig: etcdStorage, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: 1, ResourcePrefix: "images"} |
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.
to avoid confusion, rename to imageRestOptions.
@@ -69,15 +69,17 @@ func setup(t *testing.T) (etcd.KV, *etcdtesting.EtcdTestServer, *REST) { | |||
server, etcdStorage := etcdtesting.NewUnsecuredEtcd3TestClientServer(t) | |||
etcdStorage.Codec = legacyscheme.Codecs.LegacyCodec(schema.GroupVersion{Group: "image.openshift.io", Version: "v1"}) | |||
etcdClient := etcd.NewKV(server.V3Client) | |||
restOptions := generic.RESTOptions{StorageConfig: etcdStorage, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: 1, ResourcePrefix: "imagestreams"} |
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.
imageStreamRestOptions
@@ -51,11 +50,6 @@ func RunOpenShiftAPIServer(masterConfig *configapi.MasterConfig) error { | |||
preparedOpenshiftAPIServer := openshiftAPIServer.GenericAPIServer.PrepareRun() | |||
|
|||
glog.Infof("Starting master on %s (%s)", masterConfig.ServingInfo.BindAddress, version.Get().String()) | |||
glog.Infof("Public master address is %s", masterConfig.MasterPublicURL) |
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.
intentional removal?
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.
intentional removal?
yeah, logically the openshift apiserver has no public address
if err != nil { | ||
return nil, fmt.Errorf("Error reading from cloud configuration file %s: %v", kubeOptions.CloudProvider.CloudConfigFile, err) | ||
return nil, fmt.Errorf("Error reading from cloud configuration file %s: %v", cloudConfigFile, err) |
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.
lower-case error
pkg/cmd/server/origin/master.go
Outdated
kapiserveroptions "k8s.io/kubernetes/cmd/kube-apiserver/app/options" | ||
) | ||
|
||
func (c *MasterConfig) newOpenshiftAPIConfig(kubeAPIServerConfig apiserver.Config) (*openshiftapiserver.OpenshiftAPIConfig, error) { | ||
var err error |
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.
move down where it is used
Some nits. |
f865066
to
3563995
Compare
comments addressed |
New changes are detected. LGTM label has been removed. |
Builds on #20741
This collapses more start paths into a single chain.
/assign @sttts @mfojtik