feat: Refactor CLI config-ssh to improve UX #1900

mafredri · 2022-05-30T15:02:56Z

This PR improves the UX of coder config-ssh with multiple QoL improvements. Replaces #1848.

Magic block is replaced by Include statement
Writes are only done on changes
Inform user of changes via prompt
Allow displaying changes via --diff
Remove magic block if present
Safer config writing via tmp-file + rename
Parse previous config-ssh options, compare to new options and ask to use new (otherwise old ones are used)
Tests for the new functionality

TODO:

Auto-update ~/.ssh/coder on coder create and coder delete
- This will be be left to a follow-up PR, or scrapped in unwanted

Examples:

New config:

❯ coder config-ssh
> The following changes will be made to your SSH configuration:

    * Remove old config block (START-CODER/END-CODER) from /home/maf/.ssh/config
    * Add "Include coder" to /home/maf/.ssh/config
    * Write auto-generated coder config file to /home/maf/.ssh/coder

  To see changes, run diff:

    $ coder config-ssh --diff

  Continue? (yes/no) yes

You should now be able to ssh into your workspace.
For example, try running:

	$ ssh coder.work

Options changed:

❯ coder config-ssh -o ForwardAgent=yes
> New options differ from previous options:

  New options:
    * ssh-option: ForwardAgent=yes

  Previous options: none

  Use new options? (yes/no) yes

> The following changes will be made to your SSH configuration:

    * Update auto-generated coder config file in /home/maf/.ssh/coder

  To see changes, run diff:

    $ coder config-ssh --ssh-option "ForwardAgent=yes" --diff

  Continue? (yes/no) yes

You should now be able to ssh into your workspace.
For example, try running:

	$ ssh coder.work

Options would change => select no (no changes):

❯ coder config-ssh -o ForwardAgent=yes -o IdentityAgent=none
> New options differ from previous options:

  New options:
    * ssh-option: ForwardAgent=yes
    * ssh-option: IdentityAgent=none

  Previous options:
    * ssh-option: ForwardAgent=yes

  Use new options? (yes/no) no

You should now be able to ssh into your workspace.
For example, try running:

	$ ssh coder.work

Diff:

❯ coder config-ssh --diff
Changes:

  * Remove old config block (START-CODER/END-CODER) from /home/maf/.ssh/config
  * Add "Include coder" to /home/maf/.ssh/config
  * Write auto-generated coder config file to /home/maf/.ssh/coder

--- /home/maf/.ssh/config
+++ /home/maf/.ssh/config.new
@@ -1,23 +1,6 @@
+Include coder
+
 Host github.com
 	HostName github.com
 	User git

-# ------------START-CODER-----------
-# This was generated by "coder config-ssh".
-#
-# To remove this blob, run:
-#
-#    coder config-ssh --remove
-#
-# You should not hand-edit this section, unless you are deleting it.
-
-Host coder.work
-	ForwardAgent=yes
-	HostName coder.work
-	ConnectTimeout=0
-	StrictHostKeyChecking=no
-	UserKnownHostsFile=/dev/null
-	LogLevel ERROR
-	ProxyCommand "coder" --global-config "/Users/maf/Library/Application Support/coderv2" ssh --stdio work
-
-# ------------END-CODER------------

--- /home/maf/.ssh/coder
+++ /home/maf/.ssh/coder.new
@@ -0,0 +1,14 @@
+# This file is managed by coder. DO NOT EDIT.
+#
+# You should not hand-edit this file, all changes will be lost upon workspace
+# creation, deletion or when running "coder config-ssh".
+#
+# Last config-ssh options:
+#
+Host coder.work
+	HostName coder.work
+	ConnectTimeout=0
+	StrictHostKeyChecking=no
+	UserKnownHostsFile=/dev/null
+	LogLevel ERROR
+	ProxyCommand "coder" --global-config "/home/maf/.config/coderv2" ssh --stdio work
exit status 1

mafredri · 2022-05-30T19:45:14Z

Just noticed we need to place the Include directive before the first Host entry, otherwise it will be interpreted as part of one and included conditionally.

johnstcn

I like the approach so far! The --diff option is really cool!

Some observations:

We should keep the configs under ~/.ssh because that's where they always go.
We should check that we "own" ~/.ssh/coder (maybe by checking for a magic string) before writing it

mafredri · 2022-05-31T12:44:37Z

@johnstcn great observations, I've made the necessary changes.

I've now also implemented options parsing, so coder config-ssh can suggest to re-use previous options, whereas coder config-ssh -o MyOption=yes assumes the user knows what they're doing. As a result, the full diff command is output in the prompt. The two different scenarios look like this:

mafredri · 2022-05-31T12:54:14Z

cli/configssh.go

 #
-# To remove this blob, run:
+# You should not hand-edit this file, all changes will be lost upon workspace
+# creation, deletion or when running "coder config-ssh".`


I really would love to see this feature. (i.e. running coder create will automatically update the config). But I suggest we rewrite this comment to not include it, open up a ticket for this functionality, and leave it for a follow-up PR. (PS. This is one reason we write the used options to the config file.)

TODO: rewrite comment

johnstcn

I haven't tested this locally, but looks good to me. I appreciate the thorough tests!

I do think there's still scope for refactoring; a lot of the code related to reading and writing SSH configs seems like it could be ripped out to its own module. This should probably done in a separate PR however!

johnstcn · 2022-06-02T09:02:49Z

cli/configssh.go

 #
-# To remove this blob, run:
+# You should not hand-edit this file, all changes will be lost upon workspace
+# creation, deletion or when running "coder config-ssh".`


TODO: rewrite comment

cli/configssh.go

cli/configssh_test.go

cli/configssh.go

