#Honor `.dockerignore` file when running `buildDocker`

1 messages · Page 1 of 1 (latest)

digital meteor
#

I wanted to ask if folks have suggestions/concerns regarding this approach. Also do we want to make this behavior opt-in (for backward compatibility) OR we want to introduce a new version of this API where we enable this by default.

#

cc @chilly blade @sage oasis

digital meteor
#

also cc @shadow moth - Justin could you please provide feedback on these changes.

shadow moth
#

uh this seems good to me

#

there's no reason to backwards compat this imo

#

i would add some tests though with some of the more complex patterns that buildkit supports, and make sure we get the right results

#

we want it to be actually the same, not just seems to be the same

#

adding tests is gonna be super important, so we don't break it in the future

shadow moth
digital meteor
#

awesome, thank you so much.

chilly blade
#

just getting back into this. Justin's inputs SGTM! 🙏

digital meteor
#

but i could be completely wrong as well (because folks would have caught this issue in builtkit if it was an issue). so i am investigating this a bit.

#

The reason i am digging into this is because I picked up a testcase from buildkit repo, and that is not working in our implementation.

digital meteor
#

that seems intentional, but I don't have enough context to say why....

#

@shadow moth, the testcase which is failing is if the user specifies Dockerfile in the .dockerignore file, it fails to do the build as Dockerfile is no longer there.

#

I can workaround this by not-ignoring the "requested Dockerfile" even if it is configured in .dockerignore, but that also does not seem right.

digital meteor
#

Justin, if we add the .dockerignore handling in docker build command, doesn't it mean that we still transfer those files from host-system to daemon?

shadow moth
#

halloooo sorry, looking now

shadow moth
#

the reason this works in buildkit is that there are two local loads - one loads the dockerfile+dockerignore, and the second uses the dockerignore to load the build context

#

so you can't exclude the dockerfile, because it's loaded at the "same time"

#

there's only one dir that we use for everything i think, and we're then able to filter out the Dockerfile

#

no real preference, both are good

digital meteor
#

just hack, don't allow excluding the Dockerfile (this is quick+easy+fine if you can get it working)

I actually went this route and "un-ignored" the Dockerfile requested by the user. I need to test this more though.

#

I will also checkout other suggestion to see which makes more cleaner implementation.

#

thank you for your the pointers...

digital meteor
#

Hi @shadow moth , there is just one pending review comment that needs your confirmation, but otherwise the PR looks good to merge.

do we need a change entry for this?

shadow moth
#

a change entry would be good

#

i'll look through the pr when i get a moment!

digital meteor
#

Thanks Justin. I have committed a changie entry for this.

#

i am looking at the docs to update them as well. (where we mention that we don't support .dockerignore files).

digital meteor
#

Justin, I also want to make sure you are aware of this that with our changes, as we discussed earlier, we ignore the files as per dockerignore file before sending them to buildkit for building,

BUT we do upload them to dagger engine I believe. While with Dockerbuild I don't think these files are uploaded at all.

✔ Host.directory(path: "/Users/rajatjindal/go/src/github.com/rajatjindal/dagger-test"): Directory! 0.0s
│ ✔ upload /Users/rajatjindal/go/src/github.com/rajatjindal/dagger-test from aoy9hlxsaol1hcos3bqiwphn4 (client id: bb3nf3liur5zt02yv0ctv9h9s, session id: qdw13dc7sh0jvioqm5qp42o3p) 0.0s
│ │ ✔ filesync 0.0s
│ │ │ ✔ copy 0.0s

✔ foo: Foo! 0.0s
$ .build(
│ │ dir: Host.directory(path: "/Users/rajatjindal/go/src/github.com/rajatjindal/dagger-test"): Directory!
│ ): Container! 1.0s ◆ CPU Pressure (some): 416µs ◆ CPU Pressure (full): 403µs ◆ Memory Bytes (current): 954 kB ◆ Memory Bytes (peak): 23 MB CACHED
│ ✔ container: Container! 0.0s
│ $ .build(
│ │ │ context: Host.directory(path: "/Users/rajatjindal/go/src/github.com/rajatjindal/dagger-test"): Directory!
│ │ ): Container! 0.8s CACHED
│ │ ✔ Directory.withDirectory(
│ │ │ │ directory: Host.directory(path: "/Users/rajatjindal/go/src/github.com/rajatjindal/dagger-test"): Directory!
│ │ │ │ exclude: ["foo.txt", "Dockerfile"]
│ │ │ │ path: "buildctx"
│ │ │ ): Directory! 0.0s
│ │ ✔ .directory(path: "buildctx"): Directory! 0.0s
#

if needed, (in theory) we can add an optional argument useDockerignore file to host.Directory api, that would be false by default (to avoid making breaking change) and if enabled will honor the dockerignore file. do you think we should add that as well?

chilly blade
# digital meteor if needed, (in theory) we can add an optional argument `useDockerignore` file to...

this will be tricky because when using modules you don't have such API so we'll need to add a global flag or something for this which is 😬 . IMO it's ok to leave it as-is and if users bring up the fact that context uploads are slow because of the current implementation, we suggest that the best way to overcome that performance gap and make the most out of Dagger, is to migrate their Dockerfiles to code.

digital meteor
#

thanks Marcos.

shadow moth
#

yeah, i don't really mind the fact that this is uploaded!

#

sucks, but at least the behavior is correct