search

tumblr / k8s-sidecar-injector

Kubernetes sidecar injection service
Apache License 2.0
345 stars 75 forks source link

Updates #46

Closed george-angel closed 4 years ago

george-angel commented 4 years ago

What and why?

Update Go version for the docker image and latest go mod versions.

Go version carries a number of speed and security improvements. Same for dependencies really.

This PR also includes https://github.com/tumblr/k8s-sidecar-injector/pull/45

Testing Steps

Passes tests with make test

Reviewers

Required reviewers: @byxorna Request reviews from other people you want to review this PR in the "Reviewers" section on the right.

:warning: this PR must have at least 2 thumbs from the MAINTAINERS.md of the project before merging!

george-angel commented 4 years ago

This builds fine locally, but does not build in Docker reliably. It seems there is a combination of steps that make it work or not to do with vendor/.

I don't really understand the idea behind vendoring dependencies on top of go mod, so unsure how to fix this error, or what causes it.

Any steer would be appreciated.

Edit 2020-08-17: Figured it out, the problem was installing golint inside the module directory, that added golint dependencies to the go.mod and vendoring without the codebase.

george-angel commented 4 years ago

@byxorna ping, I think this is ready for review now. Thank you.

george-angel commented 4 years ago

Honestly I started off with minimal changes, and I was sure it wasn't building. I have now reverted both changes that you commented on and the build works.

Not sure what happened there, thanks for bearing with.

komapa commented 4 years ago

This looks good to me! One nit - I would personally prefer the golint install to be encapsulated in the Makefile, to help keep things tidy (its now in make lint, Dockerfile, and .travis.yaml).

@kirooshu @defect @alex-laties

Agree with @byxorna here. You got into this place because you ran go mod tidy and that removed golint from vendoring. The way to fix this is to run one time go get golang.org/x/lint/golint and commit the go.mod/go.sum changes. Then you can remove go get -u golang.org/x/lint/golint from Travis' config.

george-angel commented 4 years ago

Ping, can this be merged? Thank you.

alex-laties commented 4 years ago

Ping, can this be merged? Thank you.

eightball says yes.