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

hack/run-in-docker: refactoring #206

Merged
merged 1 commit into from Jul 9, 2019

Conversation

Projects
None yet
4 participants
@ahmetb
Copy link
Contributor

commented Jun 5, 2019

  • build krew binary outside container (do not duplicate build process)
    • allows binary to be refreshed without rebuilding container
      this way, we preserve container session/state
    • also good for not leaking unused images to docker engine
      that needs to be cleaned up regularly
  • mount KUBECONFIG inside the container (not sure if it's actually useful)
    I probably won't use it (since GKE clusters require gcloud to work, too)
  • made the script a bit friendlier to follow as a user.
@codecov-io

This comment has been minimized.

Copy link

commented Jun 5, 2019

Codecov Report

Merging #206 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #206   +/-   ##
======================================
  Coverage    53.6%   53.6%           
======================================
  Files          16      16           
  Lines         735     735           
======================================
  Hits          394     394           
  Misses        289     289           
  Partials       52      52

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3951c05...a96305a. Read the comment docs.

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Could you change the runtime image to something other than alpine? That was a bad idea, because whenever something needs a libc, there's only libc.musl.
What about gcr.io/gcp-runtimes/ubuntu_16_0_4?

That worked with the original script:

diff --git a/hack/run-in-docker.sh b/hack/run-in-docker.sh
index e05726c..beccfcf 100755
--- a/hack/run-in-docker.sh
+++ b/hack/run-in-docker.sh
@@ -20,4 +20,4 @@ SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
 
 docker build -f "${SCRIPTDIR}/sandboxed.Dockerfile" -t krew:sandbox "${SCRIPTDIR}/.."
 
-docker run --rm -ti krew:sandbox
+docker run --rm -ti krew:sandbox /bin/bash
diff --git a/hack/sandboxed.Dockerfile b/hack/sandboxed.Dockerfile
index 8b8f7c2..9e82065 100644
--- a/hack/sandboxed.Dockerfile
+++ b/hack/sandboxed.Dockerfile
@@ -12,12 +12,16 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-FROM golang:alpine as builder
+FROM gcr.io/gcp-runtimes/ubuntu_16_0_4 as builder
+
+COPY --from=golang:1.11 /usr/local/go /usr/local/go
+ENV PATH /usr/local/go/bin:/go/bin:$PATH
+ENV GOPATH /go/
 
 WORKDIR /go/src/sigs.k8s.io/krew
 
 ENV KUBECTL_VERSION v1.14.2
-RUN apk add --no-cache curl && \
+RUN apt-get install --no-install-recommends --no-install-suggests curl && \
     curl -Lo /usr/bin/kubectl https://storage.googleapis.com/kubernetes-release/release/${KUBECTL_VERSION}/bin/linux/amd64/kubectl  && \
   chmod +x /usr/bin/kubectl
 
@@ -26,8 +30,8 @@ COPY . .
 RUN go build -tags netgo -ldflags "-s -w" ./cmd/krew
 
 # production image
-FROM alpine
-RUN apk --no-cache add git && \
+FROM gcr.io/gcp-runtimes/ubuntu_16_0_4
+RUN apt-get install --no-install-recommends --no-install-suggests -y git && \
     ln -s /usr/bin/krew /usr/bin/kubectl-krew
 
 # initialize index
@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

What about gcr.io/gcp-runtimes/ubuntu_16_0_4?

We can do that. Is there a reason why we should still have go in this image?

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Is there a reason why we should still have go in this image?

The builder image needs it, but the final image with the krew binary inside has no go toolchain. Or do I misunderstand your question?

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

I'm afraid you didn't read the PR description. (:

  • build krew binary outside container (do not duplicate build process)

    • allows binary to be refreshed without rebuilding container
      this way, we preserve container session/state

we don't need Go inside the container.

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I'm afraid you didn't read the PR description.

Yes, that's right. I was only sharing the improved version of the former Dockerfile. With your improvements that will no longer be necessary.

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@corneliusweig are you actually using this script?
I'm planning on fixing the PR, but not sure about the use case.

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@ahmetb Yes, whenever I want to review a new submission to krew-index, I try to install and run it inside docker. That's my main use-case.
But I also use it to try out new features in krew.

hack/run-in-docker: refactoring
- build krew binary outside container (do not duplicate build process)
  - allows binary to be refreshed without rebuilding container
    this way, we preserve container session/state
- mount KUBECONFIG inside the container (not sure if it's actually useful)
  I probably won't use it (since GKE clusters require gcloud to work, too)
- made the script a bit friendlier to follow as a user.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

@ahmetb ahmetb force-pushed the ahmetb:run-in-docker branch from f4d1193 to a96305a Jul 8, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

/assign @corneliusweig
sorry just got to this!

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Beautiful! Works like a charm. 🦄

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 9, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jul 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ahmetb,corneliusweig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

/lgtm cancel

Can you change the PR title?

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 9, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

/lgtm
Prow won't merge anyway until the title is fixed.

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 9, 2019

@ahmetb ahmetb changed the title [WIP] hack/run-in-docker: refactoring hack/run-in-docker: refactoring Jul 9, 2019

@k8s-ci-robot k8s-ci-robot merged commit 2dab1a1 into kubernetes-sigs:master Jul 9, 2019

2 of 3 checks passed

tide Not mergeable.
Details
cla/linuxfoundation ahmetb authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.