-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SSH config format to server list command #56
base: master
Are you sure you want to change the base?
Conversation
Changes from #57 should fix up the tests, just FYI. |
Looks great, thanks for the contribution. I'm wary of removing the Separately, should we consider merging |
538b37a
to
f6707e3
Compare
f6707e3
to
a9cbd3b
Compare
Absolutely, makes sense. Done! The behaviour when you don't pass |
@@ -558,16 +575,36 @@ def list_servers(project, all, verbose): | |||
server.created_at.strftime("%Y-%m-%d %H:%M"), | |||
) | |||
) | |||
if not found_servers and verbose: | |||
|
|||
if not found_servers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing behaviour deliberately only shows "No servers" in the verbose
case, so that if you have a script consuming this output, it doesn't receive what looks like a single server called "No servers".
I think to keep existing behaviour we should have:
- For the old logic
not found_servers and verbose
printing "No servers" - add as a nestedif not found_servers
underif format == "table"
- For the old logic
project and verbose
printing a table with the first (project) column removed - as you've already implemented - For the old logic
project or not verbose
printing a list of server names - as you've implemented underelse:
(but without the short circuit this comment is attached to - no servers should result in empty output) - For the old logic
not project and verbose
printing a table printing a table including the first (project) column - as you've already implemented
Does this sound ok? Removing this short circuit would also result in the ssh-config
format being valid in the case there are no matching servers.
return | ||
|
||
if format == "table": | ||
if project: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was missing from the current implementation, but could you include a comment that this is removing the project column from the output? It's not clear from the implementation.
I regularly find myself running a python script to update my
~/.ssh/config
entries, whenever I create new servers on the Faculty platform. I thought it might be useful to contribute back and add it as a feature to the CLI.Problem statment
Currently, if you create a new server and would like to access it remotely via SSH, you have to run a few separate commands:
1️⃣ Retrieve a list of your servers by name and (only with the verbose flag) their Project:
2️⃣ Retrieve an IP address for my server:
3️⃣ Copy-paste that IP address from my std output and then run my ssh command
This is a little cumbersome for workflows where you create/update your servers often.
Solution
My proposed change is to modify the
faculty server list
command, adding an extra--format
option which supports returning a list of servers in a format which can be piped directly to a user's ssh_config file.This is super useful for workflows where you create/update your servers often, since it allows you to "save" your server ssh details and propagate that info to places like your IDE or command line client.
Example:
I've removed the
--verbose
flag, and moved retrieving the full table of server info to--format table
, since it seemed overcomplicated to have both "verbose" and "format" tags.