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

Make skip_exit_code configurable in BashOperator #14963

Merged
merged 2 commits into from
Mar 30, 2021
Merged

Make skip_exit_code configurable in BashOperator #14963

merged 2 commits into from
Mar 30, 2021

Conversation

jedcunningham
Copy link
Member

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

@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

@kaxil kaxil requested a review from dstandish March 24, 2021 00:28
dstandish
dstandish approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@dstandish dstandish left a comment

Choose a reason for hiding this comment

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

No concerns from me, especially since this has not been released yet.

@dstandish
Copy link
Contributor

dstandish commented Mar 24, 2021 •
edited
Loading

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.

@github-actions
Copy link

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.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 24, 2021
@jedcunningham
Copy link
Member Author

I'll rework this to have a configurable skip exit code, good idea.

@jedcunningham jedcunningham marked this pull request as draft March 24, 2021 15:56
@dstandish
Copy link
Contributor

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

@ashb ashb mentioned this pull request Mar 26, 2021
Closed
@jedcunningham jedcunningham force-pushed the bash_op_skipped_exit_code branch from 01344c9 to cc114bb Compare March 26, 2021 15:27
@jedcunningham jedcunningham requested a review from dstandish March 26, 2021 15:28
@jedcunningham jedcunningham marked this pull request as ready for review March 26, 2021 15:29
@ashb ashb added this to the Airflow 2.1 milestone Mar 26, 2021
dstandish
dstandish approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@dstandish dstandish left a comment

Choose a reason for hiding this comment

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

i've made one suggestion for parameterizing test but looks good to me otherwise :)

tests/operators/test_bash.py Outdated Show resolved Hide resolved
@jedcunningham jedcunningham force-pushed the bash_op_skipped_exit_code branch 2 times, most recently from 63dbdce to 2df76e9 Compare March 26, 2021 23:31
@dstandish dstandish changed the title Change BashOperator skip exit code to 100 Make skip_exit_code configurable in BashOperator Mar 27, 2021
@jedcunningham jedcunningham force-pushed the bash_op_skipped_exit_code branch from 2df76e9 to d0313cc Compare March 27, 2021 22:35
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

jedcunningham and others added 2 commits March 29, 2021 14:35
@jedcunningham
Change BashOperator skip exit code to 99
30e114d 
Exit code 127 is used when a command is not found and we don't want to
skip those tasks. Exit code 99 was arbitrarily chosen, however, most
importantly it isn't used as a standard exit code:

https://tldp.org/LDP/abs/html/exitcodes.html

This also allows users to provide their own `skip_exit_code` if they
want to use a different exit code than the default 99.
@jedcunningham @dstandish
Update tests/operators/test_bash.py
76a3d34 
Co-authored-by: Daniel Standish <15932138+dstandish@users.noreply.github.com>
@jedcunningham jedcunningham force-pushed the bash_op_skipped_exit_code branch from d0313cc to 76a3d34 Compare March 29, 2021 20:35
@ashb ashb merged commit 398ac7b into apache:master Mar 30, 2021
@ashb ashb deleted the bash_op_skipped_exit_code branch March 30, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstandish dstandish dstandish approved these changes

@ashb ashb ashb approved these changes

@kaxil kaxil Awaiting requested review from kaxil

@mik-laj mik-laj Awaiting requested review from mik-laj

@vikramkoka vikramkoka Awaiting requested review from vikramkoka

Assignees
No one assigned
Labels
full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
None yet
Milestone
Airflow 2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@jedcunningham @dstandish @ashb