Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 46 additions & 9 deletions faculty_cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -509,12 +509,29 @@ def server():
"-v",
"--verbose",
is_flag=True,
help="Print extra information about servers.",
help="",
)
def list_servers(project, all, verbose):
@click.option(
"-f",
"--format",
type=click.Choice(["list", "table", "ssh-config"], case_sensitive=False),
help="Output format.",
)
def list_servers(project, all, verbose, format):
"""List your Faculty servers.

If you do not specify a project, all servers will be listed."""
if verbose:
if format is None:
# Retain legacy behaviour for verbose flag.
format = "table"
else:
raise click.exceptions.BadOptionUsage(
"format",
"You can't specify the `--verbose` flag and also pass the "
"`--format` option",
)

status_filter = None if all else ServerStatus.RUNNING
if not project:
projects = {project.id: project.name for project in _list_projects()}
Expand Down Expand Up @@ -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:
Copy link
Member

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 nested if not found_servers under if 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 under else: (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.

click.echo("No servers.")
elif project and verbose:
servers = [server[1:] for server in found_servers]
click.echo(tabulate(servers, headers[1:], tablefmt="plain"))
elif project or not verbose:
return

if format == "table":
if project:
Copy link
Member

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.

found_servers = [server[1:] for server in found_servers]
headers = headers[1:]
click.echo(tabulate(found_servers, headers, tablefmt="plain"))

elif format == "ssh-config":
config_entries = []
for server in found_servers:
server_name = server[1]
project_name = project if project else server[0]
ssh_details = _get_ssh_details(project_name, server_name)
host = f"{server_name}-{project_name.lower().replace(' ', '_')}"
config_entries.append(
f"""Host {host}
HostName {ssh_details.hostname}
User {ssh_details.username}
Port {ssh_details.port}"""
)

click.echo("\n".join(config_entries))

else:
for server in found_servers:
click.echo(server[1])
elif not project and verbose:
click.echo(tabulate(found_servers, headers, tablefmt="plain"))


@server.command(name="open")
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
SharedServerResources,
Server,
Service,
SSHDetails,
)
from faculty.config import Profile

Expand Down Expand Up @@ -71,3 +72,8 @@
status=ServerStatus.RUNNING,
services=[SERVICE],
)


SSH_DETAILS = SSHDetails(
hostname="test-server", port=1234, username="faculty", key="not-a-real-key"
)
50 changes: 49 additions & 1 deletion test/test_servers.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
DEDICATED_SERVER,
DEDICATED_RESOURCE,
SHARED_RESOURCE,
SSH_DETAILS,
)


Expand Down Expand Up @@ -54,7 +55,7 @@ def test_list_all_servers_no_servers(
mocker.patch("faculty_cli.cli._list_user_servers", return_value=[])
result = runner.invoke(cli, ["server", "list"])
assert result.exit_code == 0
assert result.output == ""
assert result.output == "No servers.\n"


def test_list_all_servers(
Expand Down Expand Up @@ -180,6 +181,53 @@ def test_list_servers_verbose(
)


def test_list_all_servers_ssh_config(
mocker,
mock_update_check,
mock_check_credentials,
mock_profile,
mock_user_id,
):
runner = CliRunner()
mocker.patch("faculty_cli.cli._list_projects", return_value=[PROJECT])
mocker.patch(
"faculty_cli.cli._list_user_servers", return_value=[DEDICATED_SERVER]
)
mocker.patch("faculty_cli.cli._get_ssh_details", return_value=SSH_DETAILS)
result = runner.invoke(
cli,
["server", "list", "--format", "ssh-config"],
)
assert result.exit_code == 0
assert result.output == (
"Host test-server-test-project\n HostName test-server\n User "
"faculty\n Port 1234\n"
)


def test_list_all_servers_verbose_error(
mocker,
mock_update_check,
mock_check_credentials,
mock_profile,
mock_user_id,
):
runner = CliRunner()
result = runner.invoke(
cli,
["server", "list", "--verbose", "--format", "ssh-config"],
)

assert result.exit_code == 2
assert result.output == (
"Usage: cli server list [OPTIONS] PROJECT\n"
"Try 'cli server list --help' for help.\n"
"\n"
"Error: You can't specify the `--verbose` flag "
"and also pass the `--format` option\n"
)


def test_server_spec_dedicated():
machine_type, cpus, memory_gb = _server_spec(DEDICATED_SERVER)
assert machine_type == DEDICATED_RESOURCE.node_type
Expand Down