fix: include.project_directory path resolution#614
Conversation
da516f5 to
c4cf828
Compare
ac8e307 to
01e5b5e
Compare
milas
left a comment
There was a problem hiding this comment.
Fix makes sense - logic to detect directories makes me nervous because it does a stat / real FS operation, which has bit us in the past
| func (l localResourceLoader) Dir(originalPath string) string { | ||
| path := l.abs(originalPath) | ||
| if !l.isDir(path) { | ||
| path = l.abs(filepath.Dir(originalPath)) | ||
| } |
There was a problem hiding this comment.
This heuristic makes me a bit nervous. We've had issues before doing stuff like this - the isDir check is doing a stat, so requires all the paths to exist to behave correctly. In this instance, that might be ok, but I'm not sure it holds for the general case and might result in surprises for future callers of Dir().
In practice, while a pain, I think it'd be better to treat this more like the Go stdlib and ALWAYS return the enclosing dir and make the Abs function public (could rename it something like LoadedPath()?). That is, the caller needs to know what type of path they're carrying around (file or dir) and invoke the right function.
There was a problem hiding this comment.
Can be checked running compose-spec command with --no-path-resolution
There was a problem hiding this comment.
by the way, I'm confused why such a distinction is made. Dir should return owner directory, whenever path is a file or not
There was a problem hiding this comment.
depending how you declare your path foo/bar or foor/bar/, filepath.Dir won't return the same output, foo in the first case and foo/bar in the second one
https://pkg.go.dev/path/filepath#example-Dir
There was a problem hiding this comment.
then AFAICT we could just rely on filepath.Clean so that we don't need to check actual file exists but rely on file path representation
include.project_directory path resolution
Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
01e5b5e to
59d6924
Compare
Fixes docker/compose#11626 and docker/compose#11573
https://docker.atlassian.net/browse/COMP-496