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

Shell out docker build #840

Merged
merged 3 commits into from
Jul 24, 2018

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Jul 20, 2018

Support an alternative way of building locally by running docker build directly. This is also a good opportunity to let the user enable experimental buildkit.

@dgageot dgageot force-pushed the shell-out-docker-build branch 2 times, most recently from 4f92d9c to b1ae16a Compare July 21, 2018 07:26
# Pushing the images can be skipped. If no value is specified, it'll default to
# `true` on minikube or Docker for Desktop, for even faster build and deploy cycles.
# `false` on other types of kubernetes clusters that require pushing the images.
# kaffold defers to your ~/.docker/config for authentication information.
Copy link
Contributor

Choose a reason for hiding this comment

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

kaffold -> skaffold

if l.cfg.UseDockerCLI || l.cfg.UseBuildkit {
absDockerfilePath := a.DockerfilePath
if !filepath.IsAbs(a.DockerfilePath) {
absDockerfilePath = filepath.Join(workspace, a.DockerfilePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need to be sure you don't append the workspace twice, since the workspace itself can also be a relative path. I did this here, you could pull this out into a util package and reuse it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll do that

@dgageot dgageot force-pushed the shell-out-docker-build branch from b1ae16a to d36ed59 Compare July 23, 2018 22:12
dgageot added 2 commits July 23, 2018 15:16
Fix the case where a build arg has no value.

Signed-off-by: David Gageot <[email protected]>
@dgageot dgageot force-pushed the shell-out-docker-build branch from d36ed59 to d83d107 Compare July 23, 2018 22:22
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

one small nit but otherwise LGTM


cstorage "cloud.google.com/go/storage"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/pkg/errors"
)

// NormalizeDockerfilePath returns the abolute path to the dockerfile.
Copy link
Contributor

Choose a reason for hiding this comment

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

abolute -> absolute :)

Signed-off-by: David Gageot <[email protected]>
@dgageot dgageot force-pushed the shell-out-docker-build branch from d83d107 to d1fba60 Compare July 24, 2018 03:08
@dgageot dgageot merged commit 26f58ef into GoogleContainerTools:master Jul 24, 2018
@dgageot dgageot deleted the shell-out-docker-build branch December 28, 2018 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants