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

Use pack CLI to build on GCB #3503

Merged
merged 2 commits into from
Jan 15, 2020
Merged

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Jan 13, 2020

Instead of using our own implementation of the Buildpacks lifecycle, it's more robust to use pack CLI.

@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #3503 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100% <ø> (ø) ⬆️
pkg/skaffold/build/gcb/buildpacks.go 100% <100%> (ø) ⬆️
pkg/skaffold/schema/defaults/defaults.go 94.25% <100%> (+0.1%) ⬆️
pkg/skaffold/util/tar.go 52.87% <0%> (-4.6%) ⬇️

Comment on lines 1 to 6
FROM alpine:3.10
ENV PACK_VERSION 0.6.0
ENV PACK_URL https://github.com/buildpack/pack/releases/download/v${PACK_VERSION}/pack-v${PACK_VERSION}-linux.tgz
RUN wget -O pack.tgz "${PACK_URL}"
RUN tar -zxf pack.tgz
RUN mv pack /usr/local/bin
Copy link
Member

Choose a reason for hiding this comment

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

This file seems to be here in error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me fix that

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

LGTM except for 2 nits.

@@ -62,6 +62,7 @@ const (
DefaultCloudBuildMavenImage = "gcr.io/cloud-builders/mvn"
DefaultCloudBuildGradleImage = "gcr.io/cloud-builders/gradle"
DefaultCloudBuildKanikoImage = "gcr.io/kaniko-project/executor"
DefaultCloudBuildPackImage = "gcr.io/k8s-skaffold/pack"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we move this gcb/buildpacks.go as a local constant if there are cyclic dependencies issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move all those default image names next to where they are used in another PR


set -e

PACK_VERSION=0.6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

usually, folks end up copying the version string from pack --version output

 ~/Downloads/pack  --version
v0.6.0 (git sha: 109b629d388cec0ed3836b9fed6717727a9187c1)
tejaldesai-macbookpro2:~ tejaldesai$ 

Do you want to add the prefix v to the version number?

Copy link
Contributor

@tejal29 tejal29 Jan 15, 2020

Choose a reason for hiding this comment

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

Also follow the same v0.0.6 version convention for the skaffold gcb pack image tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fixing that. Thank you!


docker build . --build-arg PACK_VERSION=${PACK_VERSION} -t gcr.io/k8s-skaffold/pack:${PACK_VERSION} -t gcr.io/k8s-skaffold/pack:latest
docker push gcr.io/k8s-skaffold/pack:${PACK_VERSION}
docker push gcr.io/k8s-skaffold/pack:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Does heroku publish a container image that we can re-use or any plans in future to do so? Or else, skaffold team will have to keep the track of publishing the new image everytime pack is upgraded (which is fine for now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't seen such plan.

Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
@dgageot dgageot merged commit 9396070 into GoogleContainerTools:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants