dagger ate my JSON | Dagger | Page 1

quick elm Feb 1, 2024, 7:04 PM

#

@rough phoenix I noticed something strange while updating a module for the new dagger.json format; wanted to just manually remove "root": ".." and update code, but dagger mod sync seems to have eaten my config and left it with just a "root-for"

#

JSON before:

{
  "name": "daggerverse",
  "sdk": "go",
  "include": [
    "*.go",
    "go.mod",
    "go.sum",
    "pkg/**/*",
    "api/**/*",
    "web/**/*",
    "dag/dagger.json",
    "dag/initdb.sql",
    "dag/refs",
    "dag/**/*.go",
    "dag/go.mod",
    "dag/go.sum",
    "workers/Procfile",
    "workers/fly.toml",
    "fly.toml"
  ],
  "dependencies": [
    "github.com/quartz-technology/daggerverse/postgres@f9338ce6a760cc6cc8cc04e5fa39228f029ea27e",
    "github.com/vito/daggerverse/go@fe4bc99ba02abfb74c10ae76f1794568ece70cc3"
  ]
}

JSON after dagger mod sync:

{
  "root-for": [
    {
      "source": "."
    }
  ]
}

#

seems like the issue is the include field - if I remove it, it converts fine

rough phoenix Feb 1, 2024, 7:06 PM

#

quick elm seems like the issue is the `include` field - if I remove it, it converts fine

Huh okay let me repro locally so I can look into it

quick elm Feb 1, 2024, 7:07 PM

#

also happy to open an issue and milestone it if you want to defer

rough phoenix Feb 1, 2024, 7:07 PM

#

quick elm also happy to open an issue and milestone it if you want to defer

This is worth looking into immediately, but yes an issue can't hurt 🙂

quiet patrol Feb 1, 2024, 7:13 PM

#

Love the thread title 😂

rough phoenix Feb 1, 2024, 7:14 PM

#

Oof, okay think I know what the problem is. dagger mod sync works as expected if you add dagger.json to the include list... Basically, the CLI isn't loading dagger.json into the engine because it wasn't included and then the engine thinks "oh this isn't a module yet, let me initialize a new one"...

Makes sense, but mildly confused because there actually is an integ test that upgrades an old dagger.json with an include field set, but something must be slightly different in it.

#

Will fix (@quick elm you can skip the issue if not made already, can just cover this is the PR)

quick elm Feb 1, 2024, 7:16 PM

#

ah, so the root-level dagger.json is what needs to be included?

#

basically, i played myself

rough phoenix Feb 1, 2024, 7:16 PM

#

quick elm ah, so the root-level `dagger.json` is what needs to be included?

I mean, it shouldn't need to be there, it's a bug. Makes no sense to ever not include it. But in the current logic, yes it would have to be there explicitly

#

I suspect the fix will just be to unconditionally always include it

quick elm Feb 1, 2024, 7:20 PM

#

am I doing something wrong here? thinkspin

~/src/daggerverse/testcontainers main*
❯ cat dagger.json
{
  "name": "testcontainers",
  "sdk": "go"
}
~/src/daggerverse/testcontainers main*
❯ cat ../dagger.json
{
  "root-for": [
    {
      "source": "."
    }
  ]
}
~/src/daggerverse/testcontainers main*
❯ cat dagger.json
{
  "name": "testcontainers",
  "sdk": "go"
}
~/src/daggerverse/testcontainers main*
❯ cat ../docker/dagger.json
{
  "name": "docker",
  "sdk": "go",
  "dependencies": [
    "github.com/kpenfound/dagger-modules/proxy@4974464186921b92deec3299cf3f94d3067dddca"
  ]
}
~/src/daggerverse/testcontainers main*
❯ with-dev dagger mod install ../docker
✘ withDependencies ERROR [0.00s]
✘ resolveDependency ERROR [0.00s]
• Engine: 329650b9b289 (version devel ())
⧗ 0.26s ✔ 21 ∅ 2 ✘ 2
Error: failed to generate code: input: resolve: moduleSource: asModule: withDependencies: failed to set module dependencies config: failed to resolve dependency module: module dep source path "../docker" escapes root "/"
input: resolve: moduleSource: asModule: withDependencies: failed to set module dependencies config: failed to resolve dependency module: module dep source path "../docker" escapes root "/"

(turns out I've been a heavy user of "root":".." despite always preaching about explicit inputs 🤡)

#

trying to get my daggerverse monorepo updated seems to be a bit of an exercise, might be worth attempting yourself and recording the commands you had to run

rough phoenix Feb 1, 2024, 7:23 PM

#

quick elm trying to get my daggerverse monorepo updated seems to be a bit of an exercise, ...

Yeah actually I'll just do that, you're on the latest commit of github.com/vito/daggerverse ?

quick elm Feb 1, 2024, 7:24 PM

#

yep, latest commit should be fine

rough phoenix Feb 1, 2024, 7:33 PM

#

Okay here's how you update docker and testcontainers:

sipsma@dagger_dev:~/tmp/repro$ ls
apko  concourse  docker  gitutil-client  go  LICENSE.md  moddesc  nix  noredist  test  testcontainers
sipsma@dagger_dev:~/tmp/repro$ rm testcontainers/dagger.json 
sipsma@dagger_dev:~/tmp/repro$ dagger mod init -m testcontainers/ --name testcontainers --sdk go
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 7.39s ✔ 73
sipsma@dagger_dev:~/tmp/repro$ rm docker/dagger.json 
sipsma@dagger_dev:~/tmp/repro$ dagger mod init -m docker --name=docker --sdk=go 
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 4.07s ✔ 70 ∅ 3
sipsma@dagger_dev:~/tmp/repro$ dagger mod install -m docker github.com/kpenfound/dagger-modules/proxy@4974464186921b92deec3299cf3f94d3067dddca
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 23.17s ✔ 257 ∅ 3
sipsma@dagger_dev:~/tmp/repro$ dagger mod install -m testcontainers docker
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 6.78s ✔ 314 ∅ 27
sipsma@dagger_dev:~/tmp/repro$ cat testcontainers/dagger.json 
{
  "name": "testcontainers",
  "sdk": "go",
  "dependencies": [
    {
      "name": "docker",
      "source": "../docker"
    }
  ]
}

Basically, anything that uses root should just be rm'd and re-initialized (there's some instructions here on this, didn't end up being in release notes so just planning on pinging the Zenith channel about it once release is finalized): https://github.com/dagger/dagger/pull/6386

#

I can't repro the Error: failed to generate code: input: resolve: moduleSource: asModule: withDependencies: failed to set module dependencies config: failed to resolve dependency module: module dep source path "../docker" escapes root "/", it also works if I instead do:

sipsma@dagger_dev:~/tmp/repro$ ls
apko  concourse  docker  gitutil-client  go  LICENSE.md  moddesc  nix  noredist  test  testcontainers
sipsma@dagger_dev:~/tmp/repro$ rm testcontainers/dagger.json 
sipsma@dagger_dev:~/tmp/repro$ dagger mod init -m testcontainers/ --name testcontainers --sdk go
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 7.39s ✔ 73
sipsma@dagger_dev:~/tmp/repro$ rm docker/dagger.json 
sipsma@dagger_dev:~/tmp/repro$ dagger mod init -m docker --name=docker --sdk=go 
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 4.07s ✔ 70 ∅ 3
sipsma@dagger_dev:~/tmp/repro$ dagger mod install -m docker github.com/kpenfound/dagger-modules/proxy@4974464186921b92deec3299cf3f94d3067dddca
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 23.17s ✔ 257 ∅ 3
sipsma@dagger_dev:~/tmp/repro$ cd testcontainers/
sipsma@dagger_dev:~/tmp/repro/testcontainers$ dagger mod install ../docker
• Engine: 0b75435fd6ad (version v0.9.8)
⧗ 6.88s ✔ 312 ∅ 28
sipsma@dagger_dev:~/tmp/repro$ cat dagger.json 
{
  "name": "testcontainers",
  "sdk": "go",
  "dependencies": [
    {
      "name": "docker",
      "source": "../docker"
    }
  ]
}

quiet patrol Feb 1, 2024, 7:35 PM

#

I still think "root-for": [{"source":"."}] should be an implicit default value instead being mandatory and written out to every dagger.json everywhere... I feel like it leaves to large of a trail on too many files, it's so much easier to fix our own code than migrate thousands of user's files...

rough phoenix Feb 1, 2024, 7:36 PM

#

quiet patrol I still think `"root-for": [{"source":"."}]` should be an implicit default value...

There's no migration needed, it's updated automatically. The only config files that need manual intervention are ones using root

quiet patrol Feb 1, 2024, 7:37 PM

#

Sorry I mean hypothetical future migrations

#

I don't know it just feels weird and a possible liability to make a field mandatory and write the same default value in 99% of the files

#

but the other side is the implementation complexity that it saves, and I know I have an incomplete picture there

rough phoenix Feb 1, 2024, 7:43 PM

#

It basically saves the engine from doing extra work, which both impacts our internal complexity in terms of logic and possibly performance. You could almost think of it like a cache that says "this dagger.json doesn't have a separate root-for, don't bother going to look for it".

That being said, I'll re-assess this while making the updates discussed here: https://discord.com/channels/707636530424053791/1202321084016513044 I could imagine some hypothetical ways of maybe avoiding the need for it while also preserving relative simplicity

rough phoenix Feb 1, 2024, 7:46 PM

#

rough phoenix Okay here's how you update `docker` and `testcontainers`: ```shell sipsma@dagger...

@quick elm let me know if that works for you and is good enough for the rest of the modules or if you hit any other snags

quick elm Feb 1, 2024, 7:52 PM

#

rough phoenix <@108011715077091328> let me know if that works for you and is good enough for t...

thanks a bunch! this looks pretty straightforward, I think I must have messed something up along the way

rough phoenix Feb 1, 2024, 9:04 PM

#

Working on the fix to include problem, realizing now the test case didn't catch it because I arbitrarily made "include": ["*"] facepalm https://github.com/sipsma/dagger/blob/69786cbdf63fe48623661d1d64bef6c6ab395579/core/integration/module_config_test.go#L28-L28

rough phoenix Feb 1, 2024, 11:13 PM

#

Turns out the fix is only easy if we're willing to hardcode including dagger.json, main.go, src/main.py, src/index.ts from the CLI, which is a level of hack I'd really really like to avoid... Working on something better by having the engine do all the loading from the caller's host, which should squash these sort of issues permanently and make the CLI even more minimal.

rough phoenix Feb 2, 2024, 1:00 AM

#

Got that basically working, but hit something with dagql @quick elm. I added a new method to SDK that just returns paths that should always be included, return type is just a []string.

But then when I'm trying to invoke the module SDKs (i.e. python,ts) I'm doing this:

    var paths dagql.Array[dagql.String]
    err := sdk.dag.Select(ctx, sdk.sdk, &paths,
        dagql.Selector{
            Field: "requiredPaths",
        },
    )
    if err != nil {
        return nil, fmt.Errorf("failed to call sdk module requiredPaths: %w", err)
    }

And getting this error: failed to call sdk module requiredPaths: toSelectable: unknown type "String"

Am I doing something wrong? I tried var paths []string too but same thing

quick elm Feb 2, 2024, 1:02 AM

#

thinkies

#

ah. it looks like dagql.Select only supports selecting objects out at the moment, for a silly reason

#

i'll patch it up here and paste a version that should work, that code is probably just too golfed

rough phoenix Feb 2, 2024, 1:04 AM

#

quick elm i'll patch it up here and paste a version that should work, that code is probabl...

Haha awesome thank you 🙏

quick elm Feb 2, 2024, 1:15 AM

#

@rough phoenix does this work?

// Select evaluates a series of chained field selections starting from the
// given object and assigns the final result value into dest.
func (s *Server) Select(ctx context.Context, self Object, dest any, sels ...Selector) error {
    var res Typed = self
    var id *idproto.ID
    var err error
    for i, sel := range sels {
        res, id, err = s.cachedSelect(ctx, self, sel)
        if err != nil {
            return err
        }
        if _, ok := s.ObjectType(res.Type().Name()); ok {
            // if the result is an Object, set it as the next selection target, and
            // assign res to the "hydrated" Object
            self, err = s.toSelectable(id, res)
            if err != nil {
                return err
            }
            res = self
        } else if i+1 < len(sels) {
            // if the result is not an object and there are further selections,
            // that's a logic error.
            return fmt.Errorf("cannot sub-select %s", res.Type())
        }
    }
    return assign(reflect.ValueOf(dest).Elem(), res)
}

#

(dagql/server.go:460)

rough phoenix Feb 2, 2024, 1:25 AM

#

quick elm <@949034677610643507> does this work? ```go // Select evaluates a series of chai...

New error: failed to call sdk module requiredPaths: cannot assign dagql.DynamicArrayOutput to []string

quick elm Feb 2, 2024, 1:26 AM

#

ah, that could be implemented by filling out an interface on DynamicArrayOutput but that's getting a bit in the weeds

#

(for posterity: DynamicArrayOutput is the non-generics form of dagql.Array, which is used for a lot of module stuff)

#

(it's kind of annoying maintaining both, but the type safety + schema derivation is nice)

#

oh hey, DynamicArrayInput implements Setter, you could copy-pasta that over

#

(it's ALSO annoying maintaining input vs. output types, but the difference is significant in GraphQL)

rough phoenix Feb 2, 2024, 1:28 AM

#

quick elm ah, that could be implemented by filling out an interface on DynamicArrayOutput ...

I can give it a shot if it's more of a quick foray in the weeds rather than epic journey through the weeds 😄 I mainly just wanted to check if I was holding it wrong originally

#

Copy-pasta worked, thanks!

quick elm Feb 2, 2024, 1:32 AM

#

nice, jankynp

#dagger ate my JSON