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 2021/03/23 18:06:52 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #14963: Change BashOperator skip exit code to 100
jedcunningham opened a new pull request #14963:
URL: https://github.com/apache/airflow/pull/14963
Exit code 127 is used when a command is not found and we don't want to
skip those tasks. Exit code 100 was arbitrarily chosen, however, most
importantly it isn't used as a standard exit code:
https://tldp.org/LDP/abs/html/exitcodes.html
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#issuecomment-805945540
I'll rework this to have a configurable skip exit code, good idea.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#issuecomment-806006298
Concerning which exit code to use as the default, coincidentally I just got exit code 100 while building docs....
This page suggest best to use something <= 125: https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html
Did some googling and found no usages of 99.... I'm sure there are other uncommonly used ones to be found
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#discussion_r602420337
##########
File path: tests/operators/test_bash.py
##########
@@ -117,7 +117,31 @@ def test_default_retries(self):
assert bash_operator.retries == 0
+ def test_command_not_found(self):
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+
def test_skip(self):
- op = BashOperator(task_id='abc', bash_command='set -e; echo "hello world"; exit 127;')
- with pytest.raises(AirflowSkipException):
- op.execute({})
+ with pytest.raises(AirflowSkipException, match="Bash command returned exit code 99\\. Skipping\\."):
+ BashOperator(task_id='abc', bash_command='set -e; echo "hello world"; exit 99;').execute({})
+
+ with pytest.raises(AirflowSkipException, match="Bash command returned exit code 100\\. Skipping\\."):
+ BashOperator(
+ task_id='abc2', bash_command='set -e; echo "hello world"; exit 100;', skip_exit_code=100
+ ).execute({})
+
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(
+ task_id='abc3', bash_command='set -e; echo "hello world"; exit 101;', skip_exit_code=100
+ ).execute({})
+
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(
+ task_id='abc4', bash_command='set -e; echo "hello world"; exit 99;', skip_exit_code=None
+ ).execute({})
Review comment:
```suggestion
@parameterized.expand(
[
(None, 99, AirflowSkipException),
({'skip_exit_code': 100}, 100, AirflowSkipException),
({'skip_exit_code': 100}, 101, AirflowException),
({'skip_exit_code': None}, 99, AirflowException),
]
)
def test_skip(self, extra_kwargs, actual_exit_code, expected_exc):
match = (
f"Bash command returned exit code {actual_exit_code}\\. Skipping\\."
if expected_exc == AirflowSkipException
else "Bash command failed\\. The command returned a non-zero exit code\\."
)
kwargs = dict(task_id='abc', bash_command=f'set -e; echo "hello world"; exit {actual_exit_code};')
if extra_kwargs:
kwargs.update(**extra_kwargs)
with pytest.raises(expected_exc, match=match):
BashOperator(**kwargs).execute({})
```
paramaterized here.... i think it makes it a lot easier to understand what's being tested
i think we could probably skip the `match` part cus it does add quite a bit of noise and i think the main thing we're verifying here is what exception gets raised but i left it in
##########
File path: tests/operators/test_bash.py
##########
@@ -117,7 +117,31 @@ def test_default_retries(self):
assert bash_operator.retries == 0
+ def test_command_not_found(self):
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+
def test_skip(self):
- op = BashOperator(task_id='abc', bash_command='set -e; echo "hello world"; exit 127;')
- with pytest.raises(AirflowSkipException):
- op.execute({})
+ with pytest.raises(AirflowSkipException, match="Bash command returned exit code 99\\. Skipping\\."):
+ BashOperator(task_id='abc', bash_command='set -e; echo "hello world"; exit 99;').execute({})
+
+ with pytest.raises(AirflowSkipException, match="Bash command returned exit code 100\\. Skipping\\."):
+ BashOperator(
+ task_id='abc2', bash_command='set -e; echo "hello world"; exit 100;', skip_exit_code=100
+ ).execute({})
+
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(
+ task_id='abc3', bash_command='set -e; echo "hello world"; exit 101;', skip_exit_code=100
+ ).execute({})
+
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(
+ task_id='abc4', bash_command='set -e; echo "hello world"; exit 99;', skip_exit_code=None
+ ).execute({})
Review comment:
i couldn't make the suggestion across the deleted line so if you accept this you'll need to remove the extraneous `def test_skip(self):`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#issuecomment-805406978
lgtm.... the other option would be to make it configurable i.e. an operator parameter `skip_exit_code=100`
maybe this would be best, actually .... and if `None` then don't skip under any circumstances.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#discussion_r602472722
##########
File path: tests/operators/test_bash.py
##########
@@ -117,7 +117,31 @@ def test_default_retries(self):
assert bash_operator.retries == 0
+ def test_command_not_found(self):
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+
def test_skip(self):
- op = BashOperator(task_id='abc', bash_command='set -e; echo "hello world"; exit 127;')
- with pytest.raises(AirflowSkipException):
- op.execute({})
+ with pytest.raises(AirflowSkipException, match="Bash command returned exit code 99\\. Skipping\\."):
+ BashOperator(task_id='abc', bash_command='set -e; echo "hello world"; exit 99;').execute({})
+
+ with pytest.raises(AirflowSkipException, match="Bash command returned exit code 100\\. Skipping\\."):
+ BashOperator(
+ task_id='abc2', bash_command='set -e; echo "hello world"; exit 100;', skip_exit_code=100
+ ).execute({})
+
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(
+ task_id='abc3', bash_command='set -e; echo "hello world"; exit 101;', skip_exit_code=100
+ ).execute({})
+
+ with pytest.raises(
+ AirflowException, match="Bash command failed\\. The command returned a non-zero exit code\\."
+ ):
+ BashOperator(
+ task_id='abc4', bash_command='set -e; echo "hello world"; exit 99;', skip_exit_code=None
+ ).execute({})
Review comment:
Good call, thanks, and I whacked the `match` part too.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#issuecomment-805407160
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14963: Make skip_exit_code configurable in BashOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#issuecomment-808815919
[The Workflow run](https://github.com/apache/airflow/actions/runs/693764645) is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#discussion_r599816792
##########
File path: airflow/operators/bash.py
##########
@@ -24,11 +24,13 @@
from cached_property import cached_property
from airflow.exceptions import AirflowException, AirflowSkipException
-from airflow.hooks.subprocess import EXIT_CODE_SKIP, SubprocessHook
+from airflow.hooks.subprocess import SubprocessHook
from airflow.models import BaseOperator
from airflow.utils.decorators import apply_defaults
from airflow.utils.operator_helpers import context_to_airflow_vars
+EXIT_CODE_SKIP = 100
Review comment:
I moved this back into the operator as it really is operator specific.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#issuecomment-805406978
lgtm.... the other option would be to make it configurable i.e. an operator parameter `skip_exit_code=100`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14963: Change BashOperator skip exit code to 100
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14963:
URL: https://github.com/apache/airflow/pull/14963#issuecomment-805127873
[The Workflow run](https://github.com/apache/airflow/actions/runs/680526415) is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #14963: Make skip_exit_code configurable in BashOperator
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #14963:
URL: https://github.com/apache/airflow/pull/14963
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org