Correctly ignore files from .dockerignore #508

guineveresaenger · 2023-02-28T20:19:52Z

This PR fixes a bug discovered by a trial user:

The provider copied __pycache__ dirs into the image, but our dockerignore contains the line **/__pycache__/**

It appears the Docker client does not handle the dockerignore file on its own, so this PR adds logic to ignore file patterns read from the dockerignore file.
The .dockerignore file is always expected to be at the root of the build context, both by convention and by design.

We read the to-be-ignored patterns into a slice of strings, which can then be passed to the TarOptions as ExcludePatterns.
The provider will now build an image which properly excludes the ignore files.

Additionally this PR creates a separate function to obtain the ignore file slice, since the hash function uses this information too.
This also fixes a minor bug in the hash function.

Exclude files from dockerignore when uploading context tarball
clean up ignore patterns
Refactor ignorePatterns into separate function. Fix dockerignore path bug in hash function.

github-actions · 2023-02-28T20:22:54Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2023-02-28T23:56:36Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2023-03-01T00:19:49Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2023-03-01T00:40:13Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2023-03-01T01:05:17Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

provider/provider.go

examples/dockerfile-go/.dockerignore

github-actions · 2023-03-02T17:22:35Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

jazzyfresh

LGTM, made some comments but fine to merge as is

examples/examples_go_test.go

examples/examples_nodejs_test.go

provider/image.go

jazzyfresh · 2023-03-02T17:38:46Z

provider/provider.go

+	dockerIgnore, err := os.ReadFile(dockerIgnorePath)
+	if err != nil {
+		if os.IsNotExist(err) {
+			// .dockerignore files are optional


great comment! i found it helpful

github-actions · 2023-03-02T18:01:21Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2023-03-02T19:53:34Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2023-03-02T22:52:33Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

… bug in hash function.

github-actions · 2023-03-02T23:17:51Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

AaronFriel

One last question, otherwise the changes LGTM

provider/image.go

github-actions · 2023-03-03T19:03:13Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

… running in buildkit mode and attempting to dockerignore

github-actions · 2023-03-07T00:09:22Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

AaronFriel · 2023-03-07T02:54:16Z

provider/image.go

+					"Due to limitations when running this provider in Buildkit mode, your build files may get copied " +
+					"into your image. Please ensure any copied file systems do not include build files."


This only applies to .dockerignore and the Dockerfile. Perhaps we should tone this message down, that seems very low impact.

how is this?

AaronFriel · 2023-03-07T02:56:16Z

provider/image.go

+	// warn user about accidentally copying build files
+	if build.BuilderVersion == defaultBuilder {
+		for _, pattern := range ignorePatterns {
+			if pattern == "!Dockerfile" || pattern == "!.Dockerignore" {


I'm not sure if these patterns will fire - .dockerignore is typically lowercase.

Rather than try to check if the pattern matches, should we store the ignorePatterns on line 97 in a different variable from the return value of buildCmd.TrimBuildFilesFromExcludes and check len(before) != len(after)?

ignorePatterns, err := getIgnore(dockerIgnorePath) // copy ignore patterns to compare initialIgnorePatterns := append([]string, ignorePatterns...) ignorePatterns = buildCmd.TrimBuildFilesFromExcludes(ignorePatterns, img.Build.Dockerfile, false) // warn user about accidentally copying build files, link to GitHub Issue: if build.BuilderVersion == defaultBuilder && len(initialIgnorePatterns) != len(ignorePatterns) {

oooh, I like it! good idea!

jazzyfresh

LGTM!

github-actions · 2023-03-07T17:37:35Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2023-03-07T19:34:03Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

guineveresaenger requested review from AaronFriel, jazzyfresh and a team February 28, 2023 20:21

guineveresaenger self-assigned this Feb 28, 2023

guineveresaenger added 4.x.x kind/bug Some behavior is incorrect or out of spec labels Feb 28, 2023

guineveresaenger force-pushed the guin/dockerignore branch from daf8b79 to f82ec09 Compare March 1, 2023 00:16

AaronFriel reviewed Mar 2, 2023

View reviewed changes

provider/provider.go Outdated Show resolved Hide resolved

examples/dockerfile-go/.dockerignore Outdated Show resolved Hide resolved

guineveresaenger requested a review from AaronFriel March 2, 2023 17:36

jazzyfresh approved these changes Mar 2, 2023

View reviewed changes

guineveresaenger added 10 commits March 2, 2023 15:14

Exclude files from dockerignore when uploading context tarball

8aaa1d1

clean up ignore patterns

de13388

Refactor ignorePatterns into separate function. Fix dockerignore path…

2df3c9a

… bug in hash function.

dockerignore details

0d4091b

unskipping dotnet test

3f33e4b

Do not ignore Dockerfile

a00e445

Improve readability for dockerfile exists condition

cd6172f

address review feedback

13ae4df

undo excluding Dockerfiles

a0002d3

Do not dockerignore and

b41834b

guineveresaenger force-pushed the guin/dockerignore branch from 85e7346 to b41834b Compare March 2, 2023 23:14

AaronFriel reviewed Mar 3, 2023

View reviewed changes

provider/image.go Outdated Show resolved Hide resolved

Add . to includeFiles

9c8d526

Add warning for build files potentially being copied to an image when…

3c911ce

… running in buildkit mode and attempting to dockerignore

guineveresaenger requested a review from jazzyfresh March 7, 2023 00:05

AaronFriel reviewed Mar 7, 2023

View reviewed changes

jazzyfresh approved these changes Mar 7, 2023

View reviewed changes

use length of ignorefiles to send warning to user

28794f4

tone down message

193cb5f

guineveresaenger merged commit 83e176a into master Mar 7, 2023

guineveresaenger deleted the guin/dockerignore branch March 7, 2023 20:05

AaronFriel added this to the 0.85 milestone Apr 24, 2023

mjeffryes mentioned this pull request Feb 14, 2024

Unnecessary dockerignore warning #989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly ignore files from .dockerignore #508

Correctly ignore files from .dockerignore #508

guineveresaenger commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

github-actions bot commented Mar 1, 2023

github-actions bot commented Mar 1, 2023

github-actions bot commented Mar 1, 2023

github-actions bot commented Mar 2, 2023

jazzyfresh left a comment

jazzyfresh Mar 2, 2023

github-actions bot commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

AaronFriel left a comment

github-actions bot commented Mar 3, 2023

github-actions bot commented Mar 7, 2023

AaronFriel Mar 7, 2023

guineveresaenger Mar 7, 2023

AaronFriel Mar 7, 2023

AaronFriel Mar 7, 2023

guineveresaenger Mar 7, 2023

jazzyfresh left a comment

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

		"Due to limitations when running this provider in Buildkit mode, your build files may get copied " +
		"into your image. Please ensure any copied file systems do not include build files."

Correctly ignore files from .dockerignore #508

Correctly ignore files from .dockerignore #508

Conversation

guineveresaenger commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

Does the PR have any schema changes?

github-actions bot commented Feb 28, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 1, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 1, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 1, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 2, 2023

Does the PR have any schema changes?

jazzyfresh left a comment

Choose a reason for hiding this comment

jazzyfresh Mar 2, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 2, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 2, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 2, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 2, 2023

Does the PR have any schema changes?

AaronFriel left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 7, 2023

Does the PR have any schema changes?

AaronFriel Mar 7, 2023

Choose a reason for hiding this comment

guineveresaenger Mar 7, 2023

Choose a reason for hiding this comment

AaronFriel Mar 7, 2023

Choose a reason for hiding this comment

AaronFriel Mar 7, 2023

Choose a reason for hiding this comment

guineveresaenger Mar 7, 2023

Choose a reason for hiding this comment

jazzyfresh left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2023

Does the PR have any schema changes?

github-actions bot commented Mar 7, 2023

Does the PR have any schema changes?