Setting optional for Directory/File

[tall geyser]

Is there a good way to set defaults to core types? If I require a source directory right now the comment pragmas don't support Directory/File. They are just strings in the CLI. Are there plans to support it?

[rocky meadow]

Sorry I don't understand the question. Is this in Go?

Here's an example of taking an optional Container as argument (same idea for Directory or File): https://github.com/shykes/daggerverse/blob/main/hello/main.go#L27-L28

Here I apply the default container (.GetOr(defaultFigletContainer))

Is that what you mean?

[tall geyser]

yes

// +optional
// +default=/mydir
src *Directory

failed for me

[rocky meadow]

Oh I don't know about those pragmas. Is that a Dagger-specific thing?

This might be for documentation only?

The code I linked to definitely works (Optional[TYPE]) + .GetOr)

[tall geyser]

Yes and apparently going to supercede Optional?

[rocky meadow]

Never heard of it

cc @hot jetty @ember frost @hallow zephyr

[ember frost]

there's no way to do this at the moment, you'll have to just check for nil and assign at runtime. it's not super clear how that would work really, at the schema level it's a scalar value. I guess we could pre-compute an ID now, but not sure if it's worth it, vs. just documenting the semantic default

[rocky meadow]

Are we talking about optional arguments? I feel like maybe we're not, and I don't know what we're actually talking about 🙂

[tall geyser]

Yes Solomon, optional args. Sorry if I wasn't clear.

So, the Optional would be the way to set a default for *File or *Directory. I thought that pattern was going away in favor of the comment pragmas

[rocky meadow]

I just got confused by @ember frost 's comment that it's not possible (I'm doing it right now). Also confused by these comment pragmas possibly breaking how we do optional arguments, since I've never heard of them

[tall geyser]

Context: https://github.com/dagger/dagger/pull/6179

[hot jetty]

Yeah I think the original starting place was that we needed to add support for more "annotations" in the Go SDK (e.g. marking a field as private), but we couldn't just keep re-using the same idea as the generic Optional wrapper since you'd end up with ridiculous types wrapped in multiple levels.

So we added support for pragmas (//+private, //+default, //+optional) to have a common base to support all needs around "type annotations" going forward in Go. The existing Go dev docs have all been updated to use those too.

We did not actually remove Optional[T] though, it still works. I'm not sure at this point if it's worth the breaking change. There are times where it's nice relative to //+optional and the only downside is some extra complication in our go sdk code

[ember frost]

sorry for not clarifying, yeah the pragmas are our thing, what's not possible is specifying a default for an Object (File, Directory, Container) via the pragmas. you can do //+optional I think to just get a nil at runtime that you can re-assign

[rocky meadow]

@ember frost so we're deprecating Optional?

[ember frost]

we've talked about it and I think had consensus on it at one point, but it's been low priority, can't find an issue for it atm

[rocky meadow]

Thanks! I had totally missed it.

Looks like we should clarify which of those two features developers are supposed to use.

I was blissfully ignorant of the choice until now

[hot jetty]

Yeah I found where we discussed removing entirely https://github.com/dagger/dagger/pull/6370 but fell off the radar

The docs no longer use the Optional wrapper, so new authors are steered away from it in that sense. But does leave us in an awkward state of half-existence. I'm not convinced it's worth the breaking change, especially since there are times where it's somewhat nicer. But I guess we do need to document it then, even if under the Advanced section of the docs.

[rocky meadow]

I think the inertia from existing modules having Optional, which will influence others by the power of copy-paste, combined with the docs saying something else, will lead to a split brain situation unless we pick a primary path and actively tell people to follow it

[hot jetty]

Yeah true. Our options then are:

1. Just remove it, everyone has to update
2. Mark as // Deprecated: use // +optional instead, to more actively steer away and then remove down the line. The Go SDK could actually print warnings to progress about this too if it finds usage of it.
3. Shenanigans of trying to automatically update source code from Optional->//+optional (probably not viable)

[tall geyser]

I really like the pragmas. But they aren't equivalent to the Optional because you cant set defaults for certain core types. I'd rather not use two different ways to achieve the same but right now I have no option (pun intended) 🙂

[ember frost]

they both have roughly the same limitation for object types, no? With Optional you do foo_ := foo.GetOr(dag.Container().From("foo") and for the pragmas you use // +optional and if foo == nil { foo = dag.Container().From("foo") }

at least, i think that should work. and even in this example the pragma wins out because you don't need a second foo_ var

[tall geyser]

Ok then I misunderstood. I thought there was a way with Optional to specify a container or a directory as a string. The CLI magically converts the string values to the correct core types. I was hoping for the same magic in the SDK. But I get your point. I just don't set a default and expect nil and handle it every time. That's something that will have to be well documented as I'm sure others will try what I tried to do.

[ember frost]

Ah gotcha - yeah I don't think that exists anywhere

[tender pewter]

So it seems like we definitely agree on removing Optional at some point.
I'd say we should do it before v0.10 - to me it would be really weird to have a feature in a "first release" of something that's already deprecated.

Potentially, we could introduce a "deprecation" in v0.9.9 this week, that warns on using it - and then we remove it in v0.10.0. Maybe that's too much effort, and we can just take it out.

[rocky meadow]

flagging this as a potential launch blocker @runic tapir 👆

[runic tapir]

do you mind adding an internal (eg. linear) issue? Let's flag all 0.10 blockers as Prio: urgent, so it'll be easier to find

[tender pewter]

added in https://linear.app/dagger/issue/DEV-3374/removedeprecate-go-optional-type

[tall geyser]

Sorry I am not clear on one thing. Why can't the engine determine a default for a particular core type at runtime? For example, why is this not possible?

func (f *CoolMod) CoolFunc(
// +optional
// +default=docker:dind
dind *Container,
){
// function code using `dind`
}

I am able to pass in the dind var from the CLI and the engine converts that to a *Container automatically. Why can't the comment pragma be handled the same? It's possible I'm misunderstanding something fundamental. Hence my silly question.

[tender pewter]

There's no explicit reason we couldn't add functionality to do this - but currently, that logic is handled entirely in the CLI

the issue is that there's no way to have more complex defaults - like to be able to take a "docker:dind" and have some additional packages installed with it

[tall geyser]

I think that's implied, passing it from the CLI has the same drawback right?

My concern is more around consistency. There's something that the CLI can do but not the pragmas. I have to handle pragmas differently based on whether they are core types or not.

[rocky meadow]

@tall geyser I think nobody thought about it before. It could either be an awesome convenience, or a confusing leak of CLI convenience into the function's code.

For example, for a directory argument, you might expect this to work: // +default=./src but if won't, because of sandboxing.

It might be better to start with what @ember frost suggested: let it default to nil, then handle that in the function code.

After some mileage, we can re-evaluate if more is needed.

[tender pewter]

yeah, honestly, we never really though about this 😄

[tall geyser]

Alright, we'll wait for further feedback.

My thought process was if I can pass a folder as string via CLI, I should be able to set that same value as a default input. It works great for strings, lists, maps? But not for any of the core types that are specified via strings in the CLI. But I can sort of see the other side to this too. I'll play around with defaulting to nil .

[rocky meadow]

This leads us to the "Ambient System Access" problem: #daggernauts message

The CLI has more privileges than a function, so possible defaults in a function's code doesn't match all possible CLI values.

[tall geyser]

Right, and that could get confusing to devs. Nothing that good documentation can't solve.