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

Proper exit status for failed CLI requests #12375

Merged
merged 2 commits into from
Nov 15, 2020
Merged
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
20 changes: 7 additions & 13 deletions airflow/cli/commands/connection_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
4 changes: 2 additions & 2 deletions airflow/cli/commands/variable_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise SystemExit("Missing variables file.")
raise SystemExit("\n\tMissing variables file.")

If we want to be consistent... but to be honest I'm not sure if we need this "prefix".

Copy link
Member Author

@XD-DENG XD-DENG Nov 15, 2020

Choose a reason for hiding this comment

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

Style consistency will need more effort. For example:

  • with or without prefix "\n\t" are varying among different sub-command-groups (the same raises in user_command.py are withOUT this "prefix").
  • some CLI failures are directly throwing "original" Python exceptions.

In this PR I would dedicate on the exit status, if you don't mind.



def variables_export(args):
Expand All @@ -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():
Expand Down
19 changes: 6 additions & 13 deletions tests/cli/commands/test_connection_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
3 changes: 2 additions & 1 deletion tests/cli/commands/test_variable_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"""
Expand Down