Remove deprecatedPublicIPs field #44519

thockin · 2017-04-14T22:28:03Z

No description provided.

k8s-reviewable · 2017-04-14T22:29:09Z

This change is

xiangpengzhao · 2017-04-15T07:16:53Z

@thockin Did you generate pkg/api/v1/types.generated.go correctly? It's an empty file in your PR and it leads to the tests failure.

xiangpengzhao · 2017-04-15T07:19:54Z

federation/apis/federation/v1beta1/types.generated.go

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


Please check this generated file:)

xiangpengzhao · 2017-04-15T07:21:33Z

examples/k8petstore/k8petstore.sh

@@ -141,7 +141,7 @@ cat << EOF > fe-s.json
    "ports": [{
      "port": 3000
    }],
-    "deprecatedPublicIPs":["$PUBLIC_IP","10.1.4.89"],
+    "externalIPs":["$PUBLIC_IP","10.1.4.89"],


Also change the name of $PUBLIC_IP to EXTERNAL_IP appears in this script?

thockin · 2017-04-16T20:06:16Z

@caesarxuchao I don't know what to do wrt staging updates. I ran update-staging-client (update-all) and it made a bunch of changes like the following, which seem wrong.

@@ -15,11 +15,24 @@ go_library(
     ],
     tags = ["automanaged"],
     deps = [
+        "//pkg/apis/certificates:go_default_library",
+        "//pkg/apis/certificates/v1beta1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
-        "//vendor/k8s.io/client-go/pkg/apis/certificates:go_default_library",
-        "//vendor/k8s.io/client-go/pkg/apis/certificates/v1beta1:go_default_library",
         "//vendor/k8s.io/client-go/tools/cache:go_default_library",
     ],
 )

caesarxuchao · 2017-04-17T17:24:55Z

The Jenkins verification passed anyway because we ignore the BUILD file diffs: https://github.com/kubernetes/kubernetes/blob/master/staging/copy.sh#L220-L221. @thockin if you run hack/update-bazel.sh again, are those changes cancelled?

I don't know which script is managing the BUILD files in the staging folder, @mikedanese do you know?

thockin · 2017-04-17T17:32:06Z

I manually reverted the obviously broken BUILD changes. This is not a good developer experience. I made a simple API change, I had to run godep restore, manually revert BUILD files, and mess with my GOPATH. A casual contributor would have given up. *I* almost gave up.

…

On Mon, Apr 17, 2017 at 10:25 AM, Chao Xu ***@***.***> wrote: The Jenkins verification passed anyway because we ignore the BUILD file diffs: https://github.com/kubernetes/kubernetes/blob/master/ staging/copy.sh#L220-L221. @thockin <https://github.com/thockin> if you run hack/update-bazel.sh again, are those changes cancelled? I don't know which script is managing the BUILD files in the staging folder, @mikedanese <https://github.com/mikedanese> do you know? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44519 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVHZvY0vtXa4dLYA1KXKJ3LLgnFKAks5rw6B7gaJpZM4M-JcP> .

bowei · 2017-04-17T17:36:04Z

/lgtm

k8s-github-robot · 2017-04-17T17:36:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, thockin

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

~~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

mikedanese · 2017-04-17T17:38:11Z

Gahh, that's a bug in how ./staging/copy.sh handles BUILD files, ./hack/update-bazel.sh is canonical. I'll take a look.

caesarxuchao · 2017-04-17T17:46:01Z

@mikedanese it seems the problem is exposed by #40777

thockin · 2017-04-17T17:48:06Z

Chao: the staging scripts assume they can call godep-restore, but without a clean GOPATH that can fail in a hundred different ways, leaving the user confused and frustrated. It really needs to make a temporary space, change GOPATH, restore into that, then process. It sucks. We should not be asking users to do this.

…

On Mon, Apr 17, 2017 at 10:38 AM, Mike Danese ***@***.***> wrote: Gahh, that's a bug in how ./staging/copy.sh handles BUILD files, ./hack/update-bazel.sh is canonical. I'll take a look. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44519 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVJCx5dOkL-xeELaDOgj4u6rH-V0Uks5rw6OXgaJpZM4M-JcP> .

k8s-github-robot · 2017-04-17T19:10:59Z

Automatic merge from submit-queue

caesarxuchao · 2017-04-17T19:44:14Z

@thockin the update-staging-client-go.sh verifies the working dir is clean before proceeding. Or do you mean other things by "clean GOPATH"?

It really needs to make a temporary space, change GOPATH, restore into that, then process

We had considered this, but it will take too long to run.

thockin · 2017-04-17T20:45:37Z

On Mon, Apr 17, 2017 at 12:44 PM, Chao Xu ***@***.***> wrote: @thockin the update-staging-client-go.sh verifies the working dir is clean before proceeding. Or do you mean other things by "clean GOPATH"?

