Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/main/kotlin/com/coder/gateway/CoderGatewayConstants.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ package com.coder.gateway
object CoderGatewayConstants {
const val GATEWAY_CONNECTOR_ID = "Coder.Gateway.Connector"
const val GATEWAY_RECENT_CONNECTIONS_ID = "Coder.Gateway.Recent.Connections"
const val GATEWAY_SETUP_COMMAND_ERROR = "CODER_SETUP_ERROR"
}
33 changes: 26 additions & 7 deletions src/main/kotlin/com/coder/gateway/CoderRemoteConnectionHandle.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -523,5 +522,25 @@ class CoderRemoteConnectionHandle {

companion object {
val logger = Logger.getInstance(CoderRemoteConnectionHandle::class.java.simpleName)
fun processSetupCommand(
output: () -> String,
ignoreSetupFailure: Boolean
) {
try {
val errorText = output
.invoke()
.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)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I called it CoderSetupCommandException and provided handling when we show the dialog so it's clearly says what it's a setup command problem, not just an abstract exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

}
} catch (ex: Exception) {
if (!ignoreSetupFailure) {
throw ex
}
}
}
}
}
47 changes: 47 additions & 0 deletions src/test/kotlin/com/coder/gateway/util/SetupCommandTest.kt
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)

}
}
Loading