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

BashOperator - resolve bash by absolute path #25331

Merged
merged 1 commit into from Jul 28, 2022

Conversation

MatrixManAtYrService
Copy link
Contributor

Fixes #25330

Necessary because maybe the user sets env but doesn't set PATH. Now PATH is empty and the subprocess can't find bash. I'm a little confused why this is not a more common failure mode, does subprocess have a list of default locations to look or something?

Existing tests of BashOperator should be sufficient.

@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Jul 27, 2022
@uranusjr
Copy link
Member

uranusjr commented Jul 27, 2022

I'm a little confused why this is not a more common failure mode, does subprocess have a list of default locations to look or something?

Yes, kind of. When you supply a “bare“ command (instead of a path), subprocess relies on the OS to look up the actual executable. This is the same as looking up in PATH most of the time, but also depends on a bunch of other run-time configurations set in the OS. So the general suggestion is to always pass in a pre-resolved path instead of a bare command, since it is the behaviour most people expect in most situations, unless you are actively trying to replicate the OS behaviour down to implementation details.

This is also mentioned in the subprocess documentation: https://docs.python.org/3.10/library/subprocess.html#subprocess.Popen

@potiuk
Copy link
Member

potiuk commented Jul 27, 2022

This is also mentioned in the subprocess documentation: https://docs.python.org/3.10/library/subprocess.html#subprocess.Popen

TIL. Thanks @MatrixManAtYrService and @uranusjr for nice discussion on it and links :)

@potiuk potiuk merged commit c3adf3e into apache:main Jul 28, 2022
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Jul 28, 2022
@jedcunningham jedcunningham added this to the Airflow 2.3.4 milestone Jul 28, 2022
ephraimbuddy pushed a commit that referenced this pull request Aug 15, 2022
Co-authored-by: Matt Rixman <MatrixManAtYrService@users.noreply.github.com>
(cherry picked from commit c3adf3e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User defined env clobbers PATH, BashOperator can't find bash
4 participants