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

Issue #3037 change dependency management to dep #3136

Merged
merged 4 commits into from
Sep 26, 2018

Conversation

praveenkumar
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 18, 2018
@praveenkumar
Copy link
Contributor Author

/assign @tstromberg

cc @dlorenc

@praveenkumar
Copy link
Contributor Author

@tstromberg @dlorenc looks like CI stuck, can you folks check once?

@praveenkumar
Copy link
Contributor Author

/test all

@gbraad
Copy link
Contributor

gbraad commented Sep 20, 2018 via email

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

Do you mind updating minikube/docs/contributors/adding_a_dependency.md within this PR, so that our docs aren't mismatched?

@dlorenc
Copy link
Contributor

dlorenc commented Sep 21, 2018

This is awesome! I'll look into CI...

@dlorenc
Copy link
Contributor

dlorenc commented Sep 21, 2018

Looks like a build issue for windows:

vendor/github.com/docker/docker/pkg/term/term_windows.go:35:44: undefined: winterm.ENABLE_VIRTUAL_TERMINAL_INPUT

caused the rest of the build to fail.

@dlorenc
Copy link
Contributor

dlorenc commented Sep 21, 2018

I just added a commit to your PR that looks like it fixes the build!

@dlorenc
Copy link
Contributor

dlorenc commented Sep 21, 2018

@tstromberg could you take a quick look at my two PRs that got the build working?

r2d4
r2d4 previously requested changes Sep 21, 2018
Copy link
Contributor

@r2d4 r2d4 left a comment

Choose a reason for hiding this comment

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

You should also remove the hack/godeps/* scripts.

Gopkg.toml Outdated

[[override]]
name = "k8s.io/api"
version = "kubernetes-1.10.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why you have this 1.10.0 as an override and 1.11.2 as the constraint? All kubernetes libraries should be pointing at the same release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@r2d4 so when I do dep init it always picks the 1.11.2 but after the vendor when you try to compile it always fails with an error. So that is the reason I need to override it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got it all workin with 1.11 with this TOML:
https://gist.github.com/dlorenc/07b36d97d77b5d06299307f43d2a6167

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlorenc why you need to override the same version if it is part of constraint like https://gist.github.com/dlorenc/07b36d97d77b5d06299307f43d2a6167#file-gopkg-toml-L72-L78 and https://gist.github.com/dlorenc/07b36d97d77b5d06299307f43d2a6167#file-gopkg-toml-L88-L94 can you try without override because that what I tried.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean, we need override for non-direct dependencies and constraint for direct ones.

There are some bugs with dep and client-go, so including everything is basically the only way I've ever seen it work.

When you bump to 1.11, you also have to add an override for:

[[override]]
  name = "github.com/json-iterator/go"
  version = "1.1.3-22-gf2b4162"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlorenc I don't think you need to use different for direct and non-direct dependency as per doc https://golang.github.io/dep/docs/Gopkg.toml.html#override this is used only when dependencies, direct and transitive, and supersedes all other [[constraint]] declarations for that project. but I am not much expert in that side :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice. I just added a commit with the jsoniter and 1.11 updates. If tests pass this should be good to go.

Gopkg.toml Outdated

[[constraint]]
name = "k8s.io/client-go"
version = "7.0.0"
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 use the kubernetes-* tag for client-go as well to ensure compatibility.

Gopkg.toml Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: praveenkumar
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: tstromberg

If they are not already assigned, you can assign the PR to them by writing /assign @tstromberg 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

@praveenkumar
Copy link
Contributor Author

You should also remove the hack/godeps/* scripts.

@r2d4 this is also done 👍

@dlorenc
Copy link
Contributor

dlorenc commented Sep 26, 2018

Here we go....

@dlorenc dlorenc dismissed r2d4’s stale review September 26, 2018 18:58

changes addressed

@dlorenc dlorenc merged commit 6f797b6 into kubernetes:master Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants