Move client/unversioned/remotecommand to client-go #41331

dshulyak · 2017-02-13T15:14:07Z

Module remotecommand originally part of kubernetes/pkg/client/unversioned was moved
to client-go/tools, and will be used as authoritative in kubectl, e2e and other places.

Module remotecommand relies on util/exec module which was copied to client-go/pkg/util

k8s-reviewable · 2017-02-13T15:14:14Z

This change is

dshulyak · 2017-02-13T15:14:35Z

@caesarxuchao Can you take a look please?

dshulyak · 2017-02-14T12:04:49Z

@caesarxuchao Hello, can you tell me when you will have time to take a look at this PR? I have some weird results, for example apimachinery is removed from _vendor, and i am not able to figure out whether i should add it back manually or not..

caesarxuchao · 2017-02-14T13:33:35Z

staging/copy.sh

@@ -125,6 +127,7 @@ rm -rf "${CLIENT_REPO_TEMP}"/vendor/k8s.io/kubernetes
 # client-go will share the vendor of the main repo for now. When client-go
 # becomes a standalone repo, it will have its own vendor
 mv "${CLIENT_REPO_TEMP}"/vendor "${CLIENT_REPO_TEMP}"/_vendor
+rm -r "${CLIENT_REPO_TEMP}"/_vendor/github.com/docker/docker/project/


Why do you need to manually remove this folder?

this folder containers symlink that references file that is not copied

caesarxuchao · 2017-02-14T13:35:35Z

staging/src/k8s.io/client-go/cmd/kubeadm/app/apis/kubeadm/doc.go

@@ -0,0 +1,18 @@
+/*


Why does kubeadm get imported?

caesarxuchao · 2017-02-14T13:37:44Z

staging/src/k8s.io/client-go/federation/apis/federation/doc.go

@@ -0,0 +1,17 @@
+/*


Looks like remotecommand imports federation types as well, can we remove the dependency on federation and kubeadm types?

yes, looks like i can

caesarxuchao · 2017-02-14T13:44:42Z

staging/src/k8s.io/client-go/pkg/kubelet/qos/doc.go

@@ -0,0 +1,25 @@
+/*


We are trying to remove the packages in /pkg/kubelet, which are are duplicate packages between kuberentes and client-go. Are there other users of pkg/kubelet/qos except for client-go? If so, we can move this package to client-go, and remove the original copy. cc @deads2k @sttts

deads2k · 2017-02-14T13:51:27Z

No more copies. If this needs to move, we aren't going to make a copy of it. The authoritative (and only) copy should be in staging/client-go.

caesarxuchao · 2017-02-14T13:54:32Z

@dshulyak i think it's correct that apimachinery is removed from vendor/. deads2k ran copy.sh once yesterday. You can rebase your PR so that you'll see less changes.

sttts · 2017-02-14T14:02:52Z

Why is remotecommand needed in client-go? Everything we move there we effectively declare to be a public, stable API (at least within minor versions, but still). Hence, what is the use-case?

Moreover, kubelet/server/remotecommand and pkg/util/term with those tons of dependencies move too?

IMO we need better separation, i.e. a number of dependencies cut before moving it.

dshulyak · 2017-02-14T14:08:04Z

@sttts I am using exec in several k8s related projects to run different tests. I think there is definitely demand for such thing in client kubernetes/client-go#45

I can check what can be done to cut those dependencies before moving it

sttts · 2017-02-14T14:09:11Z

@dshulyak please try to cut those terminal related dependencies and of course the kubelet dep.

sttts · 2017-02-15T11:40:58Z

pkg/util/remotecommand/constants.go

@@ -0,0 +1,53 @@
+/*


pkg/util is an odd location for this. It's actually part of the API. As we don't have a better location for this, moving it to client-go would be fine IMO.

the same for pkg/util/exec, i can't find good place for it

perhaps pkg/util/remotecommand should be in apimachinery, i see similar comment for port forwarding from @deads2k

dshulyak · 2017-02-15T14:01:09Z

@k8s-bot verify test this

dshulyak · 2017-02-15T14:08:56Z

@sttts @deads2k @caesarxuchao I think this change now in good shape. Please review it when you will have time.

I need to move pkg/util/remotecommand/constants.go to apimachinery (the same as portforward/constants.go) and unify it with kubelet. I will do it in another change. Also I am not sure what is the proper place for pkg/util/exec, so I just placed it in client-go repo

dshulyak · 2017-02-16T08:33:38Z

@k8s-bot test this

dshulyak · 2017-02-16T08:58:34Z

I moved part of this PR into separate one, for easier review, #41543. Will rebase afterwards

caesarxuchao · 2017-04-03T21:41:39Z

Sorry I have been ignoring the PR. I'm taking another look now.

caesarxuchao · 2017-04-04T01:08:21Z

I left a few comments in #41543. Let's get that merged first.

caesarxuchao · 2017-05-10T20:08:17Z

/release-note-none

caesarxuchao · 2017-05-10T20:08:24Z

/lgtm
/approve

caesarxuchao · 2017-05-10T20:08:31Z

Thanks @dshulyak !

caesarxuchao · 2017-05-10T19:44:55Z

staging/src/k8s.io/client-go/util/exec/exec.go

+// ExitError is an interface that presents an API similar to os.ProcessState, which is
+// what ExitError from os/exec is.  This is designed to make testing a bit easier and
+// probably loses some of the cross-platform properties of the underlying library.
+type ExitError interface {


Unfortunately this package is copied. We can remove the original copy in the kubernetes repo, that could be another PR.

@dshulyak let me know if you are up for a follow-up.

@caesarxuchao i copied only parts that were used in remotecommand, do you think it makes sense to move the whole pkg/util/exec? if so, i can do it ofcourse

I see. Can we move ExitError to the remotecmd package? (removing the copy in client-go/util/exec/exec.go and in kubernetes/util/exec/exec.go). In a followup PR is fine. There are not many references to ExitError:

$ git grep -l "CodeExitError" | grep -v pkg/util/exec pkg/client/unversioned/remotecommand/v4.go pkg/kubectl/cmd/run.go pkg/kubectl/cmd/util/helpers_test.go pkg/kubelet/remote/remote_runtime.go test/e2e/framework/util.go

@dshulyak please let me know if you are interested to do the follow up

@caesarxuchao sure, i will do it

Thanks, please assign me to the PR when it's out.

lavalamp · 2017-05-12T22:45:09Z

/approve

caesarxuchao · 2017-05-13T00:03:54Z

@k8s-bot unit test this

caesarxuchao · 2017-05-13T00:04:04Z

@k8s-bot gce etcd3 e2e test this

Module remotecommand originally part of kubernetes/pkg/client/unversioned was moved to client-go/tools, and will be used as authoritative in kubectl, e2e and other places. Module remotecommand relies on util/exec module which will be copied to client-go/pkg/util

client-go/pkt/util was removed in favor of client-go util, which consists only from CodeExitError and ExitError interface

k8s-ci-robot · 2017-05-15T14:40:03Z

@dshulyak: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins Cross Build	1d8cec20d79311546f819bb0248d7b24806282fe	link	`@k8s-bot cross build this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

caesarxuchao · 2017-05-15T17:15:37Z

@k8s-bot kops aws e2e test this

caesarxuchao · 2017-05-15T17:36:52Z

/lgtm

k8s-github-robot · 2017-05-15T17:38:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, dshulyak, lavalamp

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

~~hack/OWNERS~~ [lavalamp]
~~pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [caesarxuchao,lavalamp]
~~test/OWNERS~~ [lavalamp]

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

k8s-github-robot · 2017-05-15T20:25:42Z

Automatic merge from submit-queue (batch tested with PRs 41331, 45591, 45600, 45176, 45658)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 13, 2017

k8s-github-robot assigned mwielgus Feb 13, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Feb 13, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2017

caesarxuchao assigned caesarxuchao and unassigned mwielgus Feb 14, 2017

dshulyak force-pushed the import_remotecommand branch from 3c8532f to 3de6b88 Compare February 14, 2017 12:02

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

caesarxuchao reviewed Feb 14, 2017

View reviewed changes

dshulyak force-pushed the import_remotecommand branch from 1d8cec2 to 6bbd26a Compare February 15, 2017 11:28

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2017

sttts reviewed Feb 15, 2017

View reviewed changes

dshulyak force-pushed the import_remotecommand branch from 6bbd26a to 7952350 Compare February 15, 2017 12:46

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2017

dshulyak force-pushed the import_remotecommand branch from 3ad0b9c to 7da94fb Compare May 10, 2017 13:10

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 10, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2017

caesarxuchao approved these changes May 10, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2017

dshulyak added 4 commits May 15, 2017 16:28

Add exec module to client-go/util only with required code

8574bc5

client-go/pkt/util was removed in favor of client-go util, which consists only from CodeExitError and ExitError interface

Fix golint and godeps verification

c0a3e9a

Add remotecommand to client-go/pkg/api visibility rules

64d98c2

dshulyak force-pushed the import_remotecommand branch from 7da94fb to e8aa65a Compare May 15, 2017 13:29

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 15, 2017

Use streaming constants from pkg/api/v1 instead of pkg/api

a713604

dshulyak force-pushed the import_remotecommand branch from e8aa65a to a713604 Compare May 15, 2017 14:00

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2017

k8s-github-robot merged commit f6bcd28 into kubernetes:master May 15, 2017

dshulyak mentioned this pull request Jun 7, 2017

Move CodeExitError to remocommand package and remove unnecessary copies #47111

Closed

Move client/unversioned/remotecommand to client-go #41331

Move client/unversioned/remotecommand to client-go #41331

Conversation

dshulyak commented Feb 13, 2017 • edited Loading

k8s-reviewable commented Feb 13, 2017

dshulyak commented Feb 13, 2017

dshulyak commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Feb 14, 2017

caesarxuchao commented Feb 14, 2017

sttts commented Feb 14, 2017

dshulyak commented Feb 14, 2017

sttts commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshulyak commented Feb 15, 2017

dshulyak commented Feb 15, 2017 • edited Loading

dshulyak commented Feb 16, 2017

dshulyak commented Feb 16, 2017

caesarxuchao commented Apr 3, 2017

caesarxuchao commented Apr 4, 2017

caesarxuchao commented May 10, 2017

caesarxuchao commented May 10, 2017

caesarxuchao commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented May 12, 2017

caesarxuchao commented May 13, 2017

caesarxuchao commented May 13, 2017

k8s-ci-robot commented May 15, 2017 • edited Loading

caesarxuchao commented May 15, 2017

caesarxuchao commented May 15, 2017

k8s-github-robot commented May 15, 2017

k8s-github-robot commented May 15, 2017

dshulyak commented Feb 13, 2017 •

edited

Loading

dshulyak commented Feb 15, 2017 •

edited

Loading

k8s-ci-robot commented May 15, 2017 •

edited

Loading