Skip to content

fix: include.project_directory path resolution#614

Merged
glours merged 1 commit intocompose-spec:mainfrom
glours:fix-include-project-dir
Apr 2, 2024
Merged

fix: include.project_directory path resolution#614
glours merged 1 commit intocompose-spec:mainfrom
glours:fix-include-project-dir

Conversation

@glours
Copy link
Copy Markdown
Collaborator

@glours glours commented Apr 2, 2024

@glours glours requested a review from ndeloof as a code owner April 2, 2024 12:15
@glours glours self-assigned this Apr 2, 2024
@glours glours force-pushed the fix-include-project-dir branch from da516f5 to c4cf828 Compare April 2, 2024 12:21
Copy link
Copy Markdown
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@glours glours force-pushed the fix-include-project-dir branch 4 times, most recently from ac8e307 to 01e5b5e Compare April 2, 2024 13:13
Copy link
Copy Markdown
Member

@milas milas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +151 to +155
func (l localResourceLoader) Dir(originalPath string) string {
path := l.abs(originalPath)
if !l.isDir(path) {
path = l.abs(filepath.Dir(originalPath))
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be checked running compose-spec command with --no-path-resolution

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by the way, I'm confused why such a distinction is made. Dir should return owner directory, whenever path is a file or not

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@milas milas changed the title mange properly include.project_directory when including files fix: include.project_directory path resolution Apr 2, 2024
Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
@glours glours force-pushed the fix-include-project-dir branch from 01e5b5e to 59d6924 Compare April 2, 2024 14:23
@glours glours enabled auto-merge (rebase) April 2, 2024 14:23
@glours glours merged commit 27a3a6a into compose-spec:main Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] including files with a custom project directory is ... just broken in 2.25.0

3 participants