You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/10/12 08:52:29 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request, #27008: Exit on exitcode 1 in breeze
ephraimbuddy opened a new pull request, #27008:
URL: https://github.com/apache/airflow/pull/27008
We changed how we run the DB upgrade/downgrade commands but the new way does not error whenever there's migration error.
We have a migration error in 'main' right now but it doesn't show as an error in the CI. Exiting when a command has a returned code of 1 might solve it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #27008: Exit on error in breeze command
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27008:
URL: https://github.com/apache/airflow/pull/27008#discussion_r995225296
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -593,7 +593,7 @@ def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCom
get_console().print(f"[red]Error {command_result.returncode} returned[/]")
if verbose:
get_console().print(command_result.stderr)
- return command_result
+ sys.exit(1)
Review Comment:
@ephraimbuddy
https://github.com/apache/airflow/pull/27048
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #27008: Exit on exitcode 1 in breeze
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on PR #27008:
URL: https://github.com/apache/airflow/pull/27008#issuecomment-1275836311
The error: https://github.com/apache/airflow/actions/runs/3219313776/jobs/5272841525#step:9:672
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #27008: Exit on error in breeze command
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27008:
URL: https://github.com/apache/airflow/pull/27008#discussion_r995110616
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -593,7 +593,7 @@ def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCom
get_console().print(f"[red]Error {command_result.returncode} returned[/]")
if verbose:
get_console().print(command_result.stderr)
- return command_result
+ sys.exit(1)
Review Comment:
I just missed this one, but for posterity: any reason to not pass along the exit code we receive from the command result instead of hardcoding it to `1`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy merged pull request #27008: Exit on error in breeze command
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged PR #27008:
URL: https://github.com/apache/airflow/pull/27008
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #27008: Exit on error in breeze command
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27008:
URL: https://github.com/apache/airflow/pull/27008#discussion_r995178342
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -593,7 +593,7 @@ def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCom
get_console().print(f"[red]Error {command_result.returncode} returned[/]")
if verbose:
get_console().print(command_result.stderr)
- return command_result
+ sys.exit(1)
Review Comment:
Sure, I can put up a PR for it :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #27008: Exit on exitcode 1 in breeze
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27008:
URL: https://github.com/apache/airflow/pull/27008#discussion_r993222760
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -589,6 +589,9 @@ def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCom
)
if command_result.returncode == 0:
return command_result
+ elif command_result.returncode == 1:
+ get_console().print(f"[red]Error {command_result.returncode} returned[/]")
+ sys.exit(1)
Review Comment:
I think this should be a part of the `else` block below; this is bubbled up to `enter_shell` and we should call `sys.exit` there instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #27008: Exit on error in breeze command
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #27008:
URL: https://github.com/apache/airflow/pull/27008#discussion_r995157663
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -593,7 +593,7 @@ def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCom
get_console().print(f"[red]Error {command_result.returncode} returned[/]")
if verbose:
get_console().print(command_result.stderr)
- return command_result
+ sys.exit(1)
Review Comment:
Ah, that was a mistake! Can you PR the fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #27008: Exit on exitcode 1 in breeze
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #27008:
URL: https://github.com/apache/airflow/pull/27008#discussion_r993227253
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -589,6 +589,9 @@ def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCom
)
if command_result.returncode == 0:
return command_result
+ elif command_result.returncode == 1:
+ get_console().print(f"[red]Error {command_result.returncode} returned[/]")
+ sys.exit(1)
Review Comment:
I'm not sure of other uses of this code, particularly why we returned `command_result` that was why I had to have a separate `elif` cc: @potiuk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #27008: Exit on error in breeze command
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27008:
URL: https://github.com/apache/airflow/pull/27008#discussion_r1002604817
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -589,6 +589,9 @@ def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCom
)
if command_result.returncode == 0:
return command_result
+ elif command_result.returncode == 1:
+ get_console().print(f"[red]Error {command_result.returncode} returned[/]")
+ sys.exit(1)
Review Comment:
I think this was a bit misplaced (even if it fixed the problem of course :) ).
I addressed it in #27191 - the return was fine, it was simply not used in the @click.command decorated CLI method (we really want to sys.exit(1) in the CLI methods, not in the underlying functions that are called (it allows to make tests and reuse the underlying functions in various commands as needed.
Speaking of which - I actually merged "enter_shell" and "run_shell" commands in @27191 as the `run_shell` has been only used in `enter_shell` and we could easilhy combine them. But then "enter_shell" command was used in a few places ("shell" command and "start-airflow" command), so it makes more sense to return the errorcode from "enter_shell" and only sys.exit() in the "decorated" CLI methods that uses the "enter_shell".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org