From 9c68a15e3115f32913ab5d7f3adfab80b781b79a Mon Sep 17 00:00:00 2001 From: XD-DENG Date: Sun, 15 Nov 2020 16:48:00 +0100 Subject: [PATCH 1/2] More proper exit status for failed CLI requests Some CLI commands simply `print` messages when the requests fail. The issue is the exit code for these commands are `0` while it should be non-zero. More proper exist status ensures people can better make use of the CLI. --- airflow/cli/commands/connection_command.py | 20 +++++++------------- airflow/cli/commands/variable_command.py | 4 ++-- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/airflow/cli/commands/connection_command.py b/airflow/cli/commands/connection_command.py index 7ca9f1aee4af44..5fab8d9bd70d2c 100644 --- a/airflow/cli/commands/connection_command.py +++ b/airflow/cli/commands/connection_command.py @@ -241,9 +241,8 @@ def connections_add(args): ) print(msg) else: - msg = '\n\tA connection with `conn_id`={conn_id} already exists\n' - msg = msg.format(conn_id=new_conn.conn_id) - print(msg) + msg = f'\n\tA connection with `conn_id`={new_conn.conn_id} already exists\n' + raise SystemExit(msg) @cli_utils.action_logging @@ -253,18 +252,13 @@ def connections_delete(args): try: to_delete = session.query(Connection).filter(Connection.conn_id == args.conn_id).one() except exc.NoResultFound: - msg = '\n\tDid not find a connection with `conn_id`={conn_id}\n' - msg = msg.format(conn_id=args.conn_id) - print(msg) - return + msg = f'\n\tDid not find a connection with `conn_id`={args.conn_id}\n' + raise SystemExit(msg) except exc.MultipleResultsFound: - msg = '\n\tFound more than one connection with ' + '`conn_id`={conn_id}\n' - msg = msg.format(conn_id=args.conn_id) - print(msg) - return + msg = f'\n\tFound more than one connection with `conn_id`={args.conn_id}\n' + raise SystemExit(msg) else: deleted_conn_id = to_delete.conn_id session.delete(to_delete) - msg = '\n\tSuccessfully deleted `conn_id`={conn_id}\n' - msg = msg.format(conn_id=deleted_conn_id) + msg = f'\n\tSuccessfully deleted `conn_id`={deleted_conn_id}\n' print(msg) diff --git a/airflow/cli/commands/variable_command.py b/airflow/cli/commands/variable_command.py index e76dc800eac354..8478bf83e01920 100644 --- a/airflow/cli/commands/variable_command.py +++ b/airflow/cli/commands/variable_command.py @@ -65,7 +65,7 @@ def variables_import(args): if os.path.exists(args.file): _import_helper(args.file) else: - print("Missing variables file.") + raise SystemExit("Missing variables file.") def variables_export(args): @@ -81,7 +81,7 @@ def _import_helper(filepath): try: var_json = json.loads(data) except JSONDecodeError: - print("Invalid variables file.") + raise SystemExit("Invalid variables file.") else: suc_count = fail_count = 0 for k, v in var_json.items(): From f5b1468bab1d354edcd658daad8186d8eed1c473 Mon Sep 17 00:00:00 2001 From: XD-DENG Date: Sun, 15 Nov 2020 18:02:52 +0100 Subject: [PATCH 2/2] Fix-up. Update tests. --- tests/cli/commands/test_connection_command.py | 19 ++++++------------- tests/cli/commands/test_variable_command.py | 3 ++- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/tests/cli/commands/test_connection_command.py b/tests/cli/commands/test_connection_command.py index 779f4ace7f83f8..cc91433c419372 100644 --- a/tests/cli/commands/test_connection_command.py +++ b/tests/cli/commands/test_connection_command.py @@ -626,18 +626,15 @@ def test_cli_connection_add(self, cmd, expected_output, expected_conn): self.assertEqual(expected_conn, {attr: getattr(current_conn, attr) for attr in comparable_attrs}) def test_cli_connections_add_duplicate(self): - # Attempt to add duplicate + conn_id = "to_be_duplicated" connection_command.connections_add( - self.parser.parse_args(["connections", "add", "new1", "--conn-uri=%s" % TEST_URL]) + self.parser.parse_args(["connections", "add", conn_id, "--conn-uri=%s" % TEST_URL]) ) - with redirect_stdout(io.StringIO()) as stdout: + # Check for addition attempt + with self.assertRaisesRegex(SystemExit, rf"A connection with `conn_id`={conn_id} already exists"): connection_command.connections_add( - self.parser.parse_args(["connections", "add", "new1", "--conn-uri=%s" % TEST_URL]) + self.parser.parse_args(["connections", "add", conn_id, "--conn-uri=%s" % TEST_URL]) ) - stdout = stdout.getvalue() - - # Check stdout for addition attempt - self.assertIn("\tA connection with `conn_id`=new1 already exists", stdout) def test_cli_connections_add_delete_with_missing_parameters(self): # Attempt to add without providing conn_uri @@ -687,9 +684,5 @@ def test_cli_delete_connections(self, session=None): def test_cli_delete_invalid_connection(self): # Attempt to delete a non-existing connection - with redirect_stdout(io.StringIO()) as stdout: + with self.assertRaisesRegex(SystemExit, r"Did not find a connection with `conn_id`=fake"): connection_command.connections_delete(self.parser.parse_args(["connections", "delete", "fake"])) - stdout = stdout.getvalue() - - # Check deletion attempt stdout - self.assertIn("\tDid not find a connection with `conn_id`=fake", stdout) diff --git a/tests/cli/commands/test_variable_command.py b/tests/cli/commands/test_variable_command.py index 04c253921eba9a..c9e9318c64d49d 100644 --- a/tests/cli/commands/test_variable_command.py +++ b/tests/cli/commands/test_variable_command.py @@ -119,7 +119,8 @@ def test_variables_delete(self): def test_variables_import(self): """Test variables_import command""" - variable_command.variables_import(self.parser.parse_args(['variables', 'import', os.devnull])) + with self.assertRaisesRegex(SystemExit, r"Invalid variables file"): + variable_command.variables_import(self.parser.parse_args(['variables', 'import', os.devnull])) def test_variables_export(self): """Test variables_export command"""