#dagger login broken in 0.12?

1 messages · Page 1 of 1 (latest)

finite pebble
#

Could someone please take the lead on coordinating a response? I am on a train with bad wifi and about to transfer to my next train..

keen palm
#

yeah no worries, i can take this for now

#

fyi - i'm thinking about when we should target the next release

#

this issue perhaps bumps the priority, we can try for this thursday

#

will start a thread towards eod

finite pebble
#

What worries me is that people who encounter this problem may 1) get stuck and complain about it, or 2) find a "workaround" that actually leaves their CLI unauthenticated without them realizing it.

#

So it's not a clean fix for those affected, once we ship a 0.12.1

#

I guess I'm saying thursday seems far away

keen palm
#

fair enough - both me and gerhard are not able to release tomorrow

#

us-side could lead one

finite pebble
#

(perhaps I'm overestimating how many people are affected. But not sure how to get a clearer picture there)

keen palm
#

mm, it'll likely be a lot of macos users

#

if iterm2 is having the issue

#

hm, is iterm2 having the issue?

#

huh, i think zed has a bug

#

echo https://example.com?foo=bar doesn't produce a clickable link

#

but https://example.com does

#

i don't have access to ghostty - not sure how i would, and it seems like it's a mac build

#

i'm now not actually convinced this is a dagger bug - zed clearly is bugged here, i know ghostty isn't prod ready

#

if we can't replicate this in a major, stable terminal emulator, i don't think this is actually a dagger bug

severe radish
#

I think Zed uses Alacritty as it's engine, not Ghostty

keen palm
#

sorry, yes, but solomon mentioned in the issue that he encounters the issue both in ghostty and zed

#

hm damn, i can reproduce the issue in alacritty

#

zed also has the separate issue that all query params are not detected

finite pebble
#

vscode stock configuration not affected?

keen palm
#

not for me at least

#

i wonder if alacritty also has a bug here - but i wonder what we're doing that's disabling the ability to select any text

finite pebble
#

dagger login broken in 0.12?

keen palm
#

i can imagine the code overlaps for sure

#

aha

#

mouse selection has to do with WithMouseCellMotion

#

disabling this option causes alacritty to "do the right thing" and allow clicking the link

#

currently looking through alacritty code to see if this is intentional or not

severe radish
#

yeah, that's consistent with what i'd expect - you have to ctrl+click or shift+click to bypass the mouse cell motion input

#

which should let you click the URL, but is obviously not something our users are likely to try (unless they're super familiar with CLI tools)

#

we'll likely need some way to disable mouse input at runtime, or maybe just background the TUI (same way dagger terminal works)

keen palm
#

no, alacritty doesn't let you click any terminal link if terminal input is disabled

#

even if you control clikc etc

severe radish
#

try ctrl+shift?

#

i've had to use both

keen palm
#

ahhh

#

yeah that does it

#

one possible idea - could we handle these events ourselves in the tui code?

severe radish
#

clickable regions are a bit of a pain to implement

keen palm
#

is not using the tui at all for this an option? same for dagger/version/etc.

#

it feels like a smell tho, i can't put my finger on why

severe radish
#

yea, for these simple commands it's really overkill

#

none of the buttons on the keymap are relevant

keen palm
#

yeah, and we don't have traces, etc

severe radish
#

right - maybe that's the rule of thumb; if there's no trace involved, we don't need the TUI, or we at they very least don't need mouse input / etc

keen palm
#

i wonder if we could have a property on a cobra command, and determine it from there

severe radish
#

the trick is it's hard to know when a trace is involved, because we have to start the TUI before even parsing os.Args

#

which is why i think just backgrounding it like terminal does is probably the shortest path for now

keen palm
#

yeah fair enough

severe radish
#

looks like you can also just call fe.program.DisableMouseCellMotion(), wonder if that fixes it

keen palm
#

hm just backgrounding it is a bit weird

#

just tried it

#

since we're still in raw mode

#

so, ctrl+c and similar no longer work, and we have no "\n" = "\r\n", etc

severe radish
#

ah right, which makes sense for terminal

keen palm
#

yeah, but less so for this sadly

severe radish
#

I guess we could have a hook like Frontend.SimpleMode() that disables mouse input + other UI chrome (like the keymap)

keen palm
#

i think it might be a bit much to try taking the terminal entirely out of raw mode

severe radish
#

yeah

keen palm
#

that works i think

remote sandal
#

@severe radish @keen palm does any of you have the bandwidth to take this one? Otherwise, I can take a look later today

keen palm
#

yeah, i'm looking at this now

remote sandal
#

🙏

#

@keen palm I've also noticed that dagger version hides the output at the end and it shouldn't. Probably same behavior as some commands not being friendly with the current TUI flow

sleek siren
# remote sandal 🙏

I just had that happen to me, but inconsistently.
For example, last night, version output was erased/hidden at the end, but then on subsequent runs it appears???

Desktop ➤ dagger version
dagger v0.12.0 (registry.dagger.io/engine) darwin/arm64
Desktop ➤ dagger version
dagger v0.12.0 (registry.dagger.io/engine) darwin/arm64
Desktop ➤ dagger version
dagger v0.12.0 (registry.dagger.io/engine) darwin/arm64
Desktop ➤ dagger version
dagger v0.12.0 (registry.dagger.io/engine) darwin/arm64
Desktop ➤ dagger version
dagger v0.12.0 (registry.dagger.io/engine) darwin/arm64
keen palm
#

the version one is super easy i think

#

should be solved by this one

sleek siren
sleek siren
# keen palm should be solved by this one

can likely be harmonized then with 👆 if we implement that. In 7856 we're suggesting to not send the trace for dagger version (and most others) to cloud by default (maybe send with --debug), but we'd want local behavior to be great.

I'd prefer to not show the navigation bar when it's not appropriate.

finite pebble
#

👋 I made it on my next train (just barely). I can try things if helpful.

sleek siren
#

login/logout experience on Mac iterm2 for me 👆

#

tl;dr I CMD+hover to get clickable link.

#

I'm used to trying that, not sure everyone is.

#

I do get annoyed when I can't select text to copy-paste. I have that issue in Daggerverse in a few places too.

keen palm
#

trying to consider if we can push this further down into each command

#

which would let us to the tracing thing as mentioned in 7856

severe radish
keen palm
#

i'm not sure that's entirely true - for example, dagger version doesn't load a module at all

severe radish
#

sure, but we don't even know if we're running version without parsing the CLI lol

#

it's a bit of a knot

keen palm
#

hm, i'm confused then

finite pebble
#

IMO our arg parsing is too tightly coupled to modules

#

In theory we could easily do a first pass with a "regular" cobra, then if we detect a subcommand that requires loading module (eg. call), do another "smarter" pass with schema introspection etc.

keen palm
#

i think this is what we do?

severe radish
#

kinda/sorta, we do that for global flags but not commands

#

maybe we could extend that to have 'primitive' commands parsed at that same phase

tired garden
#

Global commands do a first pass, then dagger call does another pass before loading the module and parsing the chain (for -m, etc). At which point each piece is parsed until next positional arg.

severe radish
#

right but that all happens in one Execute call I think?

keen palm
#

this is called after we know what func it is

#

but before we do anything major

tired garden
#

Just catching up now... don't know what the issue is.

keen palm
#

yeah, confirmed using a panic

#

if we shift telemetry init to inside there, then that means we can disable it for everything except call/run/functions/query

finite pebble
#

dagger init without --sdk shouldn't have TUI either - it has traces but only because is spins up engine, which it shouldn't do in the first place

severe radish
#

side note: would it be the worst thing in the world if dagger login just automatically opened your browser? it's not like you have any other option

keen palm
#

yeah, that is also good

severe radish
#

pretty sure i've used other CLIs that do that

finite pebble
#

It would certainly make fixing the rest less urgent

severe radish
#

yeah, just trying to not rush into CLI surgery since I remember there being some benefits to establishing the TUI/telemetry super duper early

finite pebble
#

But I do think we should disable TUI altogether for dagger login unless it's completely impossible. The effort justifies itself because of the other commands that as we discussed, don't need TUI either.

severe radish
#

also true (e.g. dagger version showing up in Cloud is funny)

finite pebble
severe radish
#

yeah, that'd be my preference. I think there's more to do here for sure (dagger version case I mentioned came up independently), just don't want to throw the baby out with the bathwater

#

trying to dig up messages when I originally made this change and talked about it, I specifically remember it helping to untie a different knot that had been haunting me in the CLI. but it'd all be in the flurry of the OTel switch

keen palm
#

okay, i think i have a fix maybe

severe radish
#

good job writing about those things, ALEX

image.png
keen palm
#

we probably want telemetry everywhere we have an engine

#

so we can just stick telemetry init in there

#

so it's disabled on version + login + logout

severe radish
#

eventually I ended up thinking, it's simplest to just establish the frontend ASAP and if you need to do things per-command, do them dynamically at runtime by interacting with the frontend

#

so moving this back to how it was before (this is basically withEngineAndTUI pre-otel) spooks me a little

#

tl;dr I've got a lot of FUD with this change, so if we're gonna do it as a quick fix patch release, I'd suggest we do a ton of hand-testing including error cases

#

maybe it's fine at this point and we've gradually refactored out of the position that made this untenable before (by e.g. consistently using cmd.OutOrStdout() and slog everywhere)

keen palm
#

mm, i agree on not wanting to rush this one actually

finite pebble
#

works for me if we auto-open browser & not drop the follow up 😛

keen palm
#

even the little "suspend" of Disable/Enable mouse events, I don't really like, it's such a weird user experience

severe radish
#

maybe instead we make the Frontend less feature-ful by default, and have call/etc. graduate it into the more feature-ful mode?

#

or, like I said, maybe it's fine now, I'll try to remember the specific footguns from before, it's just been a while

keen palm
#

which does seem very weird to me

#

as in, that feels like another lurking footgun

severe radish
#

Frontend doesn't have to mean Telemetry too

keen palm
#

true

#

yeah, this is all starting to blur for me, sorry, i'm definitely kinda out of it right now

#

more coffee needed

severe radish
#

lol np, it's confusing on its own, even more confusing to chat about, just gotta try things out i think. I can take a swing at this too

keen palm
#

yeah, sure 🎉 you're taking charge of this one then?

keen palm
severe radish
severe radish
keen palm
#

yup 😱

severe radish
#

(curious how that happened, will check it out for posterity, but brb)

keen palm
#

ah fixed it, ordering is important 👀

#

you have to init telemetry before referencing processors

severe radish
#

nice