move term to kubectl/util #48299

mengqiy · 2017-06-29T22:19:04Z

move term from pkg/util/term to pkg/kubectl/util/term

remove dependency of k8s.io/kubernetes/pkg/util/term for pkg/kubelet/dockershim/exec.go and pkg/kubelet/dockershim/exec.go

Ref: #48209

NONE

/assign @apelisse @monopole

cc: @pwittrock

apelisse

Thanks mengqi!

apelisse · 2017-06-29T22:19:57Z

pkg/kubectl/cmd/BUILD

@@ -218,7 +218,6 @@ go_test(
        "//pkg/printers/internalversion:go_default_library",
        "//pkg/util/i18n:go_default_library",
        "//pkg/util/strings:go_default_library",
-        "//pkg/util/term:go_default_library",


the test didn't need that? weird.

apelisse · 2017-06-29T22:20:44Z

pkg/kubectl/util/BUILD

-    srcs = [":package-srcs"],
+    srcs = [
+        ":package-srcs",
+        "//pkg/kubectl/util/term:all-srcs",


This will conflict with your other pull-requests in flight, but I guess that's ok

Thanks for reminding me :)

apelisse · 2017-06-29T22:22:02Z

pkg/kubelet/dockershim/BUILD

        "//vendor/github.com/blang/semver:go_default_library",
        "//vendor/github.com/docker/docker/pkg/jsonmessage:go_default_library",
+        "//vendor/github.com/docker/docker/pkg/term:go_default_library",


Question: why can we use the external reference in kubelet and not in kubectl?

I think it's ok to vendor 3rd party code, but not ok for kubectl to depend on main k8s repo.

I guess my question is, is //vendor/github.com/docker/docker/pkg/term very different from //pkg/util/term ? Can we replace //pkg/util/term entirely with //vendor/github.com/docker/docker/pkg/term?

kubelet (not kubectl) already depend on docker. I think it should be fine.

apelisse · 2017-06-30T00:24:57Z

/lgtm

thockin · 2017-06-30T15:41:45Z

/approve

mengqiy · 2017-06-30T22:06:29Z

Rebased to fix BUILD file conflicts.

mengqiy · 2017-06-30T22:06:45Z

@apelisse PTAL

apelisse · 2017-06-30T22:08:19Z

/lgtm

k8s-github-robot · 2017-06-30T22:08:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, mengqiy, thockin

Associated issue: 48209

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

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

~~pkg/OWNERS~~ [thockin]

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-07-01T01:42:41Z

Automatic merge from submit-queue (batch tested with PRs 47918, 47964, 48151, 47881, 48299)

luxas · 2017-07-04T10:43:01Z

@mengqiy This broke the cross-build, ref: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-cross-build/2251

Should I revert or will you fix-forward?

mengqiy · 2017-07-04T18:42:13Z

I'd like to know why this could break cross build first. This is no functionality change in this PR.

luxas · 2017-07-04T19:25:40Z

@mengqiy Yes, there is. You're changing the dep from a k8s one (pkg/util/term) to a docker one.
The k8s term package includes compile support for windows, the docker one doesn't.
See https://github.com/moby/moby/blob/master/pkg/term/winsize.go where SetWinSize() is defined; no build support for windows and no winsize_windows.go or winsize_unsupported.go files either.

cc @kubernetes/sig-node-pr-reviews
I think we should be much more careful when actually changing the deps of something.
This PR is indeed changing behavior. If you had only moved packages it wouldn't have a problem.

luxas · 2017-07-04T19:26:19Z

Please answer on revert vs forward-fix. The cross-build must stay healthy.

mengqiy · 2017-07-05T00:34:12Z

Thanks for your explaination.
I will try to give a forward-fix tomorrow morning.

luxas · 2017-07-05T09:30:08Z

Thanks @mengqiy!
Please cc me on the PR

mkumatag · 2017-07-05T15:14:44Z

Because of this PRs with pull-kubernetes-cross are failing. @mengqiy Can you fix the issue on priority basis?

mengqiy · 2017-07-05T18:03:11Z

Potential fix in #48508.

Automatic merge from submit-queue fix cross build Fix the issue introduced in #48299 which breaks cross-build (#48299 (comment)). move setsize.go and setsize_unsupported.go back to util/term for kubelet. move unmark_windows.go as well. ```release-note NONE ```

k8s-ci-robot assigned apelisse and monopole Jun 29, 2017

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

apelisse reviewed Jun 29, 2017

View reviewed changes

mengqiy mentioned this pull request Jun 29, 2017

Find home for utils used by kubectl #48209

Closed

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 29, 2017

mengqiy force-pushed the kubectl_term branch 2 times, most recently from 2adaae1 to bcbda81 Compare June 29, 2017 23:21

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

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 30, 2017

move term to kubectl/util

f0ce897

mengqiy force-pushed the kubectl_term branch from bcbda81 to f0ce897 Compare June 30, 2017 22:01

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 Jun 30, 2017

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

k8s-github-robot merged commit 73a94ea into kubernetes:master Jul 1, 2017

mengqiy deleted the kubectl_term branch July 1, 2017 05:56

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jul 4, 2017

mkumatag mentioned this pull request Jul 5, 2017

Move test-webserver from contrib/for-demos to kubernetes/test/images #48494

Merged

mengqiy mentioned this pull request Jul 5, 2017

fix cross build for util/term #48508

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move term to kubectl/util #48299

move term to kubectl/util #48299

mengqiy commented Jun 29, 2017

apelisse left a comment

apelisse Jun 29, 2017

mengqiy Jun 29, 2017

apelisse Jun 29, 2017

mengqiy Jun 29, 2017

apelisse Jun 29, 2017

mengqiy Jun 29, 2017

apelisse Jun 30, 2017

mengqiy Jun 30, 2017

apelisse commented Jun 30, 2017

thockin commented Jun 30, 2017

mengqiy commented Jun 30, 2017

mengqiy commented Jun 30, 2017

apelisse commented Jun 30, 2017

k8s-github-robot commented Jun 30, 2017

k8s-github-robot commented Jul 1, 2017

luxas commented Jul 4, 2017

mengqiy commented Jul 4, 2017

luxas commented Jul 4, 2017

luxas commented Jul 4, 2017

mengqiy commented Jul 5, 2017

luxas commented Jul 5, 2017

mkumatag commented Jul 5, 2017

mengqiy commented Jul 5, 2017

move term to kubectl/util #48299

move term to kubectl/util #48299

Conversation

mengqiy commented Jun 29, 2017

apelisse left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Jun 30, 2017

thockin commented Jun 30, 2017

mengqiy commented Jun 30, 2017

mengqiy commented Jun 30, 2017

apelisse commented Jun 30, 2017

k8s-github-robot commented Jun 30, 2017

k8s-github-robot commented Jul 1, 2017

luxas commented Jul 4, 2017

mengqiy commented Jul 4, 2017

luxas commented Jul 4, 2017

luxas commented Jul 4, 2017

mengqiy commented Jul 5, 2017

luxas commented Jul 5, 2017

mkumatag commented Jul 5, 2017

mengqiy commented Jul 5, 2017