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-4574] SSHHook private_key may only be supplied in extras #6163

Merged
merged 1 commit into from
Oct 18, 2019
Merged

[AIRFLOW-4574] SSHHook private_key may only be supplied in extras #6163

merged 1 commit into from
Oct 18, 2019

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Sep 21, 2019 •
edited
Loading

Summary:

  • discussion on original PR suggested removing private_key option as init param
  • with this PR, can still provide through extras, but not as init param
  • also add support for private_key in tunnel -- missing in original PR for this issue
  • remove test related to private_key init param
  • use context manager to auto-close socket listener so tests can be re-run

Notes:
@mik-laj @pgagnon @kaxil
In this PR I attempt to address concerns raised by @pgagnon after the merge of original PR for this issue (#6104 ).

Namely, I set out to remove private_key as an init param to SSHHook.

While doing so, I noticed that original PR did not extend support for private_key to the get_tunnel hook method, because it doesn't use get_conn but connects independently.

This PR rectifies this oversight by adding this capability.

I did not create new jira because this feels like continuation of same issue -- just more completely fulfilling its goal, and hopefully in a way that everyone can be happy with.

There were some minor tweaks that I made to testing.

  • In test, The HELLO_SERVER_CMD was not executed in context manager, so it left the socket listener running, which meant you could not rerun the tests without manually killing the listener process. I use context manager. I think this makes sense in same PR because it actively interfered with my ability to test my change.
  • In test, I also moved connection creation / destruction to setUpClass / tearDownClass so they are created and destroyed only once for the ssh hook test suite. Made sense to do this because in adding tunnel test I had to use the connection in more than one place.

Make sure you have checked all steps below.

Jira

  • [ x] My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
    • https://issues.apache.org/jira/browse/AIRFLOW-4574
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

  • [ x] Here are some details about my PR, including screenshots of any UI changes:

Tests

  • [x ] My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • [ x] My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • [x ] In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@dstandish
[AIRFLOW-4574] SSHHook private_key may only be supplied in extras
76f48e8 
* discussion on original PR suggested removing private_key option as init param
* with this PR, can still provide through extras, but not as init param
* also add support for private_key in tunnel -- missing in original PR for this issue
* remove test related to private_key init param
* use context manager to auto-close socket listener so tests can be re-run
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@123479c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6163   +/-   ##
=========================================
  Coverage          ?   80.06%           
=========================================
  Files             ?      608           
  Lines             ?    35030           
  Branches          ?        0           
=========================================
  Hits              ?    28046           
  Misses            ?     6984           
  Partials          ?        0
Impacted Files Coverage Δ
airflow/contrib/hooks/ssh_hook.py 88.49% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 123479c...76f48e8. Read the comment docs.

@dstandish
Copy link
Contributor Author

@mik-laj is this PR ok or should i split it up -- one to fix the oversight (missing tunnel support) and another to remove primary_key as __init__ param?

mik-laj
mik-laj reviewed Oct 17, 2019
View reviewed changes
tests/contrib/hooks/test_ssh_hook.py
args=["python", "-c", HELLO_SERVER_CMD],
stdout=subprocess.PIPE,
)
with subprocess.Popen(**subprocess_kwargs) as server_handle, hook.create_tunnel(2135, 2134):
Copy link
Member

Choose a reason for hiding this comment

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

Nice!.

@mik-laj
Copy link
Member

mik-laj commented Oct 17, 2019

I am sorry that I missed this PR earlier. The patch looks good to me, but @ashb wants to look, so I'll wait for its review.

In the next PR I would like to divide the cleaning changes in the tests from the changes in functionality. This made it easier to review the changes.

@dstandish
Copy link
Contributor Author

I am sorry that I missed this PR earlier.

no worries

In the next PR I would like to divide the cleaning changes in the tests from the changes in functionality.

will do next time

thank you

@ashb ashb merged commit 0790ede into apache:master Oct 18, 2019
ashb pushed a commit to ashb/airflow that referenced this pull request Oct 18, 2019
@dstandish @ashb
[AIRFLOW-4574] SSHHook private_key may only be supplied in extras (ap…
c56138c 
…ache#6163)

* discussion on original PR suggested removing private_key option as init param
* with this PR, can still provide through extras, but not as init param
* also add support for private_key in tunnel -- missing in original PR for this issue
* remove test related to private_key init param
* use context manager to auto-close socket listener so tests can be re-run

(cherry picked from commit 0790ede)
ashb pushed a commit to ashb/airflow that referenced this pull request Oct 18, 2019
@dstandish @ashb
[AIRFLOW-4574] SSHHook private_key may only be supplied in extras (ap…
7e9075f 
…ache#6163)

* discussion on original PR suggested removing private_key option as init param
* with this PR, can still provide through extras, but not as init param
* also add support for private_key in tunnel -- missing in original PR for this issue
* remove test related to private_key init param
* use context manager to auto-close socket listener so tests can be re-run

(cherry picked from commit 0790ede)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mik-laj mik-laj mik-laj left review comments

@ashb ashb ashb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dstandish @codecov-io @mik-laj @ashb