[UNW-177] CLI should create SSH config if it does not exist#9
[UNW-177] CLI should create SSH config if it does not exist#9caldempsey merged 5 commits intomasterfrom
Conversation
| os.Exit(1) | ||
| // Add an Include directive to the user's ssh config to unweave_global SSH configs - used for vscode-remote. | ||
| // Create one it if it does not exist. | ||
| if _, err := os.Stat(sshConfigPath); os.IsNotExist(err) { |
There was a problem hiding this comment.
This should also check for directories/sub-directories and not just the file.
There was a problem hiding this comment.
Can you elaborate? In the man page the only place ssh_config reads a user level ssh_config from is /home/.ssh/ssh_config: https://linux.die.net/man/5/ssh_config. Do you mean ensure that the .ssh directory exists, as that might be an issue?
There was a problem hiding this comment.
Yeah exactly. In this case you're passing the path as a variable which could be arbitrarily deeply nester. So we need a os.MkdirAll
There was a problem hiding this comment.
Great thanks for putting the time in to clarify!
There was a problem hiding this comment.
I've added a MkDir check using the same permissions as the .ssh folder
cmd/ssh.go
Outdated
| homeDir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| ui.Errorf("Failed to get user home directory: %v", err) | ||
| os.Exit(1) | ||
| } | ||
| if err := ssh.AddHost("uw:"+e.ID, e.Connection.Host, e.Connection.User, e.Connection.Port, filepath.Join(homeDir, ".ssh", "config")); err != nil { |
There was a problem hiding this comment.
I think we can defer all ssh related config and settings to the ssh package. This way all the settings are in one place. i.e. you don't need to dependency inject this.
There was a problem hiding this comment.
In this case we would have to use a global variable if we still want to be able to test the code or remove the tests. Which do you prefer?
There was a problem hiding this comment.
Using global variable for now is fine since that's just how the CLI is structured atm. Later on we can try and parse all global variables into a config before the command handlers initialize. That would be way cleaner
# Conflicts: # cmd/ssh.go # ssh/config.go
| homeDir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| ui.Errorf("Failed to get user home directory: %v", err) | ||
| os.Exit(1) | ||
| } | ||
| return filepath.Join(homeDir, ".ssh") |
There was a problem hiding this comment.
AFAIK if we can't get the home dir things might go wrong, so don't want to ignore this check for the sake of a one-liner
This PR addresses a bug where the
AddHostfunction does not ensure the presence of the.ssh/configfile before adding theIncludedirective. Preventing the proper configuration of SSH hosts may prevent VSCode remote from connecting to a new session (or other IDEs which depend on it).Fixed by adding a check for the existence of the
.ssh/configfile and creating it if it doesn't exist before adding theIncludedirective. Adds a test to sanity check, and parameterized the input directory of thessh/.configfile to make the behavior testable.