If you have github.com/foobar in your own GOPATH, and then you do a `godep restore` which includes that same package at a different rev it can fail in a dozen different ways. What I had to do to make it pass was: mkdir _gopath GOPATH=`pwd`/_gopath ./hack/godep-restore.sh GOPATH=`pwd`/_gopath:$GOPATH ./hack/update-staging-client.sh

It really needs to make a temporary space, change GOPATH, restore into that, then process We had considered this, but it will take too long to run.

The alternative is that it fails in hard to comprehend ways. We should not be making users do this at all.

caesarxuchao · 2017-04-19T00:21:21Z

If you have github.com/foobar in your own GOPATH, and then you do a
godep restore which includes that same package at a different rev it
can fail in a dozen different ways.

godep restore fails in such an environment regardless of the staging folder. Currently hack/update-staging-client.sh and hack/update-staging-godeps.sh requires running godep restore. I target to eliminate the need for update-staging-client.sh this quarter. For update-staging-godeps.sh, I think @sttts once had a version of that script that operates in a temporary space, we can add it back as another alternative to running hack/godep-restore.sh, if it's easy.

In general, i think we should focus on eliminating the staging folder, instead of building more tools around it.

thockin assigned bowei Apr 14, 2017

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

thockin added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 14, 2017

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

xiangpengzhao reviewed Apr 15, 2017

View reviewed changes

federation/apis/federation/v1beta1/types.generated.go Outdated

@@ -1,2469 +0,0 @@

/*

Copy link

Contributor

xiangpengzhao Apr 15, 2017

Choose a reason for hiding this comment

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

Please check this generated file:)

xiangpengzhao reviewed Apr 15, 2017

View reviewed changes

thockin force-pushed the remove-deprecated-public-ips branch from bcc58c2 to 54447d1 Compare April 15, 2017 19:25

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 15, 2017

thockin force-pushed the remove-deprecated-public-ips branch from 54447d1 to adfb2ca Compare April 15, 2017 20:04

k8s-github-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 15, 2017

Remove deprecatedPublicIPs field

9153bfa

thockin force-pushed the remove-deprecated-public-ips branch from adfb2ca to 9153bfa Compare April 15, 2017 20:16

--amend

fa93290

bowei approved these changes Apr 17, 2017

View reviewed changes

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

mikedanese mentioned this pull request Apr 17, 2017

./staging/copy.sh doesn't handle BUILD files correctly #44567

Closed

k8s-github-robot merged commit ca8f1bc into kubernetes:master Apr 17, 2017

caesarxuchao mentioned this pull request Apr 24, 2017

Removing hack/udpate-staging* from update-all.sh #44829

Closed

cblecker mentioned this pull request Apr 24, 2017

Improving or replacing godep for dependency vendoring #44873

Closed

thockin deleted the remove-deprecated-public-ips branch August 14, 2019 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecatedPublicIPs field #44519

Remove deprecatedPublicIPs field #44519

thockin commented Apr 14, 2017

k8s-reviewable commented Apr 14, 2017

xiangpengzhao commented Apr 15, 2017

xiangpengzhao Apr 15, 2017

xiangpengzhao Apr 15, 2017

thockin commented Apr 16, 2017

caesarxuchao commented Apr 17, 2017

thockin commented Apr 17, 2017 via email

bowei commented Apr 17, 2017

k8s-github-robot commented Apr 17, 2017

mikedanese commented Apr 17, 2017

caesarxuchao commented Apr 17, 2017

thockin commented Apr 17, 2017 via email

k8s-github-robot commented Apr 17, 2017

caesarxuchao commented Apr 17, 2017

thockin commented Apr 17, 2017 via email

caesarxuchao commented Apr 19, 2017

Remove deprecatedPublicIPs field #44519

Remove deprecatedPublicIPs field #44519

Conversation

thockin commented Apr 14, 2017

k8s-reviewable commented Apr 14, 2017

xiangpengzhao commented Apr 15, 2017

xiangpengzhao Apr 15, 2017

Choose a reason for hiding this comment

xiangpengzhao Apr 15, 2017

Choose a reason for hiding this comment

thockin commented Apr 16, 2017

caesarxuchao commented Apr 17, 2017

thockin commented Apr 17, 2017 via email

bowei commented Apr 17, 2017

k8s-github-robot commented Apr 17, 2017

mikedanese commented Apr 17, 2017

caesarxuchao commented Apr 17, 2017

thockin commented Apr 17, 2017 via email

k8s-github-robot commented Apr 17, 2017

caesarxuchao commented Apr 17, 2017

thockin commented Apr 17, 2017 via email

caesarxuchao commented Apr 19, 2017