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 #801

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions integration/dockerfiles/Dockerfile_test_dockerignore_relative
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# This dockerfile makes sure Dockerfile.dockerignore is working
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this from integration test and rename this to "Dockerfile_dockeringore_relative"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tejal29 I'm not 100% sure how things work: if the file doesn't have the word test in it, then it won't be included in integration tests or is there more to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah you answered below, thx :)

# If so then ignore_relative/foo should copy to /foo
# If not, then this image won't build because it will attempt to copy three files to /foo, which is a file not a directory
FROM scratch
COPY ignore_relative/* /foo
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# A .dockerignore file to make sure dockerignore support works
ignore_relative/**
!ignore_relative/foo
Empty file added integration/ignore_relative/bar
Empty file.
Empty file added integration/ignore_relative/baz
Empty file.
Empty file added integration/ignore_relative/foo
Empty file.
2 changes: 1 addition & 1 deletion pkg/executor/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) {
if err != nil {
return nil, err
}
if err := util.GetExcludedFiles(opts.SrcContext); err != nil {
if err := util.GetExcludedFiles(opts.DockerfilePath, opts.SrcContext); err != nil {
return nil, err
}
// Some stages may refer to other random images, not previous stages
Expand Down
2 changes: 1 addition & 1 deletion pkg/util/command_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ var isSrcValidTests = []struct {
func Test_IsSrcsValid(t *testing.T) {
for _, test := range isSrcValidTests {
t.Run(test.name, func(t *testing.T) {
if err := GetExcludedFiles(buildContextPath); err != nil {
if err := GetExcludedFiles("", buildContextPath); err != nil {
t.Fatalf("error getting excluded files: %v", err)
}
err := IsSrcsValid(test.srcsAndDest, test.resolvedSources, buildContextPath)
Expand Down
8 changes: 6 additions & 2 deletions pkg/util/fs_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -554,11 +554,15 @@ func CopyFile(src, dest, buildcontext string) (bool, error) {
}

// GetExcludedFiles gets a list of files to exclude from the .dockerignore
func GetExcludedFiles(buildcontext string) error {
path := filepath.Join(buildcontext, ".dockerignore")
func GetExcludedFiles(dockerfilepath string, buildcontext string) error {
path := dockerfilepath + ".dockerignore"
if !FilepathExists(path) {
path = filepath.Join(buildcontext, ".dockerignore")
}
if !FilepathExists(path) {
return nil
}
logrus.Infof("Using dockerignore file: %v", path)
contents, err := ioutil.ReadFile(path)
if err != nil {
return errors.Wrap(err, "parsing .dockerignore")
Expand Down
46 changes: 46 additions & 0 deletions pkg/util/fs_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -694,3 +694,49 @@ func Test_childDirInWhitelist(t *testing.T) {
})
}
}

func Test_correctDockerignoreFileIsUsed(t *testing.T) {
type args struct {
dockerfilepath string
buildcontext string
excluded string
notExcluded string
}
tests := []struct {
name string
args args
}{
{
name: "relative dockerfile used",
args: args{
dockerfilepath: "../../integration/dockerfiles/Dockerfile_test_dockerignore_relative",
buildcontext: "../../integration/",
excluded: "ignore_relative/bar",
notExcluded: "ignore_relative/foo",
Copy link
Member

Choose a reason for hiding this comment

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

i think we should test ignore/bar is not excluded along with ignore_relative/foo here to make sure context dockerignore is not used.? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very good point, yes!

},
},
{
name: "context dockerfile is used",
args: args{
dockerfilepath: "../../integration/dockerfiles/Dockerfile_test_dockerignore",
buildcontext: "../../integration/",
excluded: "ignore/bar",
notExcluded: "ignore/foo",
Copy link
Member

Choose a reason for hiding this comment

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

We should test ignore_relative/bar is not excluded here to make sure Dockerfile_test_dockerignore_relative.dockerignore is not used?

},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
excluded = nil
if err := GetExcludedFiles(tt.args.dockerfilepath, tt.args.buildcontext); err != nil {
t.Fatal(err)
}
if excl := excludeFile(tt.args.excluded, tt.args.buildcontext); !excl {
t.Errorf("'%v' not excluded as expected", tt.args.excluded)
}
if excl := excludeFile(tt.args.notExcluded, tt.args.buildcontext); excl {
t.Errorf("'%v' excluded against expectation", tt.args.notExcluded)
}
})
}
}