Skip to content
This repository was archived by the owner on May 3, 2022. It is now read-only.

Use Go 1.12 for docker builds#774

Closed
glyn wants to merge 3 commits into
cnabio:masterfrom
glyn:773-fix-build
Closed

Use Go 1.12 for docker builds#774
glyn wants to merge 3 commits into
cnabio:masterfrom
glyn:773-fix-build

Conversation

@glyn

@glyn glyn commented Jun 17, 2019

Copy link
Copy Markdown
Contributor

An alternative would be to bump quay.io/deis/lightweight-docker-go to use a
later version of Go, but it's probably safer to get out of that game and use
the more widely supported golang image instead.

Fixes #773

@silvin-lubecki

Copy link
Copy Markdown
Contributor

@glyn I think there's an issue with the vendoring test. Is there any reason why you had to bump kubernetes dependencies ?

@glyn

glyn commented Jun 17, 2019

Copy link
Copy Markdown
Contributor Author

@glyn I think there's an issue with the vendoring test. Is there any reason why you had to bump kubernetes dependencies ?

The vendoring test does dep check and so I issued dep ensure thinking this was supposed to ensure the vendored code matches Gopkg.lock.

@glyn glyn force-pushed the 773-fix-build branch 6 times, most recently from e74b1ec to 27b02fe Compare June 17, 2019 14:25
@silvin-lubecki

Copy link
Copy Markdown
Contributor

@glyn would it be possible to split this PR in 2? One to move to golang:1.12 image, and the other to bump the dependencies? Or at least in 2 separated commits? It's hard to see why this bump was needed 🤔

glyn added 2 commits June 17, 2019 15:55
This is necessary to get vendor verification to pass, e.g.

SKIP_DOCKER=true make verify-vendored-code
An alternative would be to bump quay.io/deis/lightweight-docker-go to use a
later version of Go, but it's probably safer to get out of that game and use
the more widely supported golang image instead.

Also, fix some linter problems in pkg/driver/kubernetes.go

Fixes cnabio#773
@glyn

glyn commented Jun 17, 2019

Copy link
Copy Markdown
Contributor Author

@glyn would it be possible to split this PR in 2? One to move to golang:1.12 image, and the other to bump the dependencies? Or at least in 2 separated commits?

I've split it into two commits. I don't think I could get CI to pass without both commits.

It's hard to see why this bump was needed 🤔

Try running SKIP_DOCKER=true make verify-vendored-code at master. I'd settle for another way of making this check pass if you'd like to suggest one. :-)

@carolynvs

Copy link
Copy Markdown
Contributor

Let me take a look at what's up with our makefile / dep testing code because my gut says that we shouldn't have to bump dependencies for this.

@carolynvs

Copy link
Copy Markdown
Contributor

@glyn The build for master is broken and isn't running on each push to master (but it is still triggering for PRs which is why you are getting hit with these problems). I am fixing master now and have @vdice getting the CI build running again. Once we are done, then you'll be able to rebase and just have the proper changes in your PR again.

@glyn

glyn commented Jun 17, 2019

Copy link
Copy Markdown
Contributor Author

@glyn The build for master is broken and isn't running on each push to master (but it is still triggering for PRs which is why you are getting hit with these problems). I am fixing master now and have @vdice getting the CI build running again. Once we are done, then you'll be able to rebase and just have the proper changes in your PR again.

Thanks @carolynvs. I'm a little puzzled though. The sole purpose of this PR is to fix the build. I'll look forward to comparing your PR...

@glyn

glyn commented Jun 17, 2019

Copy link
Copy Markdown
Contributor Author

Let me take a look at what's up with our makefile / dep testing code because my gut says that we shouldn't have to bump dependencies for this.

I suspect the problem is that the two commits 4dccb95 and bf4df3a conflicted logically, but not in a way that git would detect, so the PR for the second commit didn't have its checks re-run after the first commit was merged.

@glyn glyn mentioned this pull request Jun 17, 2019
@technosophos

Copy link
Copy Markdown
Member

It took me a while to realize that this is actually a fix for broken master, not just a switch to Go 1.12. So now I guess we have two PRs that are trying to fix #773. I don't actually have a strong preference for one or the other.

@jeremyrickard

Copy link
Copy Markdown
Member

Hey @glyn there was some chat in the slack channel and the consensus was to merge #775 to fix the build. Can you rebase this to just handle the upgrade to 1.12? That's a more involved change (i.e. consumers will also need to bump to 1.12).

@glyn

glyn commented Jun 18, 2019

Copy link
Copy Markdown
Contributor Author

Hey @glyn there was some chat in the slack channel and the consensus was to merge #775 to fix the build. Can you rebase this to just handle the upgrade to 1.12? That's a more involved change (i.e. consumers will also need to bump to 1.12).

I'll look into it. Actually, the only consumers who will be affected are those who specify SKIP_DOCKER=true when building, and they should know what they are doing.

Closing in favour of #777 which has a more appropriate branch name.

@glyn glyn closed this Jun 18, 2019
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.

make build fails to compile pkg/driver/kubernetes.go

5 participants