Add SubprocessHook for running commands from operators

[dstandish]

The bash operator has dialed in how to run bash commands from python and airflow.

There are other operators that sometimes need to do this. But for various reasons, they might not want to inherit from BashOperator. And in this case, it would be nice if there were a bash hook that would just let you run a bash command in your operator.

We can also imagine circumstances where perhaps within a single "BashOperator", maybe you want to run a series of bash commands. Well, if you have a hook that encapsulates this functionality, then this is very clean to implement.

[github-actions]

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

[github-actions]

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

[turbaszek]

Looks good to me. For few minutes I was considering making this hook stateless and keeping the state in operators but this adds unnecessary complexity.

[github-actions]

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.

[dstandish]

For few minutes I was considering making this hook stateless and keeping the state in operators but this adds unnecessary complexity.

Yes @turbaszek, this is one of the things I was thinking about also.

I could not think of a clean way, apart from having BashHook return a "runner" object that has methods run and send_sigterm.... but in essense this would just be another stateful hook, so might as well just leave it on BashHook, I figure.

I also considered making command env and output_encoding hook init params. But then you would not be able to use cached property in BashOperator, and this would add complexity to BashOperator as a result. And we should be able to reuse the hook multiple times in different ways.

Another thing I considered was, does it make sense to ensure that sub_process is unset at successful completion? But I think there's no benefit to aggressive unsetting. If run_command is used again, then sub_process will be overwritten with new process. We could add a check, to see if sub_process is still running (if is already set when Popen is about to be called), or at least a warning. However, these I think should probably be considered separately from this refactor.

In sum, I too think current approach is good enough and now will work on updating docs and possibly tests as necessary.

Thanks

[dstandish]

one material change

in latest commit i made I made it so BashHook's run_command will use os.environ if env not supplied (and will not use os.environ otherwise)

added / updated tests

i left all but one BashOperator test untouched. only this one -- checking the args applied to Popen -- i moved to BashHook.

where appropriate, i translated the existing tests for BashHook and copied them there.

i will remove WIP designation

[github-actions]

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

[github-actions]

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

[turbaszek]

We could add a check, to see if sub_process is still running (if is already set when Popen is about to be called), or at least a warning. However, these I think should probably be considered separately from this refactor.

That was also my concern, but I figured that the chance that we will run two processes in parallel in one operator (using one instance of a hook) is low.

[dstandish]

Alright folks... updated to make it shell-agnostic, and to take List[str]

I have called it SubprocessHook

This is obvs negotiable

Options:

• CommandHook (what kind of command?)
• ProcessHook (what kind of process? isn't everything a process)
• ShellHook (i kindof like it)
• SubprocessHook (it is a subprocess, it uses subprocess module... why get creative?)

Please take a look 🙏

[ashb]

SubprocessHook is good.

(ShellHook is not because that has a different meaning -- see https://docs.python.org/3/library/subprocess.html#security-considerations)

[dstandish]

thanks and updated 🤞

[dstandish]

ok at long last @ashb i think this should be good

there seems to be just one test failure in one run which, while i don't see how it could be flakey, does appear to be flakey. it didn't happen in other configurations and doesn't repro locally.

perhaps someone could rerun that test? i am hesitant to force push again given how fussy builds have been lately

[ashb]

Yeah, that's a timing based flakey test. Merging.