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

convert CLI to use generated clients #16603

Merged

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Sep 28, 2017

@soltysh so close!

This removes more legacy clients

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 28, 2017
c.ImageStreamByAnnotationSearcher = app.NewImageStreamByAnnotationSearcher(osclient, osclient, namespaces)
c.ImageStreamByAnnotationSearcher = app.NewImageStreamByAnnotationSearcher(
c.ImageClient,
c.ImageClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I wanna ask, but srsly? We need twice the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I wanna ask, but srsly? We need twice the same thing?

two different interfaces for getters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a TODO here to fix this.

)

// ImageStreamSearcher searches the openshift server image streams for images matching a particular name
type ImageStreamSearcher struct {
Client client.ImageStreamsNamespacer
ImageStreamImages client.ImageStreamImagesNamespacer
Client imageclient.ImageStreamsGetter
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably rename them to reflect what this client is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably rename them to reflect what this client is.

follow. We're stacked up and these pulls conflict like crazy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a TODO here to fix this.

@deads2k
Copy link
Contributor Author

deads2k commented Sep 28, 2017

/approve

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

All the rest can be addressed post-merge.

c.ImageStreamByAnnotationSearcher = app.NewImageStreamByAnnotationSearcher(osclient, osclient, namespaces)
c.ImageStreamByAnnotationSearcher = app.NewImageStreamByAnnotationSearcher(
c.ImageClient,
c.ImageClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a TODO here to fix this.

if err != nil {
return errors.NewError("cannot obtain API clients").WithCause(err).WithDetails(h.OriginLog())
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to have less info in the returned error than before?

if err != nil {
return err
}
config.SetOpenShiftClient(imageClient.Image(), templateClient.Template(), routeClient.Route(), namespace, dockerClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a TODO to fix this, looks weird with the old name currently.

@@ -252,9 +252,7 @@ func TestUnprivilegedNewProjectDenied(t *testing.T) {
}

valerieClientConfig.BearerToken = accessToken
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch 👍

@@ -232,18 +231,13 @@ func (cfg *Config) OpenShiftConfig() *restclient.Config {
}

// Clients returns an OpenShift and a Kubernetes client from a given configuration
func (cfg *Config) Clients() (osclient.Interface, kclientset.Interface, error) {
func (cfg *Config) Clients() (kclientset.Interface, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

KubeClientset or alike

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, soltysh

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2017
@deads2k deads2k added the kind/bug Categorizes issue or PR as related to a bug. label Sep 29, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16525, 16602, 16603).

@openshift-merge-robot openshift-merge-robot merged commit f67b9a8 into openshift:master Sep 29, 2017
@deads2k deads2k deleted the client-09-more-removal branch January 24, 2018 14:35
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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. needs-api-review size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants