-
Notifications
You must be signed in to change notification settings - Fork 557
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 Directory.glob on directories with a sub path #6904
Fix Directory.glob on directories with a sub path #6904
Conversation
@@ -295,14 +293,14 @@ func (dir *Directory) Glob(ctx context.Context, src string, pattern string) ([]s | |||
} | |||
// empty directory, i.e. llb.Scratch() | |||
if ref == nil { | |||
if clean := path.Clean(src); clean == "." || clean == "/" { | |||
if clean := path.Clean(dir.Dir); clean == "." || clean == "/" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Entries
uses the src = path.Join(dir.Dir, src)
here, but this branch comes from the solve from dir.LLB
which doesn't factor src
.
I tried triggering the error after this "if" in a test but couldn't because I'm not entirely sure when ref is nil without an error.
If ref == nil
really only happens in dag.directory()
, I feel like just doing this here:
if ref == nil {
return []string{}, nil
}
\cc @jedevc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM - the only caller of Glob
is in core/schema/directory.go
, which only passes .
, so every src directory is guaranteed to exist - so we shouldn't end up in a scenario where we hit the edge case.
Can this be merged? |
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
7be8608
to
3de2f18
Compare
Yep, just rebased. Waiting for checks. |
Fixes #6903