- Magic block is replaced by Include statement - Writes are only done on changes - Inform user of changes via prompt - Allow displaying changes via `--diff` - Remove magic block if present TODO: - [ ] Parse previous `config-ssh` options and suggest re-using them - [ ] Auto-update `~/.ssh/coder` on `coder create` and `coder delete` - [ ] Tests for the new functionality Fixes #1326

This reverts commit f2f4a83.

- Magic block is replaced by Include statement - Writes are only done on changes - Inform user of changes via prompt - Allow displaying changes via `--diff` - Remove magic block if present - Safer config writing via tmp-file + rename - Parse previous `config-ssh` options, compare to new options and ask to use new (otherwise old ones are used) - Tests the new functionality Fixes #1326

This commit partially reverts #1900 and removes the separate `~/.ssh/coder` config file whilst keeping most other features. This will allow us to remain more compatible with different IDEs. Fixes #2317

mafredri mentioned this pull request May 30, 2022

feat: Refactor CLI config-ssh to Include configurations #1848

Closed

mafredri requested review from kylecarbs, johnstcn and a team May 30, 2022 15:04

johnstcn reviewed May 31, 2022

View reviewed changes

mafredri commented May 31, 2022

View reviewed changes

mafredri marked this pull request as ready for review June 1, 2022 16:59

johnstcn approved these changes Jun 2, 2022

View reviewed changes

mafredri self-assigned this Jun 2, 2022

mafredri force-pushed the mafredri/ssh-config-refactor-2 branch from af23128 to 142683b Compare June 6, 2022 16:14

mafredri added 18 commits June 8, 2022 10:46

chore: Switch to github.com/pkg/diff for diffing

4f03415

fix: Output absolute paths for diffs

e196920

chore: Remove todone

cdcf6c3

chore: Rename actions => changes for consistency

263d823

fix: Remove workspaces check, removing is a legit use-case

18797e7

fix: Remove unused else

9680792

fix: Handle no workspaces case

7a4b8d6

chore: Update diffBytes comment

e552e84

fix: Cleanup temp file on failure

5250a8a

fix: Wrap error

0749e58

fix: Enable color when terminal is a tty

b3f448a

fix: Improve output, detect if stdout is tty

c5f1983

fix: Add Include to the top of config, verify semantically

8d0f72c

fix: Guard against overwriting unknown ~/.ssh/coder file

d7838b5

feat: Parse and prompt to re-use previous configuration

f94bc3f

chore: Increase indentation

00a8c12

chore: Remove unused equality func

e50c4c3

mafredri added 16 commits June 8, 2022 10:46

fix: Only rewrite coder file path if not ~/

1e33886

fix: Allow removing invalid Include coder entry

209c8ff

Revert "chore: Remove unused equality func"

5ed5e0e

This reverts commit f2f4a83.

fix: Allow replacing default ssh config file in tests

000c9b4

feat: Update options prompt, default to using new options

9d64680

fix: Improve regexes

a7cb149

feat: Add tests

a77c791

fix: Do not prompt for previous opts on first run

5bcd038

fix: Fix broken TestConfigSSH test

1632eb4

fix: Revert multiwriter change usued while testing

99693ce

fix: Change diff suggestions for more natural sentence

fb24742

fix: Typo

5c785dd

fix: Fix linting (gocyclo, revive), improve responsiveness

c60762f

chore: Update header comment

0422e71

chore: Extract function

6e1e1a0

chore: Remove test log statements

fda935f

mafredri force-pushed the mafredri/ssh-config-refactor-2 branch from 142683b to e084a73 Compare June 8, 2022 08:06

Make Include regexp stricter to avoid deleting user data

f1be4c6

mafredri force-pushed the mafredri/ssh-config-refactor-2 branch from e084a73 to f1be4c6 Compare June 8, 2022 08:07

mafredri added 2 commits June 8, 2022 11:20

chore: Update the "Changes" text for diff to match non-diff mode

bf4596c

chore: Use consistent naming of functions

093e2ce

johnstcn approved these changes Jun 8, 2022

View reviewed changes

mafredri merged commit b65259f into main Jun 8, 2022

mafredri deleted the mafredri/ssh-config-refactor-2 branch June 8, 2022 08:45

mafredri mentioned this pull request Jun 8, 2022

Automatically update coder SSH config on coder create and coder delete #2153

Closed

mafredri mentioned this pull request Jun 14, 2022

Non-recent versions of JetBrains editors don't support Include statements in SSH config #2317

Closed

This was referenced Jun 15, 2022

fix: Revert to old SSH config section management in config-ssh #2341

Merged

CLI: config-ssh: coder config-ssh --remove is not supported, but is mentioned in .ssh/config file #1704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor CLI config-ssh to improve UX #1900

feat: Refactor CLI config-ssh to improve UX #1900

mafredri commented May 30, 2022 •

edited

Loading

mafredri commented May 30, 2022

johnstcn left a comment

mafredri commented May 31, 2022 •

edited

Loading

mafredri May 31, 2022 •

edited

Loading

johnstcn Jun 2, 2022

johnstcn left a comment

johnstcn Jun 2, 2022

feat: Refactor CLI config-ssh to improve UX #1900

feat: Refactor CLI config-ssh to improve UX #1900

Conversation

mafredri commented May 30, 2022 • edited Loading

mafredri commented May 30, 2022

johnstcn left a comment

Choose a reason for hiding this comment

mafredri commented May 31, 2022 • edited Loading

mafredri May 31, 2022 • edited Loading

Choose a reason for hiding this comment

johnstcn Jun 2, 2022

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

johnstcn Jun 2, 2022

Choose a reason for hiding this comment

mafredri commented May 30, 2022 •

edited

Loading

mafredri commented May 31, 2022 •

edited

Loading

mafredri May 31, 2022 •

edited

Loading