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

Switch to dep for dependency management #78

Closed
lavalamp opened this Issue Jan 25, 2017 · 52 comments

Comments

Projects
None yet
@lavalamp
Copy link
Member

lavalamp commented Jan 25, 2017

https://github.com/golang/dep

I.e., publish whatever dep needs instead of Godeps.json, and change documentation to tell people to download via dep ensure. (After testing to make sure it works--feel free to make a temporary branch if that helps testing.)

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 1, 2017

Switching to dep or glide will enable an alternative to this proposal on how to publish client-go.

We set up the manifest of dep (or glide) to always vendor the latest version of k8s.io/apimachinary. We set up the publishing robot to publish to k8s.io/apimachinary first, so that a real commit exists in the apimachinary repo; then the publishing robot copies staging/client-go, and run dep ensure k8s.io/apimachinary, which pulls down all the latest commits in k8s.io/apimachinary and overwrites the vendor/ folder, then the robot publishes to k8s.io/client-go.

Did I miss something? I'll experiment with this approach later tonight. @deads2k @lavalamp @stts.

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 1, 2017

We set up the manifest of dep (or glide) to always vendor the latest version of k8s.io/apimachinary. We set up the publishing robot to publish to k8s.io/apimachinary first, so that a real commit exists in the apimachinary repo; then the publishing robot copies staging/client-go, and run dep ensure k8s.io/apimachinary, which pulls down all the latest commits in k8s.io/apimachinary and overwrites the vendor/ folder, then the robot publishes to k8s.io/client-go.

I think the idea is good. Assuming one pusher (the bot) or a coordinated pusher (one of us manually), this should guarantee a matched set assuming client-go has a similar sync script.

For branches, you'll need to restrict to a branch.

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 1, 2017

I don't think client-go having a similar sync script is a requirement for this idea to work. But apimachinery's sync script does some nice things like keeping the history, so I'll migrate client-go's script to yours.

I'll implement the necessary changes for the robot to implement the idea.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Feb 2, 2017

@caesarxuchao I like the idea. I don't see the connection though to golang/dep. We can do the same with Godeps now (if golang/dep support takes some time).

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 2, 2017

Yeah, I'm implementing it with Godeps now.

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 2, 2017

I manually experimented the idea, it's working. The robot is mostly done, but I stuck at running https://github.com/kubernetes/apimachinery/blob/master/hack/sync-from-kubernetes.sh, it keeps complaining fatal: Invalid revision range acda1e7401fbf79d89ba23df5e11d81f4a9d4e0d..HEAD at this line. git cannot find the old filter-branch-sha in the newly filtered branch. @sttts @deads2k could you help?

The only change I made to the script it fetching from https://github.com/kubernetes/kubernetes.git instead of git@github.com:kubernetes/kubernetes.git. [update] It also failed if fetching from the original ssh address. I ran the script from the repo root of k8s.io/apimacinery.

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

I manually experimented the idea, it's working. The robot is mostly done, but I stuck at running https://github.com/kubernetes/apimachinery/blob/master/hack/sync-from-kubernetes.sh, it keeps complaining fatal: Invalid revision range acda1e7401fbf79d89ba23df5e11d81f4a9d4e0d..HEAD at this line. git cannot find the old filter-branch-sha in the newly filtered branch. @sttts @deads2k could you help?

The repo isn't a straight filter branch. The script as a for instance, changes the SHAs. So does the metadata about what it matches.

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 2, 2017

Sorry, I don't follow you. What should I do to run that script successfully?

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

Sorry, I don't follow you. What should I do to run that script successfully?

Sorry, I misread what you were doing. My log on the kube-sync branch post-filter branch looks like this: https://gist.github.com/deads2k/7ce8f3a2dbcb564a8b369c782625800e What does yours look like?

@caesarxuchao

This comment has been minimized.

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 2, 2017

The SHAs are different, commit messages are the same. Also, did you miss the lines after "d3c7cb5 Move first pkg/api/validation's into apimachinery"?

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

The SHAs are different, commit messages are the same. Also, did you miss the lines after "d3c7cb5 Move first pkg/api/validation's into apimachinery"?

Yours handles the signed commits better. Do I have an old git maybe?

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 2, 2017

$ git version
git version 2.11.0.390.gc69c2f50cf-goog

And I'm on OSX 10.11.6

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

Fedora

[deads@deads-dev-01 apimachinery]$ git version
git version 2.5.5
@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

well, this going to be a problem.

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

Ok, found http://stackoverflow.com/questions/31552774/unexpected-behavior-with-git-filter-branch-and-signed-commits . Looks like mine is working correctly even if its ugly, since I changed the diff of the patch with filter-branch and logically the owner didn't sign it anymroe.

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 2, 2017

The robot is using git version 2.1.4, and it's producing the same git log output [edit] as my local git.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Feb 2, 2017

@caesarxuchao will you put most of the tooling about the godep update into the kube repo?

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

So I have git version 2.5.5, chao has 2.11. 2.1 worked, 2.11 worked, 2.5.5 produces weird commits. We shouldn't rewrite history, but we can correct since the metadata is on disk. We should correct the filter-branch sha and use a working version of git moving forward.

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

