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

Support Dockerfile.dockerignore #3837

Merged

Conversation

agrinh
Copy link
Contributor

@agrinh agrinh commented Mar 17, 2020

Fixes: Did not create an issue for this, please let me know if I should.

Description
Add support for getting dependencies from Dockerfile.dockerignore files. See e.g. moby/buildkit#901 . Since the same code produces dependencies for Kaniko I expect this will also make Skaffold align better with Kaniko (GoogleContainerTools/kaniko#801).

User facing changes
Users with multiple Dockerfiles will now be able to have different .dockerignore files. For example, this is the behavior with the following context:

.
├── .dockerignore
├── Dockerfile
├── Dockerfile.dev
└── Dockerfile.dev.dockerignore

before: docker would respect Dockerfile.dev.dockerignore but skaffold would not take it into account when producing dependencies, and syncing could break e.g. if .dockerignore hid files used by Dockerfile.dev.dockerignore, leading to unexpected behavior.

after: skaffold will make Dockerfile.dev.dockerignore take precedence over .dockerignore (will fall back to .dockerignore if it doesn't exist).

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@agrinh
Copy link
Contributor Author

agrinh commented Mar 17, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

Overall, it's great. A few things can be improved.

pkg/skaffold/docker/dependencies.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/dependencies.go Outdated Show resolved Hide resolved
@dgageot dgageot self-assigned this Mar 18, 2020
Copy link
Contributor Author

@agrinh agrinh left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestions @dgageot, fixed your comments.

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #3837 into master will decrease coverage by <.01%.
The diff coverage is 76.47%.

Impacted Files Coverage Δ
pkg/skaffold/docker/syncmap.go 67.16% <100%> (ø) ⬆️
pkg/skaffold/docker/dependencies.go 75% <75%> (+1.08%) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 65.85% <0%> (-2.44%) ⬇️
pkg/skaffold/debug/transform_nodejs.go 77.87% <0%> (-0.77%) ⬇️
pkg/skaffold/docker/docker_init.go 100% <0%> (ø) ⬆️
pkg/skaffold/initializer/build/util.go 100% <0%> (ø) ⬆️
pkg/skaffold/initializer/build/builders.go 57.69% <0%> (ø) ⬆️

@agrinh agrinh requested a review from dgageot March 18, 2020 18:47
@agrinh
Copy link
Contributor Author

agrinh commented Mar 18, 2020

Weird, integration tests failed at first but they succeeded after restarting.

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Mar 18, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 18, 2020
@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Mar 18, 2020
@dgageot dgageot merged commit 01ca7e9 into GoogleContainerTools:master Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kokoro:run runs the kokoro jobs on a PR size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants