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 previousconfig-ssh options, compare to new options and ask to use new (otherwise old ones are used)
Tests for the new functionality

Fixes#1326

TODO:

Auto-update~/.ssh/coder oncoder create andcoder 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) yesYou 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) yesYou 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) noYou should now be able to ssh into your workspace.For example, try running:$ ssh coder.work

Diff:

❯ coder config-ssh --diffChanges:  * 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 workexit status 1

mafredri mentioned this pull request

May 30, 2022

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

Closed

mafredri requested review fromkylecarbs,johnstcn anda team

May 30, 2022 15:04

Copy link

MemberAuthor

mafredri commentedMay 30, 2022

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

johnstcn reviewed

May 31, 2022

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

mafredri commentedMay 31, 2022•
edited
Loading

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

I've now also implemented options parsing, socoder config-ssh can suggest to re-use previous options, whereascoder 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 commented

May 31, 2022

View reviewed changes

cli/configssh.go Outdated

		#
		# 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".`

Copy link

MemberAuthor

mafredriMay 31, 2022•
edited
Loading

Choose a reason for hiding this comment

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

I really would love to see this feature. (i.e. runningcoder 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.)

Copy link

Member

johnstcnJun 2, 2022

Choose a reason for hiding this comment

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

TODO: rewrite comment

mafredri marked this pull request as ready for review

June 1, 2022 16:59

johnstcn approved these changes

Jun 2, 2022

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

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!

cli/configssh.go Outdated

		#
		# 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".`

Copy link

Member

johnstcnJun 2, 2022

Choose a reason for hiding this comment

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

TODO: rewrite comment

cli/configssh.go OutdatedShow resolvedHide resolved

cli/configssh_test.go OutdatedShow resolvedHide resolved

cli/configssh_test.goShow resolvedHide resolved

cli/configssh.goShow resolvedHide resolved

mafredri self-assigned this

Jun 2, 2022

mafredri force-pushed themafredri/ssh-config-refactor-2 branch fromaf23128 to142683bCompare

June 6, 2022 16:14

mafredri added18 commits

June 8, 2022 10:46

feat: Refactor CLI config-ssh to improve UX

b7f2b3d

- 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 presentTODO:- [ ] Parse previous `config-ssh` options and suggest re-using them- [ ] Auto-update `~/.ssh/coder` on `coder create` and `coder delete`- [ ] Tests for the new functionalityFixes#1326

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 added16 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 commitf2f4a83.

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 themafredri/ssh-config-refactor-2 branch from142683b toe084a73Compare

June 8, 2022 08:06

Make Include regexp stricter to avoid deleting user data

f1be4c6

mafredri force-pushed themafredri/ssh-config-refactor-2 branch frome084a73 tof1be4c6Compare

June 8, 2022 08:07

mafredri added2 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 commitb65259f intomain

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 oncoder create andcoder delete#2153

Closed

kylecarbs pushed a commit that referenced this pull request

Jun 10, 2022

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

55fd3e2

- 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 functionalityFixes#1326

mafredri mentioned this pull request

Jun 14, 2022

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

Closed

mafredri added a commit that referenced this pull request

Jun 15, 2022

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

0ee2b65

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

This was referencedJun 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

Labels

None yet

2 participants

Movatterモバイル変換

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

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

Uh oh!

Conversation

mafredri commentedMay 30, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mafredri commentedMay 30, 2022

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

mafredri commentedMay 31, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mafredriMay 31, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstcnJun 2, 2022

Choose a reason for hiding this comment

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

johnstcnJun 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredri commentedMay 30, 2022•
edited
Loading

mafredri commentedMay 31, 2022•
edited
Loading

mafredriMay 31, 2022•
edited
Loading