#debug module tests

1 messages · Page 1 of 1 (latest)

wintry pasture
#

@clear shell running tests for the sdk module changes, one of the pre-existing tests ("respects existing go.mod even if root points to parent that has go.mod") fails because it seems to assume that the current working directory of a Go module is the root of SourceDirectory, whereas after my changes I set it to be SourceDirectorySubpath (if set).

I'm not 100% sure, but I thought pre-vacation the workdir was the SourceDirectorySubpath, and in general that feels most logical to me (though not obvious one way or the other).

Just checking if that was an intentional change or not

clear shell
#

Doesn't seem like an intentional change, I guess this is the only test that covers that scenario?

#

agree that subpath being workdir makes more sense (thinking about the monorepo use case)

wintry pasture
wintry pasture
clear shell
#

I think it's the "root": ".." that makes it kind of confusing/debatable really

#

since you could imagine that as "project root" or something, which you could imagine is where you run from

#

but maybe i'm just imagining things

#

I think the Go SDK might also depend on this behavior

wintry pasture
#

I agree that's why it's not crystal clear, but overall if you imagine a use case for this like our ci/ module in the repo, the only reason we use root was to re-use the go.mod from the root of the repo. I think in that particular case I would prefer to start out in my actual module rather than the root?

Honestly it's kind of splitting hairs I think, in either case you have access to all your files

clear shell
wintry pasture
# clear shell https://github.com/dagger/dagger/blob/main/sdk/go/runtime/main.go#L65-L71 <- thi...

Oh that exact code is all gone now, so that very particular case isn't a problem anymore.

In general though, that's kind of the tradeoff. My first attempt to fix the test was to just change this line in the test to be Host().Directory(".."): https://github.com/sipsma/dagger/blob/27eea6df1d88cbd1ce67fe23e4e01bfc82e0da19/core/integration/module_test.go#L201

But then I remembered the whole "can't escape workdir" error, and instead just changed it to be:

wd, err := os.Getwd()
if err != nil {
  panic(err)
}
return dag.Host().Directory(wd+"/..")

and now it passes

#

Honestly we should just get rid of that error case, it serves no purpose anymore (ever? I forget)

#

I guess if you think about when you're writing your module code, it's very natural that if you type in your current code .., it would be relative to your source code file? Actually i have some nvim plugin that autocompletes based on that assumption i think

clear shell
#

yeah true

#

though, does that nvim plugin resolve relative to the file's directory or your current working directory? thinkies

#

i have a similar one and I think it's the latter

wintry pasture
#

no idea how, why, or even what plugin that is though, i probably just copy pasted some "ultimate neovim config" at some point 😅

clear shell
#

i am now convinced that the code should say Directory("..") 👍

#

now we need to convince @torn nexus that the 'escape workdir' check should be removed 😛

wintry pasture
#

haha, I think the convincing argument is that you can workaround it trivially by just doing os.Getwd()

#

IIRC the original restriction might be from back in cloak days where extensions calling c.Host().Directory were actually referring to the caller's host? As opposed to the container's

#

Which obviously isn't an issue anymore

clear shell
#

iirc it was less a security concern and more of a 'we might want some mystical magical workdir abstraction someday'