Alright, I've updated my git version, corrected the filter-branch-sha, and pushed a new sync. I did not rewrite history, so we've got some ugly commit messages we'll have to live with for all eternity.

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 2, 2017

@lavalamp @smarterclayton @liggitt in the experiment you've created here: https://github.com/lavalamp/client-go-flat/, how would another library writer who generated his clients using our client generator against the apimachinery repo go about creating a compatible client? The signatures for anything that uses an apimachinery type like RESTClient https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/client.go#L61-L89 will be incompatible.

As we add aggregated API servers, we expect this to be more common, not less, correct?

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Feb 2, 2017

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Feb 3, 2017

Yes, my experiment is aimed at people who don't want to think about
vendoring. People who want to write an apiserver will have to vendor.

But at the same time, the experiment makes it impossible for someone writing their API server to provide a client of their own for people who don't to think about it. Basically, it means that no aggregated API server will be able to have my-aggregated-apiserver/client-go because the types we generate for them are incompatible.

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 3, 2017

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Apr 24, 2017

Some issues that may be blocking:

  • Dep doesn't remove nested vendor directories golang/dep#120
  • Dep can't detect transitive dependencies of client-go. Need to either:
    • Publish a dep manifest once that format is stable golang/dep#276
    • Wait for glide to support detecting other vendor tool manifests golang/dep#380

Anything else anyone's hit?

@sathieu

This comment has been minimized.

Copy link

sathieu commented Feb 9, 2018

Hi,
Any news on this?

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Feb 9, 2018

Any news on this?

Yes, we are actively working on creating a Gopkg.toml inside of client-go during publishing. Some more patience please. We finally make progress.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 11, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Mar 11, 2018

/remove-lifecycle rotten

@AkihiroSuda

This comment has been minimized.

Copy link

AkihiroSuda commented Mar 29, 2018

Any update?

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Mar 29, 2018

Yes, the publishing-bot can publish Gopkg.toml (kubernetes/publishing-bot#55), but we are blocked on dep.

@ryanwalls

This comment has been minimized.

Copy link

ryanwalls commented Apr 16, 2018

If you get to this issue and want to follow the workaround of specifying overrides for all transitive dependencies, here are the overrides for release 7.0. https://gist.github.com/ryanwalls/96ffc7a218c05cfdfcdecdaf413b9e13

  • Update: dep doesn't like the list having non-root projects... wait one while I fix it.
  • Update 2: Okay, the gist has the dependency overrides that actually worked.
@lvangool

This comment has been minimized.

Copy link

lvangool commented May 9, 2018

Hmm - that gist is not working for me - I'm seeing:

$ dep ensure
Solving failure: No versions of k8s.io/client-go met constraints:
v7.0.0: Could not introduce k8s.io/client-go@v7.0.0 due to multiple problematic subpackages:
Subpackage k8s.io/client-go/pkg/api/v1 is missing. (Package is required by (root).)	
Subpackage k8s.io/client-go/pkg/apis/batch/v1 is missing. (Package is required by (root).)
Subpackage k8s.io/client-go/pkg/apis/extensions/v1beta1 is missing. (Package is required by (root).)
v6.0.0: Could not introduce k8s.io/client-go@v6.0.0, as it is not allowed by constraint ^7.0.0 from project ...
... 

Having so much pain with go-client deps!

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented May 9, 2018

So, it seems the go team mostly considers dep a failed experiment. We'll have to continue to live in a world of pain until golang/go#24301 rolls out and we figure out how to start publishing packages that play nice.

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented May 9, 2018

I filed kubernetes/kubernetes#63607 to call more attention to this.

@briansmith

This comment has been minimized.

Copy link

briansmith commented May 9, 2018

So, it seems the go team mostly considers dep a failed experiment.

I agree.

We'll have to continue to live in a world of pain until golang/go#24301 rolls out and we figure out how to start publishing packages that play nice.

I don't agree with this. It would be very good for client-go to be made to work well with dep-based build systems in the interim. We shouldn't underestimate how long it will take for vgo to become a real thing; it seems very likely it will take a long time to happen, and it seems pretty likely it will be abandoned as another "failed experiment."

It is seriously painful to upgrade client-go in dep-based projects so projects like the ones I work on avoid doing so unless/until absolutely necessary. This is bad and is worth fixing.

@briansmith

This comment has been minimized.

Copy link

briansmith commented May 9, 2018

At the very least, in the event of a security vulnerability that gets fixed by releasing new version(s) of client-go, it is important to provide the workarounds needed to get dep-based software to be able to do the upgrade at the same time the new release is made. Otherwise lots of software won't be able to consume the fix in a reasonable amount of time.

@sdboyer

This comment has been minimized.

Copy link

sdboyer commented May 9, 2018

that is indeed Russ' opinion. I have a number of thoughts about what k8s can do that will roll sanely into the future. unfortunately, all my time is right now is taken up by working on vgo response blog posts 😢

imma be publishing the first of those this week, though, so my time will start unblocking soon.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Sep 3, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Oct 3, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@adinunzio84

This comment has been minimized.

Copy link

adinunzio84 commented Oct 8, 2018

Any update here? Has client-go abandoned dep (even in the interim until vgo is stable)?

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 7, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 7, 2018

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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