Read .dockerignore files #879
Conversation
|
Thanks for the PR! High‑level, I like the approach: A few details I’d like us to tighten up:
It would be good to extend the matcher now so it lines up more closely with Docker’s docs or atleast call it out in the docs |
There was a problem hiding this comment.
Can you extract the two private functions you added to a dedicated type?
- Keeps BuildFSSync more concise and easier to comprehend
- Improves testability and reusablity.
EDIT: The specific comments I made in my review might not apply exactly after you rework for the changes Sid mentioned above, but it'd still be better to keep the ignore testing separate from the BuildFSSync implementation.
| let followPaths: [String] = packet.followPaths() ?? [] | ||
|
|
||
| // Parse .dockerignore if present | ||
| let ignorePatterns = try parseDockerignore() |
There was a problem hiding this comment.
resolve the path to the ignore file here and load the file into Data, and then create an IgnoreSpec(data)
| let relPath = try url.relativeChildPath(to: contextDir) | ||
|
|
||
| // Check if the file should be ignored | ||
| if try shouldIgnore(relPath, patterns: ignorePatterns, isDirectory: url.hasDirectoryPath) { |
There was a problem hiding this comment.
this can become ignoreSpec.shouldIgnore(relPath:isDirectory:)?
| return Array(globber.results) | ||
| } | ||
|
|
||
| /// Parse .dockerignore file and return list of ignore patterns |
There was a problem hiding this comment.
Extract these two methods into IgnoreSpec.swift.
| @@ -0,0 +1,209 @@ | |||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
Rename to IgnoreSpecTests.swift
Extract the .dockerignore parsing and pattern matching logic from BuildFSSync into a dedicated IgnoreSpec type. This improves code organization, testability, and reusability. Changes: - Create new IgnoreSpec.swift with IgnoreSpec type that takes Data in its initializer and provides shouldIgnore(relPath:isDirectory:) method - Update BuildFSSync to load .dockerignore file data and create an IgnoreSpec instance - Rename DockerignoreTests to IgnoreSpecTests - Add unit tests for IgnoreSpec type This addresses feedback from jglogan in PR apple#879.
This addresses feedback from jglogan in PR apple#879.
|
@wlan0 @jglogan Hello 👋 I have made an attempt now at improving the PR based on your comments. I decided to just go a regex route and move away from the Globber (didn't quite understand whether that's used for other stuff and so don't understand if it's fine to just change how it works). Also added more tests for ** and ! patterns. Let me know what you think. (Sorry if my response time is a bit slow, I don't have that much free time these days😊) |
|
Hmmm. I'm trying this on a real project, and the problem now is that it's very slow (my project has some big-but-supposed-to-be-ignored-folders, like I guess this problem might be a bit bigger? Like, the way it is now it will still walk over all files inside of |
|
Have you considered looking at how it works in moby/patternmatcher and the underlying filepath.Match() implementation in the Go library? |
3 participants
Type of Change
Motivation and Context
As the included test demonstrates, when a folder contains a .dockerignore file with pattern
to-be-ignored.txt, the fileto-be-ignored.txtis still included in the build context. Check out the first commit to see this test failing.Testing