From 10cdff22b2067a0c9cfa5baac8db7a7854f7c79e Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 9 May 2023 11:21:40 -0800 Subject: [PATCH 1/3] Bypass download if CLI version matches This is a bit faster than waiting for a 304 but more importantly it works if the user customizes the download source to be a place that does not have the same etag support that Coder does. The test bypasses Windows since we would need to create an executable there for the test. We could split out the exec and the parsing although I am not sure it is worth the noise. The part most likely to break between platforms is probably the exec, not the parsing? --- .../com/coder/gateway/sdk/CoderCLIManager.kt | 21 +++++++-- .../views/steps/CoderWorkspacesStepView.kt | 13 +++++- src/test/groovy/CoderCLIManagerTest.groovy | 43 ++++++++++++++----- 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt b/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt index 4ea27219..e695a9fd 100644 --- a/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt +++ b/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt @@ -2,6 +2,7 @@ package com.coder.gateway.sdk import com.coder.gateway.models.WorkspaceAgentModel import com.coder.gateway.views.steps.CoderWorkspacesStepView +import com.google.gson.Gson import com.intellij.openapi.diagnostic.Logger import org.zeroturnaround.exec.ProcessExecutor import java.io.BufferedInputStream @@ -277,10 +278,24 @@ class CoderCLIManager @JvmOverloads constructor( } /** - * Return the binary version. + * Version output from the CLI's version command. */ - fun version(): String { - return exec("version") + private data class Version( + val version: String, + ) + + /** + * Return the binary version or null if it could not be determined. + */ + fun version(): String? { + return try { + val raw = exec("version", "--output", "json") + val json = Gson().fromJson(raw, Version::class.java) + json.version + } catch (e: Exception) { + logger.warn("Unable to determine CLI version: ${e.message}") + null + } } private fun exec(vararg args: String): String { diff --git a/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt b/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt index 066b71eb..0a8e1e8c 100644 --- a/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt +++ b/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt @@ -467,8 +467,17 @@ class CoderWorkspacesStepView(val setNextButtonEnabled: (Boolean) -> Unit) : Cod appPropertiesService.setValue(CODER_URL_KEY, deploymentURL.toString()) appPropertiesService.setValue(SESSION_TOKEN, token.first) - this.indicator.text = "Downloading Coder CLI..." - cliManager.downloadCLI() + // Short-circuit if we already have the expected version. This + // lets us bypass the 304 which is slower and may not be + // supported if the binary is downloaded from alternate sources. + // For CLIs without the JSON output flag we will fall back to + // the 304 method. + if (cliManager.version() != clientService.buildVersion) { + this.indicator.text = "Downloading Coder CLI..." + cliManager.downloadCLI() + } else { + logger.info("Found existing binary at ${cliManager.localBinaryPath} with the expected version ${clientService.buildVersion}") + } this.indicator.text = "Authenticating Coder CLI..." cliManager.login(token.first) diff --git a/src/test/groovy/CoderCLIManagerTest.groovy b/src/test/groovy/CoderCLIManagerTest.groovy index 9c135c88..58730467 100644 --- a/src/test/groovy/CoderCLIManagerTest.groovy +++ b/src/test/groovy/CoderCLIManagerTest.groovy @@ -5,10 +5,7 @@ import com.sun.net.httpserver.HttpHandler import com.sun.net.httpserver.HttpServer import org.zeroturnaround.exec.InvalidExitValueException import org.zeroturnaround.exec.ProcessInitException -import spock.lang.Requires -import spock.lang.Shared -import spock.lang.Specification -import spock.lang.Unroll +import spock.lang.* import java.nio.file.Files import java.nio.file.Path @@ -137,7 +134,7 @@ class CoderCLIManagerTest extends Specification { then: downloaded - ccm.version().contains("Coder") + CoderSemVer.isValidVersion(ccm.version()) // Make sure login failures propagate correctly. when: @@ -161,7 +158,7 @@ class CoderCLIManagerTest extends Specification { // The mock does not serve a binary that works on Windows so do not // actually execute. Checking the contents works just as well as proof // that the binary was correctly downloaded anyway. - ccm.localBinaryPath.toFile().text == "#!/bin/sh\necho '$url'" + ccm.localBinaryPath.toFile().text.contains(url) cleanup: srv.stop(0) @@ -172,7 +169,7 @@ class CoderCLIManagerTest extends Specification { def ccm = new CoderCLIManager(new URL("https://foo"), tmpdir.resolve("does-not-exist")) when: - ccm.version() + ccm.login("token") then: thrown(ProcessInitException) @@ -193,7 +190,7 @@ class CoderCLIManagerTest extends Specification { downloaded ccm.localBinaryPath.toFile().readBytes() != "cli".getBytes() ccm.localBinaryPath.toFile().lastModified() > 0 - ccm.localBinaryPath.toFile().text == "#!/bin/sh\necho '$url'" + ccm.localBinaryPath.toFile().text.contains(url) cleanup: srv.stop(0) @@ -207,6 +204,7 @@ class CoderCLIManagerTest extends Specification { when: def downloaded1 = ccm.downloadCLI() ccm.localBinaryPath.toFile().setLastModified(0) + // Download will be skipped due to a 304. def downloaded2 = ccm.downloadCLI() then: @@ -231,8 +229,8 @@ class CoderCLIManagerTest extends Specification { then: ccm1.localBinaryPath != ccm2.localBinaryPath - ccm1.localBinaryPath.toFile().text == "#!/bin/sh\necho '$url1'" - ccm2.localBinaryPath.toFile().text == "#!/bin/sh\necho '$url2'" + ccm1.localBinaryPath.toFile().text.contains(url1) + ccm2.localBinaryPath.toFile().text.contains(url2) cleanup: srv1.stop(0) @@ -249,7 +247,7 @@ class CoderCLIManagerTest extends Specification { then: downloaded - ccm.localBinaryPath.toFile().text == "#!/bin/sh\necho '${expected.replace("{{url}}", url)}'" + ccm.localBinaryPath.toFile().text.contains(expected.replace("{{url}}", url)) cleanup: srv.stop(0) @@ -429,4 +427,27 @@ class CoderCLIManagerTest extends Specification { "malformed-start-after-end", ] } + + @IgnoreIf({ os.windows }) + def "parses version"() { + given: + def ccm = new CoderCLIManager(new URL("https://test.coder.invalid"), tmpdir) + Files.createDirectories(ccm.localBinaryPath.parent) + + when: + ccm.localBinaryPath.toFile().text = "#!/bin/sh\n$contents" + ccm.localBinaryPath.toFile().setExecutable(true) + + then: + ccm.version() == expected + + where: + contents | expected + """echo '{"version": "1.0.0"}'""" | "1.0.0" + """echo '{"version": "1.0.0", "foo": true, "baz": 1}'""" | "1.0.0" + """echo '{"foo": true, "baz": 1}'""" | null + """echo '{"version: "1.0.0", "foo": true, "baz": 1}'""" | null + "exit 0" | null + "exit 1" | null + } } From 7e119694e1214a7d2e3ecec77f92f34590d45e52 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 10 May 2023 09:29:39 -0800 Subject: [PATCH 2/3] Make invalid JSON test less obscure --- src/test/groovy/CoderCLIManagerTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/groovy/CoderCLIManagerTest.groovy b/src/test/groovy/CoderCLIManagerTest.groovy index 58730467..0a275ff2 100644 --- a/src/test/groovy/CoderCLIManagerTest.groovy +++ b/src/test/groovy/CoderCLIManagerTest.groovy @@ -446,7 +446,7 @@ class CoderCLIManagerTest extends Specification { """echo '{"version": "1.0.0"}'""" | "1.0.0" """echo '{"version": "1.0.0", "foo": true, "baz": 1}'""" | "1.0.0" """echo '{"foo": true, "baz": 1}'""" | null - """echo '{"version: "1.0.0", "foo": true, "baz": 1}'""" | null + """echo '{"version: """ | null "exit 0" | null "exit 1" | null } From ebe4bed06c2a4accf30e0d94f84321c0920f6353 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 10 May 2023 09:28:35 -0800 Subject: [PATCH 3/3] Properly compare semver --- .../com/coder/gateway/sdk/CoderCLIManager.kt | 31 ++++++-- .../com/coder/gateway/sdk/CoderSemVer.kt | 2 +- .../views/steps/CoderWorkspacesStepView.kt | 4 +- src/test/groovy/CoderCLIManagerTest.groovy | 72 +++++++++++++++++-- 4 files changed, 91 insertions(+), 18 deletions(-) diff --git a/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt b/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt index e695a9fd..13b064f9 100644 --- a/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt +++ b/src/main/kotlin/com/coder/gateway/sdk/CoderCLIManager.kt @@ -285,16 +285,33 @@ class CoderCLIManager @JvmOverloads constructor( ) /** - * Return the binary version or null if it could not be determined. + * Return the binary version. + * + * Throws if it could not be determined. */ - fun version(): String? { + fun version(): CoderSemVer { + val raw = exec("version", "--output", "json") + val json = Gson().fromJson(raw, Version::class.java) + if (json?.version == null) { + throw InvalidVersionException("No version found in output") + } + return CoderSemVer.parse(json.version) + } + + /** + * Returns true if the CLI has the same major/minor/patch version as the + * provided version and false if it does not match or the CLI version could + * not be determined or the provided version is invalid. + */ + fun matchesVersion(buildVersion: String): Boolean { return try { - val raw = exec("version", "--output", "json") - val json = Gson().fromJson(raw, Version::class.java) - json.version + val cliVersion = version() + val matches = cliVersion == CoderSemVer.parse(buildVersion) + logger.info("$localBinaryPath version $cliVersion matches $buildVersion: $matches") + matches } catch (e: Exception) { - logger.warn("Unable to determine CLI version: ${e.message}") - null + logger.info("Unable to determine $localBinaryPath version: ${e.message}") + false } } diff --git a/src/main/kotlin/com/coder/gateway/sdk/CoderSemVer.kt b/src/main/kotlin/com/coder/gateway/sdk/CoderSemVer.kt index 2d970f50..d97b19b6 100644 --- a/src/main/kotlin/com/coder/gateway/sdk/CoderSemVer.kt +++ b/src/main/kotlin/com/coder/gateway/sdk/CoderSemVer.kt @@ -14,7 +14,7 @@ class CoderSemVer(private val major: Long = 0, private val minor: Long = 0, priv override fun toString(): String { - return "CoderSemVer(major=$major, minor=$minor)" + return "CoderSemVer(major=$major, minor=$minor, patch=$patch)" } override fun equals(other: Any?): Boolean { diff --git a/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt b/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt index 0a8e1e8c..1a4c17f8 100644 --- a/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt +++ b/src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt @@ -472,11 +472,9 @@ class CoderWorkspacesStepView(val setNextButtonEnabled: (Boolean) -> Unit) : Cod // supported if the binary is downloaded from alternate sources. // For CLIs without the JSON output flag we will fall back to // the 304 method. - if (cliManager.version() != clientService.buildVersion) { + if (!cliManager.matchesVersion(clientService.buildVersion)) { this.indicator.text = "Downloading Coder CLI..." cliManager.downloadCLI() - } else { - logger.info("Found existing binary at ${cliManager.localBinaryPath} with the expected version ${clientService.buildVersion}") } this.indicator.text = "Authenticating Coder CLI..." diff --git a/src/test/groovy/CoderCLIManagerTest.groovy b/src/test/groovy/CoderCLIManagerTest.groovy index 0a275ff2..166a04cf 100644 --- a/src/test/groovy/CoderCLIManagerTest.groovy +++ b/src/test/groovy/CoderCLIManagerTest.groovy @@ -1,5 +1,6 @@ package com.coder.gateway.sdk +import com.google.gson.JsonSyntaxException import com.sun.net.httpserver.HttpExchange import com.sun.net.httpserver.HttpHandler import com.sun.net.httpserver.HttpServer @@ -131,10 +132,11 @@ class CoderCLIManagerTest extends Specification { when: def downloaded = ccm.downloadCLI() + ccm.version() then: downloaded - CoderSemVer.isValidVersion(ccm.version()) + noExceptionThrown() // Make sure login failures propagate correctly. when: @@ -443,11 +445,67 @@ class CoderCLIManagerTest extends Specification { where: contents | expected - """echo '{"version": "1.0.0"}'""" | "1.0.0" - """echo '{"version": "1.0.0", "foo": true, "baz": 1}'""" | "1.0.0" - """echo '{"foo": true, "baz": 1}'""" | null - """echo '{"version: """ | null - "exit 0" | null - "exit 1" | null + """echo '{"version": "1.0.0"}'""" | CoderSemVer.parse("1.0.0") + """echo '{"version": "1.0.0", "foo": true, "baz": 1}'""" | CoderSemVer.parse("1.0.0") + } + + @IgnoreIf({ os.windows }) + def "fails to parse version"() { + given: + def ccm = new CoderCLIManager(new URL("https://test.coder.parse-fail.invalid"), tmpdir) + Files.createDirectories(ccm.localBinaryPath.parent) + + when: + if (contents != null) { + ccm.localBinaryPath.toFile().text = "#!/bin/sh\n$contents" + ccm.localBinaryPath.toFile().setExecutable(true) + } + ccm.version() + + then: + thrown(expected) + + where: + contents | expected + null | ProcessInitException + """echo '{"foo": true, "baz": 1}'""" | InvalidVersionException + """echo '{"version: '""" | JsonSyntaxException + "exit 0" | InvalidVersionException + "exit 1" | InvalidExitValueException + } + + @IgnoreIf({ os.windows }) + def "checks if version matches"() { + given: + def ccm = new CoderCLIManager(new URL("https://test.coder.version-matches.invalid"), tmpdir) + Files.createDirectories(ccm.localBinaryPath.parent) + + when: + if (contents != null) { + ccm.localBinaryPath.toFile().text = "#!/bin/sh\n$contents" + ccm.localBinaryPath.toFile().setExecutable(true) + } + + then: + ccm.matchesVersion(build) == matches + + where: + contents | build | matches + null | "v1.0.0" | false + """echo '{"version": "v1.0.0"}'""" | "v1.0.0" | true + """echo '{"version": "v1.0.0"}'""" | "v1.0.0-devel+b5b5b5b5" | true + """echo '{"version": "v1.0.0-devel+b5b5b5b5"}'""" | "v1.0.0-devel+b5b5b5b5" | true + """echo '{"version": "v1.0.0-devel+b5b5b5b5"}'""" | "v1.0.0" | true + """echo '{"version": "v1.0.0-devel+b5b5b5b5"}'""" | "v1.0.0-devel+c6c6c6c6" | true + """echo '{"version": "v1.0.0-prod+b5b5b5b5"}'""" | "v1.0.0-devel+b5b5b5b5" | true + """echo '{"version": "v1.0.0"}'""" | "v1.0.1" | false + """echo '{"version": "v1.0.0"}'""" | "v1.1.0" | false + """echo '{"version": "v1.0.0"}'""" | "v2.0.0" | false + """echo '{"version": "v1.0.0"}'""" | "v0.0.0" | false + """echo '{"version": ""}'""" | "v1.0.0" | false + """echo '{"version": "v1.0.0"}'""" | "" | false + """echo '{"version'""" | "v1.0.0" | false + """exit 0""" | "v1.0.0" | false + """exit 1""" | "v1.0.0" | false } }