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

Update golang.org/x/net/... dependencies to release-branch.go1.11 #71501

Closed
wants to merge 3 commits into from

Conversation

PrasadG193
Copy link
Contributor

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Updates vendored files in golang.org/x/net/... packages to release-branch.go1.11

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #71016

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 28, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @PrasadG193. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 28, 2018
@k8s-ci-robot k8s-ci-robot added area/apiserver kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 28, 2018
@PrasadG193
Copy link
Contributor Author

/assign @wenjiaswe

@wenjiaswe
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 28, 2018
@wenjiaswe
Copy link
Contributor

/retest

@wenjiaswe
Copy link
Contributor

@PrasadG193 Thank you very much for helping with this! There are several tests failing now. I looked at this dependency upgrade before and unfortunately, this one is a little bit trickier than other dependency upgrade. In go/net 1.11, there are some structural changes that might have big impact, which means you will need to do some manual fix carefully before all the tests could pass.
Below is one structural change in go/net release-branch1.11 that I know of, there might be more,
lex/httplex, http/httpguts: merge the httplex package into httpguts

},
{
"ImportPath": "golang.org/x/net/internal/socks",
"Rev": "c39426892332e1bb5ec0a434a079bf82f5d30c54"
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in my comment, you can see that the httpplex package is not here anymore and httpguts and socks are added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Let me do the same in other Godep.json files

Copy link
Contributor

Choose a reason for hiding this comment

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

I would imagine other Godep.json files should be handled fine by the script? It's more of the actual source code who has dependency on those files, you probably need to go in there and modify them. Does the code compile after you run the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wenjiaswe We need to update this dependency in all the repos under https://github.com/kubernetes/kubernetes/tree/master/vendor/k8s.io. Should we create a PR against each repo? And I guess these tests will not pass until we merge changes in vendored repos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @wenjiaswe

Copy link
Contributor

Choose a reason for hiding this comment

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

@PrasadG193 Sorry for the delay! I was busy with KubeCon and just caught up with this. Actually, what you need to do is to update files in https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io in this PR, instead of vendors. cc @dims for confirmation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wenjiaswe we have already updated dependencies in staging/src/k8s.io/* files in this PR. But the hack/verify-godeps.sh script is failing as repos in vendor/k8s.io/* are still using older version of golang.org/x/net/... packages

Copy link
Member

Choose a reason for hiding this comment

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

@PrasadG193 please run hack/update-staging-godeps.sh and also hack/update-godep-licenses.sh

@PrasadG193 PrasadG193 force-pushed the update-http2-dep branch 3 times, most recently from 515aa2e to ba675e1 Compare November 29, 2018 16:02
@wenjiaswe
Copy link
Contributor

/lgtm
/approve
Yeah! Finally! @dims, would you please help to take a look and make sure everything is good?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PrasadG193, wenjiaswe
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers: cblecker, thockin

If they are not already assigned, you can assign the PR to them by writing /assign @cblecker @thockin in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@dims
Copy link
Member

dims commented Feb 27, 2019

/assign @cblecker @thockin

Please see the hack -i added to godep to ignore stale packages in old dependencies (we pull in new versions of the dependencies but during computation dep trips up).

@cblecker
Copy link
Member

/hold

Need to review this one closely.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2019
@cblecker
Copy link
Member

cblecker commented Mar 1, 2019

@dims I'm not sure that hack is really safe.. I don't like the idea of manually ignoring packages, if they're being brought into the dependency tree. Godep is of course crap at telling you why a particular package is needed, but I'm guessing it's bringing in those few repos that have Godeps.json of their own and using that to say "these must have dependencies" even if that package isn't in the actual code path for the packages we import?

@sttts If you have time today, would you mind taking a look at this? I could use a second opinion.

@dims
Copy link
Member

dims commented Mar 1, 2019

@cblecker @sttts we are ignoring the failure during processing. the old package name is not used in any of our direct or indirect dependencies, only references are within the x/net package. and we are trying to go to a newer x/net package.

@thockin
Copy link
Member

thockin commented Mar 1, 2019

What's blocking upgrading x/net at the same time?

@dims
Copy link
Member

dims commented Mar 1, 2019

@thockin short answer godep :)

godep does not seem to understand that we want a fresh x/net and is using the older x/net from vendored directories when it is computing dependencies. So i had to add a -i to godep to skip the httplex package that does not exist in the latest x/net. Then i had to explicitly add the latest x/net as a work around.

@liggitt
Copy link
Member

liggitt commented Mar 5, 2019

The external facing bits of golang.org/x/net in go1.12 and go1.11 are API compatible. It's purely a matter of convincing godeps to pull in the right version.

I think this is likely the straw that breaks the camel's back for pushing us from godep to go modules (hopefully in 1.15), which handled updating to a go1.12 version of golang.org/x/net just fine. See #74877 for experiments in that vein.

I don't object to a hack to get k/k vendor dir updated to go1.12-level golang.org/x/net (especially if we expect the lifetime of the hack to be short with go modules imminent), but I am concerned that the resulting Godep.json files we push out to published repos will not work with a standard Godep.

@k8s-ci-robot
Copy link
Contributor

@PrasadG193: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2019
@dims
Copy link
Member

dims commented Mar 12, 2019

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Mar 12, 2019
@dims
Copy link
Member

dims commented Mar 12, 2019

@PrasadG193

  • Can you please update to use go1.12?
  • Can you please rebase to latest master (and resolve conflicts)?

@dims
Copy link
Member

dims commented Mar 12, 2019

@PrasadG193 i've filed another PR #75289 that includes the commits from here and the update to 1.12. is this ok?

@liggitt
Copy link
Member

liggitt commented Mar 12, 2019

/close
in favor of #75289

@k8s-ci-robot
Copy link
Contributor

@liggitt: Closed this PR.

In response to this:

/close
in favor of #75289

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update golang.org/x/net/http2 vendored files
7 participants