Repository navigation
fix: show progress for workspace actions #361
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,7 +274,11 @@ class CoderCLIManager( | |
| /** | ||
| * Start a workspace. Throws if the command execution fails. | ||
| */ | ||
| internal fun startWorkspace(wsAddress: WorkspaceAddress, feats: Features = features): String { | ||
| internal fun startWorkspace( | ||
| wsAddress: WorkspaceAddress, | ||
| feats: Features = features, | ||
| showTextProgress: (String) -> Unit = {}, | ||
| ): String { | ||
| val args = mutableListOf( | ||
| "--global-config", | ||
| coderConfigPath.toString(), | ||
|
|
@@ -288,7 +292,7 @@ class CoderCLIManager( | |
| args.add("--") | ||
| args.add(wsAddress.ownerAndWsName) | ||
|
|
||
| return exec(*args.toTypedArray()) | ||
| return exec(*args.toTypedArray(), showTextProgress = showTextProgress) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -571,12 +575,22 @@ class CoderCLIManager( | |
| return matches | ||
| } | ||
|
|
||
| private fun exec(vararg args: String): String { | ||
| val stdout = | ||
| private fun exec( | ||
| vararg args: String, | ||
| showTextProgress: ((String) -> Unit)? = null, | ||
| ): String { | ||
| val processExecutor = | ||
| ProcessExecutor() | ||
| .command(localBinaryPath.toString(), *args) | ||
| .environment("CODER_HEADER_COMMAND", context.settingsStore.headerCommand) | ||
| .exitValues(0) | ||
|
|
||
| showTextProgress?.let { reportProgress -> | ||
| processExecutor.redirectOutput(reportProgress::invoke) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not something we have to actually worry about, but do you know if this buffers by newline or anything like that? Thinking it could be weird (although unlikely) if half a sentence appears, then is replaced by the next half, just depending on how the output happens to be buffered.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're using zt-exec for now (though I have a PR in review of OAuth2 support that gets rid of the library because it had some drawbacks and I need to make a note about your question to be researched there as well) and zt-exec is line-buffered and it accumulates bytes until it encounters \n or \r. |
||
| } | ||
|
|
||
| val stdout = | ||
| processExecutor | ||
| .readOutput(true) | ||
| .execute() | ||
| .outputUTF8() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package com.coder.toolbox.sdk.v2.models | ||
|
|
||
| import com.squareup.moshi.Json | ||
| import com.squareup.moshi.JsonClass | ||
| import java.time.Instant | ||
|
|
||
| @JsonClass(generateAdapter = true) | ||
| data class ProvisionerJobLog( | ||
| @property:Json(name = "id") val id: Long, | ||
| @property:Json(name = "created_at") val createdAt: Instant, | ||
| @property:Json(name = "log_source") val source: String, | ||
| @property:Json(name = "log_level") val level: String, | ||
| @property:Json(name = "stage") val stage: String, | ||
| @property:Json(name = "output") val output: String, | ||
| ) |
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 wonder if we should add
follow=trueto the build log request? That turns it into a web socket and lets us watch the logs.Otherwise we may be sitting there without more logs unless/until
updateruns again.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 would keep this PR as-is and handle WebSocket following separately, as it is a more complex solution.
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.
Makes sense!