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

Build Fixes from PR #765 #775

Merged
merged 4 commits into from
Jun 17, 2019
Merged

Build Fixes from PR #765 #775

merged 4 commits into from
Jun 17, 2019

Conversation

carolynvs
Copy link
Contributor

@carolynvs carolynvs commented Jun 17, 2019

PR #765 didn't get a clean build before merging. This fixes the build on master by

  • syncs the vendor directory with Gopkg.lock
  • makes the linter happy
  • uses only go 1.11 string functions

PR cnabio#765 forgot to commit changes to vendor to sync it to the
changes to Gopkg.lock. This syncs the vendor directory
with those changes.
@silvin-lubecki
Copy link
Contributor

Tests are failing:

pkg/driver/kubernetes.go:353:10: undefined: strings.ReplaceAll

@glyn
Copy link
Contributor

glyn commented Jun 17, 2019

Tests are failing:

pkg/driver/kubernetes.go:353:10: undefined: strings.ReplaceAll

That's why #774 bumps Go to 1.12.

@carolynvs
Copy link
Contributor Author

Sorry I'm aware, just in a meeting. Will fix in an hour.

Copy link
Contributor

@glyn glyn left a comment

Choose a reason for hiding this comment

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

Needs to bump Go to 1.12 like #774 did.

Note that I'd like this to be done in a way that any duffle maintainer could also do, so PR 774 avoids using quay.io/deis/lightweight-docker-go and uses standard golang images instead.

Please take a look at PR 774 as it may provide most if not all of what's needed to sync the vendor directory and fix the build.

@carolynvs
Copy link
Contributor Author

@glyn I was hoping to just fix the build and let the go bumping happen in your PR (which looks great). I am 100% on board with switching to a better base image. I just wanted to focus on the changes that should have been in that PR to begin with, and not smoosh too many things together into this PR.

@carolynvs carolynvs requested a review from glyn June 17, 2019 16:23
Copy link
Member

@technosophos technosophos left a comment

Choose a reason for hiding this comment

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

I see a linting fix, a Go 1.11 fix, and some updated dependencies, and this looks good to me.

@carolynvs carolynvs changed the title Sync vendor to PR #765 Build Fixes from PR #765 Jun 17, 2019
@carolynvs
Copy link
Contributor Author

I updated the title of the PR to better reflect what all ended up in this grab bag PR. 😀

@technosophos
Copy link
Member

@glyn Since this one does not require 1.12, can you re-review this one and see if we can get it in to quickly get things green again?

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

LGTM.
I'm very much in favour of merging this PR first, then decide how to best switch to Go 1.12.

@technosophos
Copy link
Member

Okay. Since we've got a broken master right now, let's get this one merged, then start working on the 1.12 as a follow-up.

@carolynvs carolynvs merged commit 0ce61af into cnabio:master Jun 17, 2019
@carolynvs carolynvs deleted the sync-vendor branch June 17, 2019 18:02
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.

7 participants