Honor .gitignore when packaging job context - #279
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the packager utility to support packaging Git-tracked and non-ignored untracked files using git ls-files, falling back to a standard directory walk if Git is not available. The feedback focuses on improving robustness, performance, and cross-platform compatibility. Key suggestions include broadening exception handling from FileNotFoundError to OSError in subprocess execution, normalizing base_dir to an absolute path to prevent exclusion check failures, optimizing the exclusion helper to return early when no paths are excluded, and using posixpath.join to ensure ZIP archive paths use forward slashes on all operating systems.
|
This PR has been waiting for 11 days. Could a maintainer review when you have time? All checks are passing. |
|
Just resolved the merge conflicts with main! PR is now ready for review. 👍 |
|
Thanks for the contribution - can you verify this is still compatible post #284? |
|
PR #284 needs the plan_json parameter in zip_working_dir(). I've added it back to maintain compatibility while keeping your git-aware improvements. All set. |
What I found
I was working on packaging job contexts and realized the packager wasn't actually
respecting
.gitignorefiles. It would include files that should've been ignored,which is a problem when you're trying to keep your job context clean and not upload
stuff you explicitly told Git to ignore.
Why this matters
When you're packaging a job with sensitive files or build artifacts, you expect
.gitignoreto actually work. Right now it doesn't, so you end up shipping filesyou never intended to include.
How I fixed it
Instead of trying to manually parse and apply ignore patterns (which is error-prone),
I switched to using
git ls-fileswhen Git is available. Git already knows how tohandle
.gitignoreproperly, so why reinvent that wheel? If Git isn't available,it still falls back to the old directory traversal approach.
Testing
Tested it with a few different
.gitignorepatterns to make sure it works:*.tmpandbuild/All the existing tests pass too, so no regressions.