-
Notifications
You must be signed in to change notification settings - Fork 16
setup script can communicate an error message to the end user #538
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 1 commit
88134ba
55870c4
eb5e2ac
c0b6fea
e3a37fb
acc6a7f
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 |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
package com.coder.gateway | ||
|
||
import com.coder.gateway.CoderGatewayConstants.GATEWAY_SETUP_COMMAND_ERROR | ||
import com.coder.gateway.cli.CoderCLIManager | ||
import com.coder.gateway.models.WorkspaceProjectIDE | ||
import com.coder.gateway.models.toIdeWithStatus | ||
|
@@ -412,18 +413,16 @@ class CoderRemoteConnectionHandle { | |
) { | ||
if (setupCommand.isNotBlank()) { | ||
indicator.text = "Running setup command..." | ||
try { | ||
exec(workspace, setupCommand) | ||
} catch (ex: Exception) { | ||
if (!ignoreSetupFailure) { | ||
throw ex | ||
} | ||
} | ||
processSetupCommand( | ||
{ exec(workspace, setupCommand) }, | ||
ignoreSetupFailure | ||
) | ||
} else { | ||
logger.info("No setup command to run on ${workspace.hostname}") | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Execute a command in the IDE's bin directory. | ||
* This exists since the accessor does not provide a generic exec. | ||
|
@@ -523,5 +522,25 @@ class CoderRemoteConnectionHandle { | |
|
||
companion object { | ||
val logger = Logger.getInstance(CoderRemoteConnectionHandle::class.java.simpleName) | ||
fun processSetupCommand( | ||
output: () -> String, | ||
kirillk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ignoreSetupFailure: Boolean | ||
) { | ||
try { | ||
val errorText = output | ||
.invoke() | ||
kirillk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.lines() | ||
.firstOrNull { it.contains(GATEWAY_SETUP_COMMAND_ERROR) } | ||
?.let { it.substring(it.indexOf(GATEWAY_SETUP_COMMAND_ERROR) + GATEWAY_SETUP_COMMAND_ERROR.length).trim() } | ||
|
||
if (!errorText.isNullOrBlank()) { | ||
throw Exception(errorText) | ||
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. in Java&Kotlin it is best to avoid throwing the generic exception, for many reasons - but some of them are that it's too generic, and probably the most important one: upstream code can be forced to catch all checked exceptions In other words we should be more specific and throw one of java/kotlin checked exceptions or we can create our own custom exception. IOException is probably good enough, or maybe something custom like CmdExecutionException. 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. I agree, just tried not to change the code much. So do we want a new exception type for this? What do you want it to be called? 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. So I called it 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. |
||
} | ||
} catch (ex: Exception) { | ||
if (!ignoreSetupFailure) { | ||
throw ex | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package com.coder.gateway.util | ||
|
||
import com.coder.gateway.CoderRemoteConnectionHandle.Companion.processSetupCommand | ||
import org.junit.jupiter.api.Test | ||
import org.junit.jupiter.api.assertThrows | ||
import kotlin.test.assertEquals | ||
|
||
internal class SetupCommandTest { | ||
|
||
@Test | ||
fun executionErrors() { | ||
assertEquals( | ||
"Execution error", | ||
assertThrows<Exception> { | ||
processSetupCommand({ throw Exception("Execution error") }, false) | ||
}.message | ||
) | ||
processSetupCommand({ throw Exception("Execution error") }, true) | ||
} | ||
|
||
@Test | ||
fun setupScriptError() { | ||
assertEquals( | ||
"Your IDE is expired, please update", | ||
assertThrows<Exception> { | ||
processSetupCommand({ | ||
""" | ||
execution line 1 | ||
execution line 2 | ||
CODER_SETUP_ERRORYour IDE is expired, please update | ||
execution line 3 | ||
""" | ||
}, false) | ||
}.message | ||
) | ||
|
||
processSetupCommand({ | ||
""" | ||
execution line 1 | ||
execution line 2 | ||
CODER_SETUP_ERRORYour IDE is expired, please update | ||
execution line 3 | ||
""" | ||
}, true) | ||
|
||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.