Add support for connections to multiple deployments #292
Conversation
code-asher
force-pushed
the
asher/multiple-deployments
branch
from
56e4bb6 to
55ccb56
Compare
This lets us create multiple ssh config blocks that each point to different Coder binaries and config directories for those binaries, instead of always overwriting a single configuration block, binary, and config directory with the most recently configured deployment is like we currently do. This scoping is accomplished by changing the ssh config block's comment markers to include the label and by adding the label to the host name: coder-vscode.label--*. The Coder binary splits by -- and ignores the first part, so we are free to use it however we need. This scoping is used support config blocks per deployment, where the label is derived from the deployment URL. That means we can have one config block for dev.coder.com, another for test.coder.com, and so on, each with their own binaries and tokens, coexisting in harmony.
Every time you log in or out we would add or remove the cli and its config in a single directory, meaning logging into another deployment would wipe out the binary and config for the previous one. Now, similar to the ssh config blocks, the cli and its config files are scoped by their label, which is derived from the deployment URL, so they can coexist.
Instead of pulling the url and token from the currently logged-in deployment, we use the url and token as they exist on the disk organized by the label (~/.config/.../coder.coder-remote/dev.coder.com for example). The label we extract from the host (coder-vscode.dev.coder.com-- for example). Because the new format is a little different, the parsing has also changed. It has been moved to utils for easier testing. This way, we pull the right url and token for the host even if the user is logged into a different deployment currently. If there is no label because it is the older format, we read the old deployment-unaware path instead (~/.config/.../coder.coder-remote). Since this now uses the same url and token the cli itself will use, this has the extra advantage of preventing a potential desync between what exists on disk and what exists in VS Code's memory.
code-asher
force-pushed
the
asher/multiple-deployments
branch
from
55ccb56 to
c8c0e6d
Compare
There was a problem hiding this comment.
Looks good, and everything seems to make sense (I followed your recommendation and went through things commit-by-commit)
I'm gonna want to review more of how this codebase is set up, though. All the individual function calls seemed to make sense, but I feel like there's probably some finer nuances that I'm missing (especially with the VSCode APIs)
|
|
||
| it("escapes url host", async () => { | ||
| expect(toSafeHost("https://foobar:8080")).toBe("foobar") | ||
| expect(toSafeHost("https://ほげ")).toBe("xn--18j4d") |
There was a problem hiding this comment.
TIL that "hoge" is the Japanese equivalent of "foo"
| await storage.setSessionToken(token) | ||
| } | ||
|
|
||
| // Store on disk to be used by the cli. | ||
| await storage.configureCli(toSafeHost(url), url, token) |
There was a problem hiding this comment.
I've noticed that all the calls to storage.configureCli have also required a call to toSafeHost. Do you think that configureCli could call toSafeHost internally?
There was a problem hiding this comment.
That is a good question. My thinking is that this storage interface is taking a directory name (or label as I am calling it), so from that perspective it does not make too much sense for it to be aware of URLs and how to encode them.
So, for similar reasons that, say, fs.writeFile(fileName) would not automatically encode the file name (er, at least, pretty sure it does not).
Similar to how SSHConfig takes a label rather than a URL as well.
There was a problem hiding this comment.
Probably it will always be a URL, but in the future there could be tests doing things like storage.configureCli("temp-dir") or some such. But you could argue that the tests should just use URLs too, and the storage abstraction should change to take URLs rather than generic labels (and SSHConfig as well).
But, I am partial to the generic label abstraction personally, maybe because it feels less coupled/simpler to me.
I would have tests already except we need to switch away from vitest back to the VS Code testing framework. 😭
I feel that, it is really tough to do code review in an unfamiliar repository. I appreciate your reviews all the same though! |
This is broadly three parts, as described by the commits, but the main gist is that the SSH blocks are now scoped (with the deployment URL in the host name), and the individual config files referenced in those config blocks are also scoped (with the deployment URL in the directory), and the connection process has been decoupled from the credentials in VS Code's memory and coupled to the credentials in the associated config directory instead. More context in each commit message; it might even make sense to review by commit.
Existing hosts should not be affected, only new connections will be scoped by deployment. Existing hosts (ones without the deployment in the host) will continue using the old config block and directories. But it will not be possible to add new hosts using the old style; new connections will always be scoped.
I should also clarify that this specifically is for supporting the "Recents" menu that may contain connections from multiple deployments and for reconnections (say you connect and then log into a different deployment while the other connection is still up). It does not let you log into multiple deployments and see all the workspaces from all of them at once.
In other words it closes #173 but not #102.