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

[Airflow 16364] Add conn_timeout and cmd_timeout params to SSHOperator; add conn_timeout param to SSHHook #17236

Merged
merged 70 commits into from
Sep 10, 2021
Merged

[Airflow 16364] Add conn_timeout and cmd_timeout params to SSHOperator; add conn_timeout param to SSHHook #17236

merged 70 commits into from
Sep 10, 2021

Conversation

fatmumuhomer
Copy link
Contributor

@fatmumuhomer fatmumuhomer commented Jul 26, 2021 •
edited by uranusjr
Loading

closes #16364 (Timeout is ambiguous in SSHHook and SSHOperator)

Add conn_timeout and cmd_timeout params to SSHOperator. Add conn_timeout param to SSHHook. The legacy timeout param remains but is marked as deprecated. The new params take precedence over the legacy timeout one if both/all are provided.

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@boring-cyborg
Copy link

boring-cyborg bot commented Jul 26, 2021

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

@fatmumuhomer fatmumuhomer mentioned this pull request Jul 26, 2021
Closed
@fatmumuhomer fatmumuhomer reopened this Jul 26, 2021
@uranusjr
Copy link
Member

See inline comments (the same also apply to the operator class).

fatmumuhomer and others added 21 commits September 8, 2021 19:23
@fatmumuhomer
Fix static checks for operators/test_ssh.py and hooks/test_ssh.py
e51910f
@fatmumuhomer
Set deprecated timeout param default value to None; add more tests
05b62d9
@fatmumuhomer
Remove unnecessary extra_options.get() calls for timeout and conn_tim…
8347232 
…eout
@fatmumuhomer
Add reference to new timeout parameters in docstrings for deprecated …
cb5c301 
…timeout
@fatmumuhomer
Move timeout docstring text to 2nd line to keep line within 110 chara…
df5a5cd 
…cters
@fatmumuhomer
Favor conn_timeout parameter over extra options
b7d0115
@fatmumuhomer
Add conn_timeout to SSH Connection docs and mark timeout as deprecated
5c07819
@fatmumuhomer
Fix issue with defaults for timeouts in SSHHook/SSHOperator; add para…
d287e4d 
…metrized unit tests for SSHHook
@fatmumuhomer
Fix spelling mistake in ssh.rst
8399a81
@fatmumuhomer
Move details for conn_timeout to SSHHook docstring instead of ssh.rst
fa80b3a
@jedcunningham @fatmumuhomer
Chart: Make cleanup cronjob cmd/args configurable (#17970)
a68185d
@mik-laj @fatmumuhomer
Delete unnecessary parameters in EKSPodOperator (#17960)
e177208
@josh-fell @ashb @fatmumuhomer
Remove default_args pattern + added get_current_context() use for Cor…
a5f7249 
…e Airflow example DAGs (#16866)

Co-authored-by: Ash Berlin-Taylor <[email protected]>
@potiuk @fatmumuhomer
Fix constraint generation properly (#17964)
e9513b2 
The #17939 did not fix the problem finally. It turned out that
one more change was needed - since we now always upgrade to latest
dependencies in `push` and `schedule` type of build we do not need
to check for the variable UPGRADE_TO_NEWER_DEPENDENCIES (which
was not set in "Build Image" step.

This fixes it, but also changes the constraint generation to add
comments in the generated constraint files, describing how and
why the files are generated.
@ephraimbuddy @fatmumuhomer
Fix max_active_runs not allowing moving of queued dagruns to running (#…
1e8ce4f 
…17945)

Currently, if you set max_active_runs for a dag and that dag has many queued dagruns
with execution dates older than another dag's queued dagruns, airflow will not move
the newer queued dagruns to running with the effect that only one dagruns would be in running at any time

This PR fixes this by updating the DagRun.last_scheduling_decision whenever a decision of scheduling
was made
@bbenshalom @uranusjr
@ephraimbuddy @fatmumuhomer
Add DAG run endpoint for marking a dagrun success or failed(#17839)
ff925ae 
Co-authored-by: bbenshalom <[email protected]>
Co-authored-by: Tzu-ping Chung <[email protected]>
Co-authored-by: Ephraim Anierobi <[email protected]>
@bbovenzi @fatmumuhomer
Fix blank dag dependencies view (#17990)
6822833 
* Fix blank dag dependencies view

* calculate graph if node and edges are empty
@BasPH @fatmumuhomer
Serialize the template_ext attribute to show it in UI (#17985)
81cfaa1 
Co-authored-by: Bas Harenslak <[email protected]>
@fatmumuhomer
Remove commented out line
40f3cfe
@fatmumuhomer
Merge branch 'airflow-16364' of https://github.com/fatmumuhomer/airflow…
1872901 
… into airflow-16364-v2
@fatmumuhomer
Merge branch 'main' of https://github.com/apache/airflow into airflow…
99bcf65 
…-16364-v2
@potiuk potiuk merged commit 68d99bc into apache:main Sep 10, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 10, 2021

Awesome work, congrats on your first merged pull request!

@potiuk
Copy link
Member

potiuk commented Sep 10, 2021

Woohooo!

@fatmumuhomer fatmumuhomer deleted the airflow-16364 branch September 10, 2021 14:45
@ashb
Copy link
Member

ashb commented Sep 10, 2021

Also congrats on being the contributor who edged Airflow ahead of Spark in terms of number of contributors :D

image

image

@fatmumuhomer
Copy link
Contributor Author

@ashb That's a pretty cool honor :)

Thanks to everyone who helped me with this!

@potiuk
Copy link
Member

potiuk commented Sep 10, 2021

How cool's that !

@potiuk potiuk mentioned this pull request Sep 30, 2021
Closed
56 tasks
@o-nikolas o-nikolas mentioned this pull request Oct 25, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedcunningham jedcunningham jedcunningham approved these changes

@uranusjr uranusjr uranusjr approved these changes

@potiuk potiuk potiuk 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:providers okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timeout is ambiguous in SSHHook and SSHOperator