Go SDK update

[feral sparrow]

/cc @prime kite @whole grail @small helm @random rapids @near brook

Just wanted to get your input before I make a round of updates

Also, anyone know how to check for zero value in Go, without resorting to reflect?

[prime kite]

Comment on the PR sounds good to me, shipit.

[prime kite]

I wonder if generics can somehow do this, probably depends on the details on what you're trying to do exactly. But honestly reflection is not the end of the world here if needed. It's ugly but the only other downside (I'm aware of) is performance, which we are a ways away from needing to be concerned with I think

[whole grail]

excited to see this land! 🛳️

[feral sparrow]

So in option 2, the struct doesn't contain pointers and instead I should somehow check whether the field was set or not (zero value)

it's a bit fragile since there won't be a way to explicitly SET a zero value (e.g. "")

but I think it's worth it

(e.g. avoid pointers in the Opt struct)

the other downside is, I need to check whether the field was set or not (figure out if it's the "zero" value)

I could either ... not do it. Just send zero values to the API if fields aren't set ( @whole grail is this a problem?)

or check in the code and only send non-zero values

[whole grail]

hmm right that gets tricky...

i would prefer the API to treat empty values as if they weren't provided tbh, it feels really funky dealing with string-pointers and slice-pointers.

ExecOpts makes a distinction between empty stdin and not-provided stdin, but I'm not sure how important that is.

i wonder if in cases where it matters it would be clearer to have a separate field to set instead (like CloseStdin: true instead of Stdin: "")

[feral sparrow]

Right. Maybe for now let's say the API can distinguish between empty and not provided, it's up to the SDKs to deal with that. Probably easier to debug incoming queries etc, at the very least

I'll push the problem down to the SDK (can't make the difference between zero value and not provided, if still going with Option 2)

[feral sparrow]

However with the problem pushed down, this wouldn't work with the Go SDK

[feral sparrow]

func isZeroValue(value any) bool {
    v := reflect.ValueOf(value)
    kind := v.Type().Kind()
    switch kind {
    case reflect.Pointer:
        return v.IsNil()
    case reflect.Slice, reflect.Array:
        return v.Len() == 0
    default:
        return v.IsZero()
    }
}

^^^ that's what I came up with, if anyone has a better way I'm all ears

side effect is passing e.g. entrypoint: []string{} will NOT set an empty entrypoint

[whole grail]

all the more reason to do https://github.com/dagger/dagger/issues/3284

[whole grail]

sorry what do you mean here?

[feral sparrow]

Speaking of stdin:

Would it make sense to move that stuff as optional arguments to exec, rather than wrapped in a ExecOpts input?

Otherwise it ends up double wrapped:

Exec(dagger.ContainerExecOpts{
  Args: []string{"ls"},
  Opts: dagger.ExecOpts {
    Stdin: "foo",
  }
}

vs:

Exec(dagger.ContainerExecOpts{
  Args: []string{"ls"},
  Stdin: "foo",
}

[whole grail]

hm yeah it does kind of stick out from the rest of the API

[small helm]

Go SDK update

[feral sparrow]

Updated to use "Option 2" as a separate commit (can revert easily):

Here's the updated tests as an example:
https://github.com/dagger/dagger/pull/3125/commits/6ef9c99bf2d9a5f2a0319786bec5a9408c2e1052#diff-aeaeaeeb193775ecf54fd9bf3273000f4013120e1446a2ef3e61afde3f29c59f

Used @prime kite's suggestion -- no need to pass nil, functions take a ...opt, last one where a field is set wins

e.g.

# this works, no `nil`
Exec()

# can pass the opts as a struct
Exec(dagger.ContainerExecOpts{
  Args: []string{"ls"},
  Stdin: "foo",
}

# this, although weird, also works
Exec(
    dagger.ContainerExecOpts{
        Args: []string{"ls"},
    }, dagger.ContainerExecOpts{
        Stdin: "foo",
    },
)

[feral sparrow]

https://github.com/dagger/dagger/issues/3340

I'm going to grab some lunch, but #3125 is ready to go

There's still a bunch of work left in the SDK (e.g. not use engine.Start) but I'd rather do that in a follow op

[feral sparrow]

@whole grail weird one... host (query field) is not returned by the introspection query. I added a dummy hostzz next to it and it works

:?

[whole grail]

wat

[feral sparrow]

yes

https://tenor.com/view/wat-what-wtf-que-gif-12854631

[whole grail]

hostzz it is 👍

[feral sparrow]

I can see it in graphiql though

[whole grail]

maybe some obscure conflict with the old schema...?

[feral sparrow]

we should be sending the same thing though:

https://github.com/dagger/dagger/blob/cloak/codegen/introspection/introspection.graphql

vs

https://github.com/graphql/graphql-js/blob/main/src/utilities/getIntrospectionQuery.ts

that was my thought, but I grepped host and can't find anything else

[whole grail]

case sensitivity? Host type vs host? ...but we have Container and container. hm

I actually don't really know what this line does:

"host": router.PassthroughResolver,

I guess it's like a no-op and it just uses the GraphQL schema type hint (host: Host!) to route any sub-fields to Host?

[feral sparrow]

I believe so, yes (/cc @prime kite)

shouldn't matter at that stage I think because we're not yet resolving

just introspecting, it's between the gql client and the graphql server, shouldn't be hitting our code

also ...

extend type Query {
  "Query the host environment"
  host: Host!
  hostzz: Host!
}

I do see hostzz :wat:

[prime kite]

Yeah if you don't have that line you get an error when trying to invoke the API that the server returned null for host. So instead we return struct{}{} to make it happy.

[feral sparrow]

ghost also works

Host too

no host instances in graphql-go's codebase

[whole grail]

I think it's returned if I run the introspection query in GraphiQL, but I'm not sure if I'm reading this right:

            {
              "args": [],
              "deprecationReason": null,
              "description": "Query the host environment",
              "isDeprecated": false,
              "name": "host",
              "type": {
                "kind": "NON_NULL",
                "name": null,
                "ofType": {
                  "kind": "OBJECT",
                  "name": "Host",
                  "ofType": null
                }
              }
            },

[feral sparrow]

wat

the plot thickens. I must be discarding it somehow, somewhere

🤦

yep, I did this

        if f.Name == "core" || f.Name == "host" {
            continue
        }

[whole grail]

lmao

[feral sparrow]

:slow clap:

[prime kite]

We should still change it to be hostzz anyways though

[feral sparrow]

@whole grail i've removed the "limits" to codegen to generate ALL types (unfortunately also includes the old API as well, until we remove it): https://github.com/dagger/dagger/pull/3343

[whole grail]

great, yeah I was wondering about maintaining that too

[prime kite]

I think we are pretty much good to remove the old API at this point? I mean we'll have to update tons of tests and extensions, but there's nothing else left to port I don't think??

[whole grail]

I think we're pretty close, I'm just chasing down loose ends now, it might be more than done enough

[feral sparrow]

Yep. I was just thinking, maybe a tad bit of more stability to the API (since we need to convert from old to new all extensions, might as well use the bindings). But maybe that's how we get stability in the first place

[whole grail]

though we might want to resolve https://github.com/dagger/dagger/issues/3204 to save some find-and-replace later

[feral sparrow]

err, meant to the API codegen, which is like 20 minutes old

[prime kite]

Yeah I mean switching all the tests+extensions to use the new API and new codegen will be good validation for both really

[whole grail]

looks like @.tanguy is on PTO this week so maybe we should just pick this up where he left off? https://github.com/dagger/dagger/pull/3294

[near brook]

Yes, I'm gonna come back on friday, but feel free to finish it

Let me check where I was

Hmmm, bad state, I messed up my rebase

So you should start from the previous tip before rebasing

[whole grail]

the schema relocation probably threw a wrench into the gears

[near brook]

There were a lot of conflicts

You know what

It's a little bit late but maybe by starting fresh, it shouldn't be hard

Let me have 30mn at it

[whole grail]

thanks! yeah starting fresh is probably easier at this point. no rush though if you've got other things to do

[feral sparrow]

@whole grail @prime kite Added a magefile and CI integration for codegen checking (e.g. fail if we forget to do a go generate while updating the API)

using cloak was not really necessary, but at least the integration is up and running for future stuff (e.g. running graphql schema validators which all happen to be in js)

https://github.com/dagger/dagger/pull/3344

[near brook]

Yeah, I'm just too exausted. I need to just recuperate. Couldn't manage. My eyelids just shut by themselves

[random rapids]

@feral sparrow Writing code with the Query builder is sooooo much nicer. I'm using it for the container_test.go tests and .

[small helm]

@random rapids you're boosting my confidence in the Go SDK launch 🙂

If you like it, others will too

[random rapids]

It's much tighter code, I don't have to define all of the types myself (I've mistyped queries and not gotten any feedback that I did it wrong...)

dagger-classic is going to be much easier + faster to implement with this. I'll get to rely on the compiler to catch mistakes. 🙂

[feral sparrow]

Pushed an update: https://github.com/dagger/dagger/pull/3349

[for tomorrow] @whole grail The only thing left to migrate are the API tests (still using engine.Start). I see some are using testutil.Query while others are using engine.Start directly, should I switch everything to one or the other?

[small helm]

@feral sparrow are you still stuck / puzzled on dagger.Connect and problematic stickiness of engine.Start ?

[feral sparrow]

Mostly got rid of it everywhere, except for really internal things

even dagger do is built on top of the Go SDK now, could have been a separate binary, nothing special

there's still ... checks notes ... 17 occurrences left

(just in tests now)

couple of hours and it should be good

[feral sparrow]

done!

[for tomorrow] @prime kite need some help with a couple of tests that are now broken, after conversion:

• TestCodeToSchema: cannot convert ctx (variable of type dagger.Context) to type dagger.Client

• TestCoreImageExport is timing out ... wondering if it has something to do with the runaway goroutine that spawns a registry

Pushed everything ... hope to merge it soon tomorrow because it's an absolute nightmare to maintain (switching away from engine.Start means re-indenting all code one level below, which gives no hope of rebase working)

[small helm]

https://tenor.com/view/line-up-line-cutting-the-line-excuse-me-front-of-the-line-gif-13048002

your PR tomorrow @feral sparrow 👆

[prime kite]

TestCodeToSchema just needs to be updated to not use the old graphql client anymore: https://github.com/sipsma/dagger/blob/c5b941d438bcf744deb8f103657d4f264973ab45/core/integration/testdata/codetoschema/main.go#L75-L75

[feral sparrow]

also: cmd/generate.go relies on ctx.Workdir, ctx.ConfigPath. I was wondering if the simplest fix would be to just rm it

[prime kite]

I thought it was rm'd already 😄

[feral sparrow]

perfect 🙂

Ok neat -- the only thing using engine.Start are: 1) dagger dev and 2) the SDK itself

[feral sparrow]

Oh neat!

I saw dagger.Context everywhere and panicked, I confused it with engine.Context

great, it's passing now

[prime kite]

I'm trying TestCoreImageExport locally now too, that is weird

[feral sparrow]

on my branch or on cloak?

(e.g. broken before or did i do this?)

[prime kite]

On your branch, I pulled it down

[feral sparrow]

and it's not just that one, there's a similar test that does the same thing (old push vs new push), breaks the same way

doh

wait

speaking of indentation: https://github.com/dagger/dagger/pull/3349/files#diff-4d36b7617bfca262caeba1e8af4f80ac924ce74976431d6d328b2d42811026acL176-L179

that changed the scope of that defer wg.Wait()

eh, doesn't change anything actually

[prime kite]

It's a lot of synchronization but yeah I think it should still work. Maybe the problem is that registryCtx is getting cancelled but c.Do doesn't get unblocked?

[feral sparrow]

i commented out wg.Wait, still getting a timeout

seems like it's stuck in #13 sh -c for i in $(seq 1 60); do nc -zv 127.0.0.1 5000 && exit 0; sleep 1; done; exit 1

like the registry is not coming up

[prime kite]

Oh yeah, I just noticed that the execution of the registry is cached in the progress output

It's supposed to never be cached by including the random var in it (I hate this test... there just was no other way to do it)

Wait nevermind, it may have been a different step that was cached, the progress output is just confusing

[feral sparrow]

yeah, not cached on my end

#8 [build 3/6] RUN apk add --no-cache file git
#0 0.058 fetch https://dl-cdn.alpinelinux.org/alpine/v3.16/main/aarch64/APKINDEX.tar.gz
#8 CANCELED

this, i do not understand

[prime kite]

It seems like everytime I run it only runs one thing or the other. Like I'll see some registry output one time, next time I'll see the bash wait loop, etc.

Is there a problem with trying to run multiple concurrent requests with the graphql client?

[feral sparrow]

yeah, that's my feeling as well but I don't know how it would happen

hmm

i think it has to do with the singleConnListener

doh

yeah, got it

was only able to serve one request at a time

it's the mess in singleConnListener + router.ServeConn -- hope there's a better way to do that

ok fixed

[prime kite]

Ah I see the update, makes sense

yeah it's a lot of hoops to jump through to pretend you're talking over a real socket

[feral sparrow]

crazy that we have to do that, all we need is a http.Client from a http.Handler

the interfaces aren't that different either, they both use http.Request, one uses http.Response while the other http.ResponseWriter

[prime kite]

I have some vague recollection of using some library to just convert between request and response objects many months ago, but then we couldn't use it anymore when we switched servers or something, I don't remember

agree though

[feral sparrow]

Tests are passing. Nobody moves

[feral sparrow]

Of course everything is passing except with test -race

I’ll check tomorrow

Marked the PR as ready to review

[feral sparrow]

😱😱😱

[feral sparrow]

@prime kite @whole grail addressed your comments

Waiting for CI to pass now

[prime kite]

:shipit: