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.

Sign up for GitHub

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

Jump to bottom

fixed local run 'no tty message' not appearing with --build #916

Merged
merged 2 commits into from Dec 3, 2016
Merged

fixed local run 'no tty message' not appearing with --build #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0af1130
fixed local run notyy message not appearing with --build
mjksmith Dec 2, 2016
6d601d6
mocked stdin instead of stdin.isatty
mjksmith Dec 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 13 additions & 6 deletions paasta_tools/cli/cmds/local_run.py
View file
Open in desktop
Expand Up @@ -632,18 +632,25 @@ def configure_and_run_docker_container(
Function prints the output of run command in stdout.
"""

soa_dir = args.yelpsoa_config_root
if instance is None and args.healthcheck:
paasta_print(
"With --healthcheck, --instance must be provided!",
file=sys.stderr,
)
sys.exit(1)
if instance is None and not sys.stdin.isatty():
Copy link
Contributor

Choose a reason for hiding this comment

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

How about if

interactive is Tru and not tty

I think that would achieve the same effect and be clearer, but you would need to place it further down after we've figured out if we are interactive or not.

Copy link
Contributor Author

@mjksmith mjksmith Dec 2, 2016 •
edited

Choose a reason for hiding this comment

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

@solarkennedy it would be nice to have a different error message between no --instance (you need to specify --instance and --cluster when you don't have a tty) vs interactive being set with -I (don't use -I without a tty). We can't have that if we just check interactive is True and not sys.stdin.isatty().

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

paasta_print(
"--instance and --cluster must be specified when using paasta local-run without a tty!",
file=sys.stderr,
)
sys.exit(1)

soa_dir = args.yelpsoa_config_root
volumes = list()

load_deployments = docker_hash is None or pull_image

interactive = args.interactive

try:
if instance is None and args.healthcheck:
paasta_print("With --healthcheck, --instance must be provided!", file=sys.stderr)
sys.exit(1)
if instance is None:
instance_type = 'adhoc'
instance = 'interactive'
Expand Down
3 changes: 3 additions & 0 deletions tests/cli/test_cmds_local_run.py
View file
Open in desktop
Expand Up @@ -479,16 +479,19 @@ def test_configure_and_run_pulls_image_when_asked(

def test_configure_and_run_docker_container_defaults_to_interactive_instance():
with contextlib.nested(
mock.patch('paasta_tools.cli.cmds.local_run.sys.stdin', autospec=True),
mock.patch('paasta_tools.cli.cmds.local_run.validate_service_instance', autospec=True),
mock.patch('paasta_tools.cli.cmds.local_run.socket.getfqdn', autospec=True),
mock.patch('paasta_tools.cli.cmds.local_run.run_docker_container', autospec=True),
mock.patch('paasta_tools.cli.cmds.local_run.get_default_interactive_config', autospec=True),
) as (
mock_stdin,
mock_validate_service_instance,
mock_socket_get_fqdn,
mock_run_docker_container,
mock_get_default_interactive_config,
):
mock_stdin.isatty.return_value = True
mock_validate_service_instance.side_effect = NoConfigurationForServiceError
mock_docker_client = mock.MagicMock(spec_set=docker.Client)
mock_socket_get_fqdn.return_value = 'fake_hostname'
Expand Down