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

changing to modules from dependencies #869

Merged
merged 6 commits into from
Dec 21, 2019

Conversation

prary
Copy link
Contributor

@prary prary commented Nov 19, 2019

Fixes #838

Description

changing to modules from dependencies

@prary
Copy link
Contributor Author

prary commented Nov 19, 2019

build succeed locally with go version 1.13.3

@cvgw
Copy link
Contributor

cvgw commented Nov 25, 2019

Hi @prary thanks for the PR (very happy to see this change)! I took a look at the travis failures and it looks like at least one failure was caused by the name change on the highwayhash pkg. It appears the vendored package still has the old name https://github.com/GoogleContainerTools/kaniko/tree/6e77b77d7ca7289593a393740eb08411d5d3acec/vendor/github.com/minio/HighwayHash

Let me know if there is anything I can do to help. Cheers

@prary
Copy link
Contributor Author

prary commented Dec 4, 2019

Hi @cvgw intentionally that change is added as Highwayhash breaks with the latest go and latest highwayhash version. Hence I had uptick the golang version as well.

@cvgw
Copy link
Contributor

cvgw commented Dec 4, 2019

@prary I apologize for not being more clear in my first message

I see that as part of updating kaniko to use modules the name of the highwayhash pkg has changed. I also see that you updated the import reference for that package.

However, it looks like the pkg in /vendor still has the old name HighwayHash. Maybe this is unrelated to why the travis build is failing, but it seemed like perhaps it could be part of the problem.

@cvgw
Copy link
Contributor

cvgw commented Dec 4, 2019

These lines from the travis build are what caught my eye

github.com/minio/highwayhash (cannot find package "github.com/minio/highwayhash" in any of:
	/home/travis/gopath/src/github.com/GoogleContainerTools/kaniko/vendor/github.com/minio/highwayhash (vendor tree)
	/home/travis/.gimme/versions/go1.13.3.linux.amd64/src/github.com/minio/highwayhash (from $GOROOT)
	/home/travis/gopath/src/github.com/minio/highwayhash (from $GOPATH)) and 827 more errors: run `golangci-lint run --no-config --disable-all -E typecheck` to see all errors `

@cvgw
Copy link
Contributor

cvgw commented Dec 4, 2019

@prary when I checkout your branch locally and run go mod vendor there are a lot of updates to the vendor directory. Should those be checked in?

I also noticed that golangci-lint (which runs in travis) might need to be upgraded.
From their https://github.com/golangci/golangci-lint#faq
go1.12+ are officially supported by the latest version of golangci-lint (>= 1.18.0).

When I upgraded golangci-lint to v1.21.0 I didn't see any errors running hack/linter.sh (which is what travis calls to run golangci-lint

@cvgw
Copy link
Contributor

cvgw commented Dec 4, 2019

@tejal29 can we remove hack/dep.sh as part of this change?

@vigith
Copy link

vigith commented Dec 5, 2019

should we remove vendor dir since go 1.13 supports module mirroring?

@tejal29
Copy link
Member

tejal29 commented Dec 9, 2019

@cvgw yes. We can definitely remove hack/dep.sh as part of this change.

@tejal29
Copy link
Member

tejal29 commented Dec 18, 2019

should we remove vendor dir since go 1.13 supports module mirroring?

removing vendor dir, slows down travis a lot. we shd keep it.

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Dec 21, 2019
@cvgw
Copy link
Contributor

cvgw commented Dec 21, 2019

rebased against master

@cvgw cvgw merged commit 2298205 into GoogleContainerTools:master Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade from dep to gomodules for dependency management
6 participants