You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/08/27 15:49:00 UTC
[GitHub] [airflow] LionelZhao28 opened a new pull request #17751: add `cwd` in `BashOperator`
LionelZhao28 opened a new pull request #17751:
URL: https://github.com/apache/airflow/pull/17751
For this change, the `BashOperator` allows the user to specify the command folder.
As if the user wants to execute a command in the amount folder which contains lots of project scripts, he can just specify the bash_cwd to the folder and he doesn't need to switch the folder manually.
I did the pre-commit check and added the unit-test for the changes.
If I miss other necessary works for creating the PR, please let me know.
Thanks a lot.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 closed pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 closed pull request #17751:
URL: https://github.com/apache/airflow/pull/17751
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-906308024
@uranusjr I'm a little confused about this issue. I have committed the `request changes` already. And it shows `changes requested` , do you need to approve this again? Or I just close and open the PR to re-try the test?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #17751: Allow setting specific cwd for BashOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-908905247
Awesome work, congrats on your first merged pull request!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692832663
##########
File path: tests/operators/test_bash.py
##########
@@ -123,6 +125,41 @@ def test_command_not_found(self):
):
BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+ def test_command_without_cwd(self):
+ val = "xxxx"
+ op = BashOperator(task_id='abc', bash_command=f'set -e; echo "{val}";')
+ line = op.execute({})
+ assert line == val
+
+ def test_command_with_cwd(self):
+
+ test_cwd_folder = "/tmp/airflow/test_bash/test_command_with_cwd"
+ test_file_name = "py_test.txt"
+ test_cwd_file_path = f"{test_cwd_folder}/{test_file_name}"
+ # The command is echo a string to a file into cwd folder and return the string as the output
+ test_cmd = f'set -e; echo "xxxx" |tee {test_file_name}'
+
+ # Test the the bash_cwd doesn't exist
+ if os.path.exists(test_cwd_folder):
+ _shutil.rmtree(test_cwd_folder)
Review comment:
Changed it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693601105
##########
File path: airflow/operators/bash.py
##########
@@ -134,13 +137,20 @@ def __init__(
env: Optional[Dict[str, str]] = None,
output_encoding: str = 'utf-8',
skip_exit_code: int = 99,
+ cwd: str = None,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.bash_command = bash_command
self.env = env
self.output_encoding = output_encoding
self.skip_exit_code = skip_exit_code
+ self.cwd = cwd
+ if self.cwd is not None:
+ if not os.path.exists(self.cwd):
+ raise AirflowException(f"Can not find the cwd: {cwd}")
+ if not os.path.isdir(self.cwd):
+ raise AirflowException(f"The cwd {cwd} must be a directory")
Review comment:
You are right. And the unit test already did cover it.
I will update it
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #17751: Allow setting specific cwd for BashOperator
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #17751:
URL: https://github.com/apache/airflow/pull/17751
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692833057
##########
File path: airflow/hooks/subprocess.py
##########
@@ -80,6 +85,13 @@ def pre_exec():
self.sub_process.wait()
self.log.info('Command exited with return code %s', self.sub_process.returncode)
+ return line
+
+ if cwd is None:
+ with TemporaryDirectory(prefix='airflowtmp') as tmp_dir:
+ line = __run_subprocess(tmp_dir)
+ else:
+ line = __run_subprocess(cwd)
Review comment:
Thanks for sharing this with me.
I've change it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-907989990
trigger CI
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 closed pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 closed pull request #17751:
URL: https://github.com/apache/airflow/pull/17751
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692810818
##########
File path: airflow/hooks/subprocess.py
##########
@@ -35,24 +35,29 @@ def __init__(self) -> None:
super().__init__()
def run_command(
- self, command: List[str], env: Optional[Dict[str, str]] = None, output_encoding: str = 'utf-8'
+ self,
+ command: List[str],
+ env: Optional[Dict[str, str]] = None,
+ output_encoding: str = 'utf-8',
+ bash_cwd: str = None,
Review comment:
Done
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-903496095
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 main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #17751: Allow setting specific cwd for BashOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-908905247
Awesome work, congrats on your first merged pull request!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-906857662
@uranusjr I rebase to main already. I followed the doc
https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#how-to-rebase-pr
I hope what I did is correct.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693510046
##########
File path: airflow/operators/bash.py
##########
@@ -134,13 +137,20 @@ def __init__(
env: Optional[Dict[str, str]] = None,
output_encoding: str = 'utf-8',
skip_exit_code: int = 99,
+ cwd: str = None,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.bash_command = bash_command
self.env = env
self.output_encoding = output_encoding
self.skip_exit_code = skip_exit_code
+ self.cwd = cwd
+ if self.cwd is not None:
+ if not os.path.exists(self.cwd):
+ raise AirflowException(f"Can not find the cwd: {cwd}")
+ if not os.path.isdir(self.cwd):
+ raise AirflowException(f"The cwd {cwd} must be a directory")
Review comment:
This would raise an error on DAG parsing and force the scheduler to have the same directory set up as the task runner (which can be different machines). So I think we should *not* check `self.cwd` here, but only in `execute()`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692826579
##########
File path: tests/operators/test_bash.py
##########
@@ -123,6 +125,41 @@ def test_command_not_found(self):
):
BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+ def test_command_without_cwd(self):
+ val = "xxxx"
+ op = BashOperator(task_id='abc', bash_command=f'set -e; echo "{val}";')
+ line = op.execute({})
+ assert line == val
+
+ def test_command_with_cwd(self):
+
+ test_cwd_folder = "/tmp/airflow/test_bash/test_command_with_cwd"
+ test_file_name = "py_test.txt"
+ test_cwd_file_path = f"{test_cwd_folder}/{test_file_name}"
+ # The command is echo a string to a file into cwd folder and return the string as the output
+ test_cmd = f'set -e; echo "xxxx" |tee {test_file_name}'
+
+ # Test the the bash_cwd doesn't exist
+ if os.path.exists(test_cwd_folder):
+ _shutil.rmtree(test_cwd_folder)
Review comment:
I have a question that what I want to do is make sure the directory doesn't exist at this time, so I try to clean it.
I check the source codes of `TemporaryDirectory`, the `__exit__` method is `_shutil.rmtree(name)`.
But I agree the following test we can use `TemporaryDirectory` to do this
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693703901
##########
File path: tests/operators/test_bash.py
##########
@@ -123,6 +125,36 @@ def test_command_not_found(self):
):
BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+ def test_command_without_cwd(self):
+ val = "xxxx"
+ op = BashOperator(task_id='abc', bash_command=f'set -e; echo "{val}";')
+ line = op.execute({})
+ assert line == val
+
+ def test_command_with_cwd(self):
+
+ # The command is echo a string to a file into cwd folder and return the string as the output
+ test_cmd = f'set -e; echo "xxxx" |tee outputs.txt'
+
+ with TemporaryDirectory(prefix='test_command_with_cwd') as test_cwd_folder:
+ # Test if the cwd is a file_path
+ test_1_file_path = f'{test_cwd_folder}/test1.txt'
+ with open(test_1_file_path, 'w') as tmp_file:
+ tmp_file.write("xxxxx")
+ with pytest.raises(AirflowException, match=f"The cwd {test_1_file_path} must be a directory"):
+ BashOperator(
+ task_id='abc', bash_command=test_cmd, cwd=f'{test_cwd_folder}/test1.txt'
+ ).execute({})
Review comment:
Thanks for the suggestions.
Changed it already.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 closed pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 closed pull request #17751:
URL: https://github.com/apache/airflow/pull/17751
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-908837800
As the test bug is fix in the PR: https://github.com/apache/airflow/pull/17916
I rebase to main again and check the test again
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-907726486
@uranusjr I found that the test `test_cwd_does_not_exist` has a bug, it didn't cover the case that there should be no exceptions when the `cwd` doesn't exist.
So I pushed a new commit.
Please review it again.
Thanks
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-906318234
Yes I need to re-approve. But first place rebase to `main`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693694626
##########
File path: tests/operators/test_bash.py
##########
@@ -123,6 +125,36 @@ def test_command_not_found(self):
):
BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+ def test_command_without_cwd(self):
+ val = "xxxx"
+ op = BashOperator(task_id='abc', bash_command=f'set -e; echo "{val}";')
+ line = op.execute({})
+ assert line == val
+
+ def test_command_with_cwd(self):
+
+ # The command is echo a string to a file into cwd folder and return the string as the output
+ test_cmd = f'set -e; echo "xxxx" |tee outputs.txt'
+
+ with TemporaryDirectory(prefix='test_command_with_cwd') as test_cwd_folder:
+ # Test if the cwd is a file_path
+ test_1_file_path = f'{test_cwd_folder}/test1.txt'
+ with open(test_1_file_path, 'w') as tmp_file:
+ tmp_file.write("xxxxx")
+ with pytest.raises(AirflowException, match=f"The cwd {test_1_file_path} must be a directory"):
+ BashOperator(
+ task_id='abc', bash_command=test_cmd, cwd=f'{test_cwd_folder}/test1.txt'
+ ).execute({})
+
+ # Test everything goes alright
Review comment:
Nit
```suggestion
# Test everything went alright
```
##########
File path: tests/operators/test_bash.py
##########
@@ -123,6 +125,36 @@ def test_command_not_found(self):
):
BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+ def test_command_without_cwd(self):
+ val = "xxxx"
+ op = BashOperator(task_id='abc', bash_command=f'set -e; echo "{val}";')
+ line = op.execute({})
+ assert line == val
+
+ def test_command_with_cwd(self):
+
+ # The command is echo a string to a file into cwd folder and return the string as the output
+ test_cmd = f'set -e; echo "xxxx" |tee outputs.txt'
+
+ with TemporaryDirectory(prefix='test_command_with_cwd') as test_cwd_folder:
+ # Test if the cwd is a file_path
+ test_1_file_path = f'{test_cwd_folder}/test1.txt'
+ with open(test_1_file_path, 'w') as tmp_file:
+ tmp_file.write("xxxxx")
+ with pytest.raises(AirflowException, match=f"The cwd {test_1_file_path} must be a directory"):
+ BashOperator(
+ task_id='abc', bash_command=test_cmd, cwd=f'{test_cwd_folder}/test1.txt'
+ ).execute({})
Review comment:
This should be split into a separate function (and use `TemporaryFile` instead).
It’s probably best to split `test_command_with_cwd` into three functions:
1. `cwd` does not exist.
2. `cwd` exists but is not a directory.
3. `cwd` points to a valid directory.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-907750113
trigger CI
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #17751: Allow setting specific cwd for BashOperator
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #17751:
URL: https://github.com/apache/airflow/pull/17751
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692809269
##########
File path: airflow/operators/bash.py
##########
@@ -134,13 +137,20 @@ def __init__(
env: Optional[Dict[str, str]] = None,
output_encoding: str = 'utf-8',
skip_exit_code: int = 99,
+ bash_cwd: str = None,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.bash_command = bash_command
self.env = env
self.output_encoding = output_encoding
self.skip_exit_code = skip_exit_code
+ self.bash_cwd = bash_cwd
+ if self.bash_cwd is not None:
+ if not os.path.exists(self.bash_cwd):
+ raise AirflowException(f"Can not find the bash_cwd: {bash_cwd}")
+ if not os.path.isdir(self.bash_cwd):
+ raise AirflowException(f"The bash_cwd {bash_cwd} must be a folder")
Review comment:
```suggestion
raise AirflowException(f"The bash_cwd {bash_cwd} must be a directory")
```
(The two are slightly different and technically we don’t need it to be a folder.)
##########
File path: tests/operators/test_bash.py
##########
@@ -123,6 +125,41 @@ def test_command_not_found(self):
):
BashOperator(task_id='abc', bash_command='set -e; something-that-isnt-on-path').execute({})
+ def test_command_without_cwd(self):
+ val = "xxxx"
+ op = BashOperator(task_id='abc', bash_command=f'set -e; echo "{val}";')
+ line = op.execute({})
+ assert line == val
+
+ def test_command_with_cwd(self):
+
+ test_cwd_folder = "/tmp/airflow/test_bash/test_command_with_cwd"
+ test_file_name = "py_test.txt"
+ test_cwd_file_path = f"{test_cwd_folder}/{test_file_name}"
+ # The command is echo a string to a file into cwd folder and return the string as the output
+ test_cmd = f'set -e; echo "xxxx" |tee {test_file_name}'
+
+ # Test the the bash_cwd doesn't exist
+ if os.path.exists(test_cwd_folder):
+ _shutil.rmtree(test_cwd_folder)
Review comment:
It’d be easier to use `tempfile.TemporaryDirectory` instead of manually cleaning things up. This would also be friendly for cross-platform support (we are working toward having a Windows-compatible task runner).
##########
File path: airflow/hooks/subprocess.py
##########
@@ -80,6 +85,13 @@ def pre_exec():
self.sub_process.wait()
self.log.info('Command exited with return code %s', self.sub_process.returncode)
+ return line
+
+ if cwd is None:
+ with TemporaryDirectory(prefix='airflowtmp') as tmp_dir:
+ line = __run_subprocess(tmp_dir)
+ else:
+ line = __run_subprocess(cwd)
Review comment:
This can be more easily implemented with `contextlib.ExitStack`:
```python
with contextlib.ExitStack() as stack:
if cwd is None:
cwd = stack.enter_context(TemporaryDirectory(prefix='airflowtmp'))
# Now you don’t need the __run_subprocess wrapper.
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693603236
##########
File path: airflow/operators/bash.py
##########
@@ -134,13 +137,20 @@ def __init__(
env: Optional[Dict[str, str]] = None,
output_encoding: str = 'utf-8',
skip_exit_code: int = 99,
+ cwd: str = None,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.bash_command = bash_command
self.env = env
self.output_encoding = output_encoding
self.skip_exit_code = skip_exit_code
+ self.cwd = cwd
+ if self.cwd is not None:
+ if not os.path.exists(self.cwd):
+ raise AirflowException(f"Can not find the cwd: {cwd}")
+ if not os.path.isdir(self.cwd):
+ raise AirflowException(f"The cwd {cwd} must be a directory")
Review comment:
I've updated it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-907724750
> I _think_ this is good to go if CI passes.
@uranusjr But it failed again. 😅
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693603423
##########
File path: airflow/operators/bash.py
##########
@@ -134,13 +137,20 @@ def __init__(
env: Optional[Dict[str, str]] = None,
output_encoding: str = 'utf-8',
skip_exit_code: int = 99,
+ cwd: str = None,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.bash_command = bash_command
self.env = env
self.output_encoding = output_encoding
self.skip_exit_code = skip_exit_code
+ self.cwd = cwd
+ if self.cwd is not None:
+ if not os.path.exists(self.cwd):
+ raise AirflowException(f"Can not find the cwd: {cwd}")
+ if not os.path.isdir(self.cwd):
+ raise AirflowException(f"The cwd {cwd} must be a directory")
Review comment:
I didn't saw you commit the changes.
But I think I made the same changes as yours.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 edited a comment on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 edited a comment on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-908837800
As the test bug is fixed in the PR: https://github.com/apache/airflow/pull/17916
I rebase to main again and check the test again
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-908141571
Looks like `test_current_user_has_permissions` is failing in `main` as well, looks like a race condition issue? I don’t think it’s related to this PR.
All tests are passing except that one, so this is good to go IMO.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-903498163
@uranusjr This is the first time that I create the PR to airflow.
As there are many different coding habits, there are lots of updates for this review. I appreciate your patience and suggestions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693824611
##########
File path: airflow/hooks/subprocess.py
##########
@@ -35,23 +35,31 @@ def __init__(self) -> None:
super().__init__()
def run_command(
- self, command: List[str], env: Optional[Dict[str, str]] = None, output_encoding: str = 'utf-8'
+ self,
+ command: List[str],
+ env: Optional[Dict[str, str]] = None,
+ output_encoding: str = 'utf-8',
+ cwd: str = None,
) -> SubprocessResult:
"""
- Execute the command in a temporary directory which will be cleaned afterwards
+ Execute the command.
+ If `cwd` is None, execute the command in a temporary directory which will be cleaned afterwards.
Review comment:
```suggestion
If ``cwd`` is None, execute the command in a temporary directory which will be cleaned afterwards.
```
##########
File path: airflow/hooks/subprocess.py
##########
@@ -35,23 +35,31 @@ def __init__(self) -> None:
super().__init__()
def run_command(
- self, command: List[str], env: Optional[Dict[str, str]] = None, output_encoding: str = 'utf-8'
+ self,
+ command: List[str],
+ env: Optional[Dict[str, str]] = None,
+ output_encoding: str = 'utf-8',
+ cwd: str = None,
) -> SubprocessResult:
"""
- Execute the command in a temporary directory which will be cleaned afterwards
+ Execute the command.
+ If `cwd` is None, execute the command in a temporary directory which will be cleaned afterwards.
If ``env`` is not supplied, ``os.environ`` is passed
:param command: the command to run
:param env: Optional dict containing environment variables to be made available to the shell
environment in which ``command`` will be executed. If omitted, ``os.environ`` will be used.
:param output_encoding: encoding to use for decoding stdout
+ :param cwd: the bash cwd.
+ If it is None, generate a temporary directory which will be cleaned afterwards
Review comment:
```suggestion
:param cwd: Working directory to run the command in.
If None (default), the command is run in a temporary directory.
```
##########
File path: docs/spelling_wordlist.txt
##########
@@ -44,6 +44,7 @@ Booleans
Boto
BounceX
Bq
+cwd
Review comment:
```suggestion
```
##########
File path: airflow/operators/bash.py
##########
@@ -50,6 +50,9 @@ class BashOperator(BaseOperator):
in ``skipped`` state (default: 99). If set to ``None``, any non-zero
exit code will be treated as a failure.
:type skip_exit_code: int
+ :param cwd: The folder that the command is executed.
+ If it is None, the operator will a temporary directory which will be cleaned afterwards.
Review comment:
```suggestion
:param cwd: Working directory to execute the command in.
If None (default), the command is run in a temporary directory.
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r693828558
##########
File path: docs/spelling_wordlist.txt
##########
@@ -44,6 +44,7 @@ Booleans
Boto
BounceX
Bq
+cwd
Review comment:
OK. committed the suggestions. Thanks
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692824689
##########
File path: airflow/operators/bash.py
##########
@@ -134,13 +137,20 @@ def __init__(
env: Optional[Dict[str, str]] = None,
output_encoding: str = 'utf-8',
skip_exit_code: int = 99,
+ bash_cwd: str = None,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.bash_command = bash_command
self.env = env
self.output_encoding = output_encoding
self.skip_exit_code = skip_exit_code
+ self.bash_cwd = bash_cwd
+ if self.bash_cwd is not None:
+ if not os.path.exists(self.bash_cwd):
+ raise AirflowException(f"Can not find the bash_cwd: {bash_cwd}")
+ if not os.path.isdir(self.bash_cwd):
+ raise AirflowException(f"The bash_cwd {bash_cwd} must be a folder")
Review comment:
Got it.
changing it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-908837800
As the test bug is fix in the PR: https://github.com/apache/airflow/pull/17916
I rebase to main again and check the test again
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 edited a comment on pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 edited a comment on pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#issuecomment-908837800
As the test bug is fixed in the PR: https://github.com/apache/airflow/pull/17916
I rebase to main again and check the test again
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692807439
##########
File path: airflow/hooks/subprocess.py
##########
@@ -35,24 +35,29 @@ def __init__(self) -> None:
super().__init__()
def run_command(
- self, command: List[str], env: Optional[Dict[str, str]] = None, output_encoding: str = 'utf-8'
+ self,
+ command: List[str],
+ env: Optional[Dict[str, str]] = None,
+ output_encoding: str = 'utf-8',
+ bash_cwd: str = None,
Review comment:
I feel this argument should simply be `cwd`. This matches the `subprocess` interface, which are probably familiar to many developers with Python experience.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692833057
##########
File path: airflow/hooks/subprocess.py
##########
@@ -80,6 +85,13 @@ def pre_exec():
self.sub_process.wait()
self.log.info('Command exited with return code %s', self.sub_process.returncode)
+ return line
+
+ if cwd is None:
+ with TemporaryDirectory(prefix='airflowtmp') as tmp_dir:
+ line = __run_subprocess(tmp_dir)
+ else:
+ line = __run_subprocess(cwd)
Review comment:
Thanks for sharing this with me.
I've changed it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] LionelZhao28 commented on a change in pull request #17751: add `cwd` in `BashOperator`
Posted by GitBox <gi...@apache.org>.
LionelZhao28 commented on a change in pull request #17751:
URL: https://github.com/apache/airflow/pull/17751#discussion_r692808423
##########
File path: airflow/hooks/subprocess.py
##########
@@ -35,24 +35,29 @@ def __init__(self) -> None:
super().__init__()
def run_command(
- self, command: List[str], env: Optional[Dict[str, str]] = None, output_encoding: str = 'utf-8'
+ self,
+ command: List[str],
+ env: Optional[Dict[str, str]] = None,
+ output_encoding: str = 'utf-8',
+ bash_cwd: str = None,
Review comment:
😄, that's right.
I change it now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org