Refactor CLI manager to support multiple deployments #213
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
code-asher
force-pushed
the
refactor-cli-manager
branch
9 times, most recently
from
0e8c4e0 to
171555b
Compare
code-asher
force-pushed
the
refactor-cli-manager
branch
5 times, most recently
from
fa39f57 to
49235fb
Compare
Also rename the function since it loads all workspaces, not just one.
Since the comments are now out of date I removed them; I figure it is more or less as easy to just read the workflow to see what it does.
code-asher
force-pushed
the
refactor-cli-manager
branch
6 times, most recently
from
a522c74 to
b5ef3df
Compare
Currently multiple deployments will delete each other's binaries so this uses a directory for each deployment host to store the binaries. Add a catch for download errors; previously they would go unnoticed. Also refactor the download a little; instead of checking for the file's existence catch the appropriate error and set the permissions from Kotlin code rather than spawning chmod. Remove a chunk of code where we configure the CLI again before moving to the next step; we already do that when we first connect (plus it is missing some checks like making sure the CLI has actually been downloaded and is the right version, etc). This also makes it unnecessary to globally store the version and CLI path on the model since we now only do it in one spot. This is just a first step; in a future PR the config code will all be extracted out since we will want to configure every time we try to connect (rather than just in the initial setup step) since otherwise the recent connections can fail if you have configured a different deployment in the meantime (and probably reconnections would also fail if you already have an IDE open) or if you have restarted (since the binaries are currently in tmp). We will also need to configure ourselves since `config-ssh` does not support multiple deployments and we want to add an environment variable via SetEnv.
We will need to create a new function for config-ssh anyway since we will be doing our own thing rather than spawning the CLI in order to support multiple deployments so this preemptively encapsulates that logic.
code-asher
force-pushed
the
refactor-cli-manager
branch
from
b5ef3df to
125b1f2
Compare
johnstcn
reviewed
Apr 4, 2023
src/main/kotlin/com/coder/gateway/views/steps/CoderWorkspacesStepView.kt
Outdated
Show resolved
Hide resolved
This way we will not need to name the binaries with their versions or otherwise extract the version from each binary since we can just grab their hash and set that in the request. This also means we will not need to remove old versions as there will only be the one file.
The @jvmoverloads annotations make Kotlin generate overloads otherwise the test code thinks the optional arguments are not optional.
code-asher
force-pushed
the
refactor-cli-manager
branch
from
0241244 to
6636f31
Compare
code-asher
force-pushed
the
refactor-cli-manager
branch
3 times, most recently
from
c0805a2 to
6a0895a
Compare
Environment variables can be used to test against a real deployment. I now just check if the file is executable rather than trying to actually execute it since we would need to actually compile something that can run as a .exe on Windows.
code-asher
force-pushed
the
refactor-cli-manager
branch
from
6a0895a to
9b0d21e
Compare
code-asher
force-pushed
the
refactor-cli-manager
branch
from
48e19fd to
0c47258
Compare
That is enough to prove the downloading works and the rest can just test their own extra assumptions on top of that.
code-asher
force-pushed
the
refactor-cli-manager
branch
from
c8db570 to
1e87ae3
Compare
johnstcn
approved these changes
Apr 7, 2023
| def ccm = new CoderCLIManager(new URL("https://test.😉.invalid")) | ||
|
|
||
| expect: | ||
| ccm.localBinaryPath.getParent() == CoderCLIManager.getDataDir().resolve("test.xn--n28h.invalid") |
Comment on lines
+109
to
+112
| def url = System.getenv("CODER_GATEWAY_TEST_DEPLOYMENT") | ||
| if (url == null) { | ||
| url = "https://dev.coder.com" | ||
| } |
| private fun getBinaryETag(): String? { | ||
| try { | ||
| return try { |
There was a problem hiding this comment.
👀 this is a cool thing that I didn't know was a thing!
There was a problem hiding this comment.
Same!! Thank goodness for IDE suggestions lol
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is just phase one; the SSH config itself does not support multiple deployments yet. For that I think I will end up doing a prefix thing just like our VS Code extension does.
I enabled auto-formatting so there are a bunch of added newlines, indents, and import optimizations.Edit: Going to remove these changes, too much noise in the PR. Also should be using explicit imports as that is the general practice.Done.More info in the commit messages.