Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: include.project_directory path resolution #614

Merged
merged 1 commit into from Apr 2, 2024

Conversation

glours
Copy link
Contributor

@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
Copy link
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
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
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
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
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
Contributor 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
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 enabled auto-merge (rebase) April 2, 2024 14:23
@glours glours merged commit 27a3a6a into compose-spec:main Apr 2, 2024
8 checks passed
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