shahar1 commented on code in PR #70437:
URL: https://github.com/apache/airflow/pull/70437#discussion_r3657103308
##########
providers/standard/src/airflow/providers/standard/operators/bash.py:
##########
@@ -215,7 +213,7 @@ def execute(self, context: Context):
raise AirflowException(f"The cwd {self.cwd} must be a
directory")
env = self.get_env(context)
- if self._is_inline_cmd:
+ if self._is_inline_command(self.bash_command):
Review Comment:
@bramhanandlingala
**Human note:** I think that I've led you astray, my sincere apologies - you
implemented my suggestion exactly as I wrote it, and the suggestion was wrong.
I only checked it properly after you pushed. The following describes the
mistake and the suggested fixes.
---
Drafted by Claude (using Apache Magpie), reviewed and verified by me:
What I got wrong: `template_ext` files aren't substituted at render time,
they're substituted at **parse** time, by `resolve_template_files()`
([dagbag.py:407](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/dag_processing/dagbag.py#L407)
->
[templater.py:91](https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/definitions/_internal/templater.py#L91)).
So `render_template_fields` runs *after* the `.sh` path has already become the
file's contents, and classifying there hits the exact failure I was trying to
warn you about. Same Dag, only these two files swapped:
```text
this branch main
_is_inline_cmd : True False
239 KB script : OSError [Errno 7] Arg list too long OK
$0 : /usr/bin/bash tmpXXXX.sh
```
One hook earlier is the right place - it's the last point where the field
still holds the value the DagTwo things go with it:
- Restore the line you deleted from `decorators/bash.py::execute()`, above
`self.render_template_fields(context)` - at parse time the decorator's value is
still the `SET_DURING_EXECUTION` sentinel, so without it `@task.bash` returning
a `.sh` filename runs inline.
- **Don't** add an `isinstance` guard here, even though it looks like the
safe move. Unguarded, `False` means "parse ran, value wasn't a string" and
`None` means "parse never ran", and the `is None` fallback in `execute()` needs
that distinction - `bash_command=some_task.output` has to stay on the
script-file path. A guard collapses both to `None` and breaks one of them. Keep
the `Any` widening, it's load-bearing for this.
I tried five variants of the fix across ten scenarios, this is the only one
that matches `main` on all of them. Small knock-on:
`decorators/test_bash.py:107` asserts `_is_inline_cmd is None`, which becomes
`False` - both falsy, same meaning, just needs the one-line update.
Last thing - the new test passes on `main` unchanged, since it asserts on
the `_is_inline_command` classmethod rather than on which strategy the operator
actually picked. Worth asserting `op._is_inline_cmd` (or the argv the
`SubprocessHook` gets) after a full parse + render instead, so it fails without
the fix. Happy to hand you both tests if that's easier.
---
Again, sorry for the round trip - this one was on me, not on your work.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
