Support relative paths in ignore files

[mwaskom]

Describe your changes

Currently we pass absolute paths into ignore functions, so relative patterns like *.txt don't properly ignore .txt files in the top-level of the context dir.

Not 100% sure about the consequences here. I think we want to do this for all string-based ignore patterns (i.e. ignore=["*.txt"] _and _ when a dockerignore file is in use). But this changes behavior also for when users pass an ignore function. OTOH I think we basically want some consistency between pattern-based and function-based ignoring. So maybe we just live with the change?

Needs a thoughtful changelog at least...

• Fixes CLI-373

Backward/forward compatibility checks

---

Check these boxes or delete any item (or this section) if not relevant for this PR.

• [ ] Client+Server: this change is compatible with old servers
• [ ] Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to multiple entities (client, server, worker, database). See points above.

---

Changelog

[freider]

It feels a bit scary to me to change the contract of the ignore/condition function to pass in relative paths, I'm pretty sure there could be users relying on the current behavior. As an alternative, I wonder if we could just inspect the input patterns that are passed and transform them into "absolute patterns" if they don't have an "absolute prefix", by adding the current working directory as a prefix to the pattern?

[mwaskom]

I'm pretty sure there could be users relying on the current behavior.

Yeah. Worst case is that mounts would be more inclusive right?

I wonder if we could just inspect the input patterns that are passed and transform them into "absolute patterns" if they don't have an "absolute prefix", by adding the current working directory as a prefix to the pattern?

I suspect that the code to do this would be pretty complicated, given how distributed this logic is 🫤

[freider]

I'm pretty sure there could be users relying on the current behavior. Yeah. Worst case is that mounts would be more inclusive right?

They could also be more exclusive if people are currently doing if path == Path(__file__).parent / "blah.txt" etc. :(

I wonder if we could just inspect the input patterns that are passed and transform them into "absolute patterns" if they don't have an "absolute prefix", by adding the current working directory as a prefix to the pattern? I suspect that the code to do this would be pretty complicated, given how distributed this logic is 🫤

Not necessarily - couldn't we just add something to the FilePatternMatcher that does this when adding patterns? if not pattern.startswith("/"): pattern = cwd + "/" + pattern

[mwaskom]

Not necessarily - couldn't we just add something to the FilePatternMatcher that does this when adding patterns? if not pattern.startswith("/"): pattern = cwd + "/" + pattern

That wouldn't work for **/ patterns right? I'm not sure it would work for ./ patterns either. I'm not sure what else.

It's quite unfortunate; it really feels like operating on relative paths (even in a user-provided function) is the preferred behavior here.

[mwaskom]

couldn't we just add something to the FilePatternMatcher

But also, currently the FilePatternMatcher doesn't know about the context_dir (and while we instantiate the FilePatternMatcher in image.py, we don't resolve the context dir until mount.py. So that's what I mean about the logic being distributed.

[freider]

couldn't we just add something to the FilePatternMatcher

But also, currently the FilePatternMatcher doesn't know about the context_dir (and while we instantiate the FilePatternMatcher in image.py, we don't resolve the context dir until mount.py. So that's what I mean about the logic being distributed.

Hmm, yeah maybe we'd want the logic to make the patterns absolute outside of the FIlePatternMatcher somehow 🤔

That wouldn't work for **/ patterns right? I'm not sure it would work for ./ patterns either. I'm not sure what else.

Wouldn't a **/ pattern just become <cwd>/**/pattern which would be what I expect?

An explicitly local pattern with ./ prefix would become <cwd>/./pattern which should either just work (haven't tested) or we'd have to strip the ./ before concatenation. Feels to me like this would work?

[mwaskom]

@prbot approve

[freider]

Some (late) thoughts, which shouldn't be blocking for the concept of relative paths, but more about making sure we have thought about/have tests for the various cases of what the paths are relative to:

• We would by default make this relative to the cwd of the process - this makes sense and is consistent with docker
• However, this can be overridden in some cases - namely from_dockerfile() and dockerfile_commands() which take a context_dir argument - do we know if paths to the ignore function are then relative to this context_dir? (in addition to the COPY source being relative to this path)
• There is also the case of auto-dockerignore support (when no ignore argument is provided), but I think that the dockerignore file is only allowed to be in the context dir, so it should be fine (otherwise we might have to make the predicate paths relative to where that file is 🤯 )
• When using add_local_dir() and add_local_python_source() it's a bit unclear to me what expected relative-to behavior for ignore= should be. Relative to the added directory, or to the cwd? (It's currently the latter, which makes things like ignore=[*.txt] not work :( )