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.

Sign up for GitHub

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

Jump to bottom

Allow setting specific cwd for BashOperator #17751

Merged
merged 15 commits into from
Aug 31, 2021
Merged

Allow setting specific cwd for BashOperator #17751

merged 15 commits into from
Aug 31, 2021

Conversation

LeonZhao28
Copy link
Contributor

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.

@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Aug 20, 2021
@LeonZhao28 LeonZhao28 changed the title add cwd in BaseOperator add cwd in BashOperator Aug 20, 2021
uranusjr
uranusjr reviewed Aug 20, 2021
View reviewed changes
airflow/operators/bash.py Outdated Show resolved Hide resolved
tests/operators/test_bash.py Outdated Show resolved Hide resolved
airflow/hooks/subprocess.py Outdated Show resolved Hide resolved
uranusjr
uranusjr requested changes Aug 22, 2021
View reviewed changes
airflow/operators/bash.py Outdated Show resolved Hide resolved
uranusjr
uranusjr requested changes Aug 23, 2021
View reviewed changes
tests/operators/test_bash.py Outdated Show resolved Hide resolved
tests/operators/test_bash.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Aug 23, 2021
@github-actions
Copy link

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.

@LeonZhao28
Copy link
Contributor Author

@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.

uranusjr
uranusjr requested changes Aug 23, 2021
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should avoid the need to add cwd to the dictionary. I think more people are familiar with the term working directory (it’s also easier to Google this term if they aren’t).

airflow/hooks/subprocess.py Outdated Show resolved Hide resolved
airflow/hooks/subprocess.py Outdated Show resolved Hide resolved
airflow/operators/bash.py Outdated Show resolved Hide resolved
docs/spelling_wordlist.txt Outdated Show resolved Hide resolved
@LeonZhao28
Copy link
Contributor Author

@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?

@uranusjr
Copy link
Member

Yes I need to re-approve. But first place rebase to main.

@LeonZhao28
Copy link
Contributor Author

@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.

@LeonZhao28 LeonZhao28 closed this Aug 27, 2021
@LeonZhao28 LeonZhao28 reopened this Aug 27, 2021
uranusjr
uranusjr approved these changes Aug 28, 2021
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go if CI passes.

@LeonZhao28
Copy link
Contributor Author

I think this is good to go if CI passes.

@uranusjr But it failed again. 😅

@LeonZhao28
Copy link
Contributor Author

@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

@LeonZhao28
Copy link
Contributor Author

trigger CI

@LeonZhao28 LeonZhao28 reopened this Aug 29, 2021
@LeonZhao28 LeonZhao28 closed this Aug 30, 2021
@LeonZhao28
Copy link
Contributor Author

trigger CI

@LeonZhao28 LeonZhao28 reopened this Aug 30, 2021
@uranusjr
Copy link
Member

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.

LeonZhao28 and others added 15 commits August 31, 2021 10:05
@LeonZhao28
add cwd in BaseOperator
be17035
@LeonZhao28
change the bash_cwd to cwd
dabec27
@LeonZhao28
add cwd in BashOperator
2b1e70a
@LeonZhao28
check cwd in execute() in BashOperator
0c79a7c
@LeonZhao28
update the unittest about cwd
282d9dc
@LeonZhao28 @uranusjr
Update tests/operators/test_bash.py
989cf32 
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@LeonZhao28
fix the static check error
9a8f0b9
@LeonZhao28
added cwd into the spelling_wordlist
faf63f1
@LeonZhao28 @uranusjr
Update docs/spelling_wordlist.txt
ca1c91d 
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@LeonZhao28 @uranusjr
Update airflow/operators/bash.py
e49457f 
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@LeonZhao28 @uranusjr
Update airflow/hooks/subprocess.py
5b3d1f4 
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@LeonZhao28 @uranusjr
Update airflow/hooks/subprocess.py
1a8af1d 
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@LeonZhao28
Update the test test_cwd_does_not_exist in test_bash.py
d8aa6a6
@LeonZhao28
Update the test test_cwd_does_not_exist in test_bash.py
53683ff
@LeonZhao28
Update the test test_cwd_does_not_exist in test_bash.py
dce4e40
@LeonZhao28
Copy link
Contributor Author

LeonZhao28 commented Aug 31, 2021 •
edited

As the test bug is fixed in the PR: #17916
I rebase to main again and check the test again

@uranusjr uranusjr changed the title add cwd in BashOperator Allow setting specific cwd for BashOperator Aug 31, 2021
@uranusjr uranusjr merged commit 098765e into apache:main Aug 31, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 31, 2021

Awesome work, congrats on your first merged pull request!

@LeonZhao28 LeonZhao28 deleted the bash_operate_support_cwd branch August 31, 2021 10:52
@eladkal eladkal added this to the Airflow 2.2 milestone Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uranusjr uranusjr uranusjr approved these changes

@jedcunningham jedcunningham jedcunningham approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

@vikramkoka vikramkoka Awaiting requested review from vikramkoka

Assignees
No one assigned
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Milestone
Airflow 2.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@LeonZhao28 @uranusjr @jedcunningham @eladkal