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

Fix dockerfile resolution #4260

Merged
merged 3 commits into from May 27, 2020

Conversation

briandealwis
Copy link
Member

Fixes: #4243

Description

pkg/skaffold/docker.NormalizeDockerfilePath attempted to handle the case where a user specified their dockerfile including the context directory:

- image: my/image
   context: project
   docker:
     dockerfile: "project/Dockerfile"

The code uses strings.HasPrefix to check if the dockerfile path included the context directory:

func NormalizeDockerfilePath(context, dockerfile string) (string, error) {
if filepath.IsAbs(dockerfile) {
return dockerfile, nil
}
if !strings.HasPrefix(dockerfile, context) {
dockerfile = filepath.Join(context, dockerfile)
}
return filepath.Abs(dockerfile)
}

This breaks on @ sujit-kamireddy's example in #4243 where the context directory is a prefix of Dockerfile:

- image: my/image
   context: Dock

(Although this situation may seem far-fetched, a context with Docker is not so unexpected.)

This code should first check if the context+dockerfilePath exists and only then use just the dockerfilePath alone.

User facing changes (remove if N/A)

There should be no changes.

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #4260 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
pkg/skaffold/docker/dependencies.go 75.34% <100.00%> (+2.73%) ⬆️
pkg/skaffold/kubernetes/watcher.go 87.50% <0.00%> (-12.50%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 63.41% <0.00%> (-4.88%) ⬇️
...ffold/kubernetes/portforward/resource_forwarder.go 84.31% <0.00%> (-3.45%) ⬇️
...g/skaffold/kubernetes/portforward/pod_forwarder.go 82.75% <0.00%> (-3.18%) ⬇️
pkg/skaffold/server/server.go 53.65% <0.00%> (ø)
pkg/skaffold/deploy/helm.go 80.27% <0.00%> (+0.02%) ⬆️
...affold/kubernetes/portforward/forwarder_manager.go 30.00% <0.00%> (+1.42%) ⬆️
pkg/skaffold/kubernetes/log.go 36.84% <0.00%> (+2.69%) ⬆️
...g/skaffold/kubernetes/portforward/entry_manager.go 96.49% <0.00%> (+2.87%) ⬆️
... and 2 more

@briandealwis briandealwis merged commit b8616a8 into GoogleContainerTools:master May 27, 2020
@briandealwis briandealwis deleted the fix-4243 branch May 27, 2020 09:38
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.

skaffold build fails if the artifact context is a substring of Docker
3 participants