stop injecting special discovery behavior into oc/kubectl by deads2k · Pull Request #19327 · openshift/origin

deads2k · 2018-04-11T20:31:26Z

This moves oc to use nothing but groupified APIs and allow us to eliminate any special factory handling. We'd have fewer special cases.

@openshift/sig-master

deads2k · 2018-04-11T21:36:37Z

Wow, that's a green test-cmd!

deads2k · 2018-04-11T22:51:58Z

And the e2es are failing on the test for old defaulting. This is great. Looks like I just need to limit restmapper patch.

soltysh · 2018-04-12T10:03:26Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util/factory_client_access.go

+	// The more groups you have, the more discovery requests you need to make.
+	// given 25 groups (our groups + a few custom resources) with one-ish version each, discovery needs to make 50 requests
+	// double it just so we don't end up here again for a while.  This config is only used for discovery.
+	cfg.Burst = 100


This definitely deserves upstreaming.

This definitely deserves upstreaming.

Already working it

mfojtik · 2018-04-13T12:07:13Z

test/cmd/authentication.sh

 os::cmd::expect_success_and_text "oc policy can-i create pods --token='${accesstoken}' -n '${project}'" 'no'
-os::cmd::expect_success_and_text "oc policy can-i create subjectaccessreviews --token='${accesstoken}' -n '${project}'" 'no'
-os::cmd::expect_success_and_text "oc policy can-i create subjectaccessreviews --token='${accesstoken}' -n '${project}' --ignore-scopes" 'yes'
+os::cmd::expect_success_and_text "oc policy can-i create subjectaccessreviews.authorization.openshift.io --token='${accesstoken}' -n '${project}'" 'no'


is the full name required now? looks ugly

is the full name required now? looks ugly

It's a different resource if you let the group be filled in automatically.

deads2k · 2018-04-13T18:22:43Z

I've rebased this on top of all the prereq fixes. I think it may work.

deads2k · 2018-04-13T19:52:53Z

This is a test update and commit fix up from green. I think we should do it.

@smarterclayton @liggitt anyone really against?

openshift-ci-robot · 2018-04-16T13:39:05Z

[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

Details

Needs approval from an approver in each of these files:

~~pkg/oc/OWNERS~~ [deads2k]
~~test/cmd/OWNERS~~ [deads2k]
~~test/extended/OWNERS~~ [deads2k]
~~test/integration/OWNERS~~ [deads2k]
~~test/testdata/OWNERS~~ [deads2k]
~~vendor/k8s.io/kubernetes/pkg/OWNERS~~ [deads2k]
~~vendor/k8s.io/kubernetes/staging/src/k8s.io/client-go/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh

One small nit/question, otherwise this lgtm

soltysh · 2018-04-17T12:05:29Z

test/cmd/login.sh

 unset KUBERNETES_MASTER
 # test client not configured
-os::cmd::expect_failure_and_text "env -u KUBERNETES_SERVICE_HOST oc get services" 'Missing or incomplete configuration info.  Please login'
+os::cmd::expect_failure_and_text "env -u KUBERNETES_SERVICE_HOST oc get services --loglevel=8" 'Missing or incomplete configuration info.  Please login'


Debug leftover?

mfojtik · 2018-04-17T12:25:07Z

pkg/oc/admin/diagnostics/diagnostics/client/config_contexts.go

-					break
+		projectClient, err := projectclientinternal.NewForConfig(clientConfig)
+		// client create now *fails* if cannot connect to server; so, address connectivity errors below
+		if err == nil {


what happens when err != nil ?

what happens when err != nil ?

Review with whitespace ignored. This increased nesting. The error is handled below as it was before.

mfojtik · 2018-04-17T12:27:22Z

pkg/oc/admin/prune/deployments.go

+		return err
+	}
+	o.AppsClient = appsClient
 	o.KClient = kClient


can you rename this to KubeClient since you are here?

mfojtik · 2018-04-17T12:32:21Z

LGTM as well, nice job!

deads2k · 2018-04-17T14:31:14Z

nits addressed

deads2k · 2018-04-17T17:19:03Z

/retest

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/master approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 11, 2018

openshift-ci-robot requested review from pecameron and smarterclayton April 11, 2018 20:31

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 11, 2018

soltysh reviewed Apr 12, 2018

View reviewed changes

deads2k mentioned this pull request Apr 12, 2018

use the upstream construction for kubeconfig evaluation #19335

Merged

mfojtik reviewed Apr 13, 2018

View reviewed changes

deads2k force-pushed the cli-23-removeoapi-2 branch from 5042f9e to 80bdf3f Compare April 13, 2018 18:21

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2018

deads2k changed the title ~~[WIP] stop injecting special discovery behavior into oc/kubectl~~ stop injecting special discovery behavior into oc/kubectl Apr 13, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2018

deads2k force-pushed the cli-23-removeoapi-2 branch from 80bdf3f to a5971e1 Compare April 16, 2018 13:38

deads2k force-pushed the cli-23-removeoapi-2 branch 4 times, most recently from bcfcefd to 6b85582 Compare April 16, 2018 18:10

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2018

deads2k force-pushed the cli-23-removeoapi-2 branch from 6b85582 to e659b32 Compare April 16, 2018 18:19

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2018

update oc commands to build their own clients

5264945

deads2k force-pushed the cli-23-removeoapi-2 branch from e659b32 to f7e8a85 Compare April 16, 2018 19:46

UPSTREAM: 62461: allow higher burst for discovery

e9adaf8

UPSTREAM: <carry>: rewrite unstructured objects on the CLI to avoid oapi

7d3a129

deads2k force-pushed the cli-23-removeoapi-2 branch from f7e8a85 to ebd6f88 Compare April 16, 2018 19:47

soltysh approved these changes Apr 17, 2018

View reviewed changes

mfojtik reviewed Apr 17, 2018

View reviewed changes

stop shimming the upstream factory inputs

e974738

deads2k force-pushed the cli-23-removeoapi-2 branch from ebd6f88 to e974738 Compare April 17, 2018 14:31

deads2k added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2018

deads2k mentioned this pull request Apr 17, 2018

update oc commands to build their own clients #19345

Merged

deads2k added the queue/multiple-rebases label Apr 17, 2018

openshift-merge-robot merged commit d5cb67d into openshift:master Apr 17, 2018

enj mentioned this pull request Apr 27, 2018

Correctly handle legacy ungroupified PSP review resources #19542

Merged

deads2k deleted the cli-23-removeoapi-2 branch July 3, 2018 17:47

Conversation

deads2k commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Apr 11, 2018

Uh oh!

deads2k commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 13, 2018

Uh oh!

deads2k commented Apr 13, 2018

Uh oh!

openshift-ci-robot commented Apr 16, 2018

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Apr 17, 2018

Uh oh!

deads2k commented Apr 17, 2018

Uh oh!

deads2k commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

deads2k commented Apr 11, 2018 •

edited

Loading