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: Execute templated bash script as file #42783

Merged

Conversation

Joffreybvn
Copy link
Contributor

@Joffreybvn Joffreybvn commented Oct 6, 2024

This PR change the behavior of the BashOperator: When executing a .sh file with Jinja templating enabled (without the space -- .sh ), the rendered script is written back into a temporary file, and then executed. Instead of being directly executed as inline command.

Pasted image

Note: No changes / side-effect when executing a inline command or a script without Jinja templating ( .sh with space).

Why ?

When using templated bash command, the rendered string can be longer than the maximum argument length. This causes "Argument list too long" error.

Without this PR, currently, the bash command is passed as argument into ["bash", "-c", command]. The command cannot be longer than the max length of the system.

With this PR, the user can write his command into a bash file. The file is jinja-rendered and executed as a file, which bypass the maximum argument length limit.

Breaking change

The BashOperator writes the rendered file to the disk. By default, it's in a temporary directory (/tmp) where Airflow has write access (in a common Linux system). However, if the user specify a cwd directory where Airflow cannot write, task will fail.

Backport #43191 warns the user when permissions are missing, and fallback to executing the script as inline command.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Joffreybvn Joffreybvn requested a review from potiuk as a code owner October 6, 2024 16:02
@boring-cyborg boring-cyborg bot added area:core-operators Operators, Sensors and hooks within Core Airflow kind:documentation labels Oct 6, 2024
@eladkal
Copy link
Contributor

eladkal commented Oct 7, 2024

Breaking change

The BashOperator writes the rendered file to the disk. By default, it's in a temporary directory (/tmp) where Airflow has write access (in a common Linux system). However, if the user specify a cwd directory where Airflow cannot write, task will fail.

IMO this will affect only a small portion of users. Should I add a flag to control the behavior of the Operator ? Or maybe another PR with a warning that behavior will change in the future ?

We need newsfragment in the PR that explains the change.
We need a new PR against v 2.10 test branch that adds the warnning and allow users to migrate to the new functionality. We show warning only if user can do something about it

@Joffreybvn Joffreybvn force-pushed the feature/bashoperator-execute-rendered-bash-scripts branch from 33ed823 to 8afa080 Compare October 19, 2024 10:59
@Joffreybvn
Copy link
Contributor Author

Created a new PR #43191, which implements this feature with a fallback to the former behaviour in case of permission issue; and a warning for the user to fix it.

@potiuk potiuk merged commit 0e112bf into apache:main Oct 24, 2024
56 checks passed
PaulKobow7536 pushed a commit to PaulKobow7536/airflow that referenced this pull request Oct 24, 2024
* feat: execute templated shell script as file

* feat: Add documentation

* fix: docstring typo

* feat: Add newsfragment

* fix: mark tests using db with `@pytest.mark.db_test`

---------

Co-authored-by: Joffrey Bienvenu <[email protected]>
@utkarsharma2 utkarsharma2 added the type:bug-fix Changelog: Bug Fixes label Oct 24, 2024
@utkarsharma2 utkarsharma2 added this to the Airflow 2.10.3 milestone Oct 24, 2024
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
* feat: execute templated shell script as file

* feat: Add documentation

* fix: docstring typo

* feat: Add newsfragment

* fix: mark tests using db with `@pytest.mark.db_test`

---------

Co-authored-by: Joffrey Bienvenu <[email protected]>
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 kind:documentation type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants