Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

[WIP] vendor: Replace Godep with glide #1335

Closed
wants to merge 1 commit into from

Conversation

nhlfr
Copy link

@nhlfr nhlfr commented Oct 12, 2016

Fixes #1314


This change is Reviewable

@piosz piosz self-assigned this Oct 13, 2016
@piosz
Copy link
Contributor

piosz commented Oct 14, 2016

Did you bump any deps? I'd expect this change to be transparent in term of added/removed files.

@DirectXMan12
Copy link
Contributor

Perhaps glide isn't doing the filtering based on used vs unused files, like Godep does? Do we need glide-vc?

@AlmogBaku
Copy link
Contributor

@DirectXMan12 @piosz @nhlfr as I wrote in #1314 there is no need to commit vendor directory at all.. that's one of the great benefits of glide.. specify semver versions for deps, and just do glide install and voila!

@nhlfr
Copy link
Author

nhlfr commented Oct 19, 2016

That's right. I'm going to remove the vendor dir today.

Copy link
Contributor

@AlmogBaku AlmogBaku left a comment

Choose a reason for hiding this comment

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

IMO, from my experience there is no need to commit vendor directory at all.

@@ -8,25 +8,26 @@ SUPPORTED_KUBE_VERSIONS = "1.3.6"
TEST_NAMESPACE = heapster-e2e-tests

deps:
which godep || go get github.com/tools/godep
which glide || go get github.com/Masterminds/glide
glide update
Copy link
Contributor

Choose a reason for hiding this comment

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

should be glide install

@AlmogBaku
Copy link
Contributor

@nhlfr can you fix the PR? it looks like the tests failed

@nhlfr nhlfr force-pushed the glide branch 2 times, most recently from 0f15bd7 to b602f43 Compare October 26, 2016 07:04

sanitize:
hooks/check_boilerplate.sh
hooks/check_gofmt.sh
hooks/run_vet.sh

test-unit: clean deps sanitize build
GOOS=linux GOARCH=amd64 godep go test --test.short -race ./... $(FLAGS)
GOOS=linux GOARCH=amd64 go test --test.short -race ./... $(FLAGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be

GOOS=linux GOARCH=amd64 go test --test.short -v -race `glide novendor` $(FLAGS)

This will prevent testing the vendored libs.

which godep || go get github.com/tools/godep
verify-glide-installation:
which glide || go get github.com/Masterminds/glide
which glide-vc || go get github.com/sgotti/glide-vc
Copy link
Contributor

Choose a reason for hiding this comment

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

why is that needed if we are not going to commit the vendor dir?

@nhlfr
Copy link
Author

nhlfr commented Nov 9, 2016

Looks like glide is using raw hg to fetch a repo from bitbucket...

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 04895e3. Full PR test history.

The magic incantation to run this job again is @k8s-bot test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@AlmogBaku
Copy link
Contributor

if im not mistaken so as godep... so why usage of hg natively here doesnt work?

@nhlfr
Copy link
Author

nhlfr commented Nov 14, 2016

Probably go get and godep don't use hg as an executed command - that's my assumption, I cannot imagine any other scenario.

However, I'll try to think about and propose a change to k8s infra.

@AlmogBaku
Copy link
Contributor

AlmogBaku commented Nov 20, 2016

sounds very strange... since im pretty sure that godep required me to install hg..
can we re-run it again? or at least just fix the integration test?

@AlmogBaku
Copy link
Contributor

ping @nhlfr

@piosz
Copy link
Contributor

piosz commented Jan 11, 2017

@luxas

@luxas
Copy link
Contributor

luxas commented Jan 11, 2017

I would not switch to Glide in any k8s official projects before we have done it for core (if we really want that, I'm not sure). Actually I'm in favor for committing the vendor dir into the project, because once you've downloaded the project with git, you don't have to fetch it with glide in scripts or manually.

cc @thockin for thoughts on glide

This will not at least make the next release, and I don't think there is any argument for doing so either.

@thockin
Copy link
Contributor

thockin commented Jan 12, 2017 via email

@piosz
Copy link
Contributor

piosz commented Jan 12, 2017

As I wrote in #1314 (comment), I think Heapster should follow Kubernetes standards. According to what @thockin we want to stay with godeps for a while. Once Kubernetes is migrated we will revisit this decision. Closing the PR in the meantime.

@piosz piosz closed this Jan 12, 2017
@piosz piosz mentioned this pull request Jan 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants