Skip to content
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

fix interactive TUI #5681

Merged
merged 2 commits into from
Aug 30, 2023
Merged

fix interactive TUI #5681

merged 2 commits into from
Aug 30, 2023

Conversation

vito
Copy link
Contributor

@vito vito commented Aug 23, 2023

It wasn't able to cope with the new 'init' vertex that we create while initializing, and it wasn't transitioning to COMPLETED anymore.

Along the way, also remove the redundant 'Duration' status info now that Progrock displays timing info natively in its UI. Also use Pipeliner to dedupe vertices since that was pretty confusing before.

fixes #5662

Name: "Duration",
Value: time.Since(before).Truncate(time.Millisecond).String(),
Order: 3,
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed now that Progrock shows a timer by default. Note the redundant timing info:

image

@@ -219,259 +218,3 @@ func (i *Item) tasksView() string {

return strings.Join(tasks, "\n")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the noise, I don't think anything actually changed here, just wanted to split it out.

@@ -347,16 +350,41 @@ func (m Model) processUpdate(msg *progrock.StatusUpdate) (tea.Model, tea.Cmd) {
return m, tea.Batch(cmds...)
}

func (m Model) addToFirstGroup(id string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A side fix; I recently changed it to include duplicates vertices, but I think it was ultimately just confusing DX, so now it just picks the first pipeline path that the vertex is seen in, the same way the Cloud telemetry flow works.

@@ -45,12 +45,12 @@ type PipelinedVertex struct {
Pipelines []pipeline.Path
}

func (t *Pipeliner) WriteStatus(ev *progrock.StatusUpdate) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another side fix; there was no need for this type to be a progrock.Writer, and removing the error return value makes the caller's lives easier.

@vito
Copy link
Contributor Author

vito commented Aug 28, 2023

Growing scope a bit to bump to Progrock v0.10 which cleans up the clumsy "multiple root groups" situation by forcing every root group to have the same ID.

it wasn't able to cope with the new 'init' vertex that we create while
initializing

along the way, also remove the redundant 'Duration' status info now that
Progrock displays timing info natively in its UI

Signed-off-by: Alex Suraci <alex@dagger.io>
Signed-off-by: Alex Suraci <alex@dagger.io>
@vito
Copy link
Contributor Author

vito commented Aug 28, 2023

Manually verified root group labels are making their way to Cloud even with the additional 'root group' for client initializing.

cc @marcosnils - tl;dr Progrock now enforces that all "root groups" (i.e. the outermost group for every Recorder) have the exact same ID. Nothing should break but let me know if you see anything strange!

I temporarily updated the TestClientSendsLabelsInTelemetry integration test to log the events sent to Cloud, just to take a peek, and here's what you can expect - note the first Op has no Labels on the root pipeline, and the later ones do:

    engine_test.go:247: received events:
        {"v":"2023-02-28.01","ts":"2023-08-28T18:29:49.619491328Z","run_id":"0b4342b9-a822-421d-8035-137254f1a426","type":"op","payload":{"op_id":"starting-engine","op_name":"connect","pipeline":[{"name":""},{"name":"init","weak":true}],"internal":false,"inputs":null,"started":"2023-08-28T18:29:49.619402409Z","completed":null,"cached":false,"error":""}}
        {"v":"2023-02-28.01","ts":"2023-08-28T18:29:49.619491328Z","run_id":"0b4342b9-a822-421d-8035-137254f1a426","type":"op","payload":{"op_id":"starting-engine","op_name":"connect","pipeline":[{"name":""},{"name":"init","weak":true}],"internal":false,"inputs":null,"started":"2023-08-28T18:29:49.619402409Z","completed":null,"cached":false,"error":""}}
        {"v":"2023-02-28.01","ts":"2023-08-28T18:29:49.870300473Z","run_id":"0b4342b9-a822-421d-8035-137254f1a426","type":"op","payload":{"op_id":"sha256:139a643d0a7ab1c0a2d18773efafbea882afd8bde41814d9b042b1fc33eab058","op_name":"defaultPlatform","pipeline":[{"name":"","labels":[{"name":"dagger.io/git.ref","value":"3975a48a43fc6eb84284c91c18474d6083149fc1"},{"name":"dagger.io/git.author.name","value":"Tiësto User"},{"name":"dagger.io/git.author.email","value":"test@example.com"},{"name":"dagger.io/git.committer.name","value":"Tiësto User"},{"name":"dagger.io/git.committer.email","value":"test@example.com"},{"name":"dagger.io/git.title","value":"init test repo"},{"name":"dagger.io/git.branch","value":"main"},{"name":"dagger.io/engine","value":"7vfu121b7rug6.vfk39ri8sqroc.dagger.local"}]}],"internal":true,"inputs":null,"started":"2023-08-28T18:29:49.868720791Z","completed":null,"cached":false,"error":""}}
        {"v":"2023-02-28.01","ts":"2023-08-28T18:29:49.870300473Z","run_id":"0b4342b9-a822-421d-8035-137254f1a426","type":"op","payload":{"op_id":"sha256:139a643d0a7ab1c0a2d18773efafbea882afd8bde41814d9b042b1fc33eab058","op_name":"defaultPlatform","pipeline":[{"name":"","labels":[{"name":"dagger.io/git.ref","value":"3975a48a43fc6eb84284c91c18474d6083149fc1"},{"name":"dagger.io/git.author.name","value":"Tiësto User"},{"name":"dagger.io/git.author.email","value":"test@example.com"},{"name":"dagger.io/git.committer.name","value":"Tiësto User"},{"name":"dagger.io/git.committer.email","value":"test@example.com"},{"name":"dagger.io/git.title","value":"init test repo"},{"name":"dagger.io/git.branch","value":"main"},{"name":"dagger.io/engine","value":"7vfu121b7rug6.vfk39ri8sqroc.dagger.local"}]}],"internal":true,"inputs":null,"started":"2023-08-28T18:29:49.868720791Z","completed":null,"cached":false,"error":""}}

I think the proper behavior here is probably to just roll up all the labels ever seen for the root group, which might already be what Cloud does.

As a knock-on effect this might mean users now see the initializing/connecting group + vertex in the Cloud UI. Not sure if that's already happening, but it's definitely happening with this change (example). It matches the TUI so this isn't necessarily a bad thing. We could mark it "internal" if we don't want it shown in either, but that's tricky because we at least want to show it in the TUI while the initialization is happening in case it's taking a long time.

@vito vito mentioned this pull request Aug 29, 2023
@vito vito merged commit 0f4c575 into dagger:main Aug 30, 2023
26 of 27 checks passed
@vito vito deleted the fix-interactive-tui branch August 31, 2023 21:22
@gerhard gerhard added this to the v0.8.5 milestone Sep 12, 2023
@gerhard gerhard added the area/cli All about go code on dagger CLI label Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli All about go code on dagger CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐞 Interactive TUI seems broken since Dagger 0.8.x
3 participants