-
Notifications
You must be signed in to change notification settings - Fork 579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: improve engine session loading logging #5488
Conversation
2aa0828
to
8bd6ab7
Compare
If it's a user-facing change, then yes. Remember to add it in as many places as needed (both TIP: |
5500a01
to
5d9becb
Compare
Picking this one up now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a wording change to the Node.js SDK. Does this read better to you @TomChv?
Before:
Starting Dagger session...
Dagger session started! Establishing connection with the SDK...
Connection established with Dagger session version 0.0.0
After:
Creating new Engine session... OK!
Establishing connection to Engine... OK!
I also removed the the SDK version from the output since it will be confusing if/when the Engine/CLI version differs from the SDK version.
WDYT?
@gerhard LGTM, let's merge it after I also update the Go SDK with the same changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing the download step (i.e., when no CLI is found in the cache, it's downloaded).
096c33b
to
8f00d7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there 🙂
39184ea
to
693a861
Compare
3429bff
to
45741c7
Compare
It's very noisy when running multiple connects (e.g., in tests): ❯ go test
Downloading CLI... Downloading CLI... Downloading CLI... Downloading CLI... Downloading CLI... Downloading CLI... Downloading CLI... Downloading CLI... Downloading CLI... Downloading CLI... OK!
Creating new Engine session... OK!
Establishing connection to Engine... OK!
OK!
Creating new Engine session... OK!
Establishing connection to Engine... Creating new Engine session... OK!
Establishing connection to Engine... OK!
OK!
Creating new Engine session... OK!
Establishing connection to Engine... OK!
Creating new Engine session... OK!
OK!
Establishing connection to Engine... Creating new Engine session... OK!
Establishing connection to Engine... OK!
OK!
OK!
Creating new Engine session... OK!
Establishing connection to Engine... Creating new Engine session... Creating new Engine session... OK!
Establishing connection to Engine... Creating new Engine session... OK!
Establishing connection to Engine... OK!
Establishing connection to Engine... OK!
OK!
OK!
OK!
OK!
OK!
OK!
OK!
OK!
OK!
PASS
ok dagger.io/dagger 46.922s In this case, the |
This is why I wanted to add a special flag so we can explicitely trigger the display of the loading |
Python implementation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do follow ups to improve on this, but lgtm for now. Rather have the fixed test, but non blocking.
One approach I took in Python was to only do this on TTY. The intent is to show that something's happening when it's run interactively, not when writing to a file for example. I understand why you wanted the flag, but I'm more inclined for any smart detection we can make, up to a general escape hatch for the default of showing the output. Not a flag for explicitly enabling it. In Python there's an escape hatch, but not a specific setting for it either. And detecting for TTY probably avoids some of situations where you'd want to escape it anyway. As for #5488 (comment), that test is testing concurrent provisioning for the automatic provisioning feature in the SDK, so it explicitly ignores an existing session. I say it's not a big deal in this case and we can remove LogOutput from the test. We'll be focusing more on the CLI experience going forward so hopefully less and less users will be running off the SDK directly in the future. In the meantime this is the problematic situation that remains. It's if someone is doing multiple However, in this PR I think we can cross that bridge when we get there and improve then. This already feels like an improvement, and you can add the tty check as well. |
Add few lines during engine loading to update user. This PR update Typescript and Go SDK. Related to: dagger#4820 Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Remove the SDK version from the output since it will be confusing if/when the Engine/CLI version differs from the SDK version. Before: Starting Dagger session... Dagger session started! Establishing connection with the SDK... Connection established with Dagger session version 0.0.0 After: Creating new Engine session... OK! Establishing connection to Engine... OK! Signed-off-by: Gerhard Lazu <gerhard@dagger.io>
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Signed-off-by: Gerhard Lazu <gerhard@dagger.io>
Signed-off-by: Gerhard Lazu <gerhard@dagger.io>
Without this, users will need to do an extra step to get this output. The idea is that this should work better **out of the box**, without adding an extra burden on users. Signed-off-by: Gerhard Lazu <gerhard@dagger.io>
Remove `EngineLoading` output. Add extra log when downloading CLI. Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
45741c7
to
0facd07
Compare
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Add few lines during engine loading to update user. This PR update Typescript and Go SDK.
Related to: #4820
Example on Typescript SDK
Example on Go SDK
@gerhard Should I add a change in the release note with this PR? I think it's a really small change but it can be useful!
Note: Originally, I tried to do something with a progress bar but it became really complex for just a logging issue, I went back to something simpler but still effective!
@helderco I saw you already did some work on logging, should I also add these print in the Python SDK?
EDIT: NEW FORMAT
And in case of CLI downloading: