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.

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

[AIRFLOW-3106] Validate Postgres connection after saving it #3941

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@BasPH
Copy link
Contributor

BasPH commented Sep 24, 2018

I want to create connection validation when saving a connection. I started with Postgres. Would be nice to know if you agree with the PR, before I implement validation for other connection types.

Jira

Description

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

This PR adds validation for the Postgres connection type. After saving a Connection, it validates by creating a new database connection and executing SELECT 1. In case this fails, an error is displayed:

image

Tests

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

Commits

  • 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

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff
[AIRFLOW-3106] Validate Postgres connection after saving it
Implemented connection validation for Postgres connections in order to
discover incorrect credentials or non-accessible systems from the
Airflow instance.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 24, 2018

Codecov Report

Merging #3941 into master will decrease coverage by 0.94%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3941      +/-   ##
==========================================
- Coverage   75.79%   74.85%   -0.95%     
==========================================
  Files         199      200       +1     
  Lines       15946    15982      +36     
==========================================
- Hits        12086    11963     -123     
- Misses       3860     4019     +159
Impacted Files Coverage Δ
airflow/hooks/postgres_hook.py 77.27% <12.5%> (-14.4%) ⬇️
airflow/www/views.py 68.66% <16.66%> (-0.2%) ⬇️
airflow/sensors/s3_prefix_sensor.py 0% <0%> (-100%) ⬇️
airflow/sensors/s3_key_sensor.py 30.3% <0%> (-69.7%) ⬇️
airflow/hooks/mysql_hook.py 58% <0%> (-32%) ⬇️
airflow/sensors/sql_sensor.py 90.47% <0%> (-9.53%) ⬇️
airflow/models.py 88.64% <0%> (-3.07%) ⬇️
airflow/hooks/http_hook.py 93.75% <0%> (-1.71%) ⬇️
airflow/utils/dag_processing.py 89.03% <0%> (-1.32%) ⬇️
airflow/jobs.py 82.13% <0%> (-0.27%) ⬇️
... and 11 more

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 b8be322...ce9e0f5. Read the comment docs.

@Fokko
Copy link
Contributor

Fokko left a comment

Awesome suggestion @BasPH

I really like this and makes setting up connections through the UI much easier. 2 points:

  • Can we also add this to the model hook? So we can generalize it also to other hooks than the Postgres one.
  • Can you add it to the rbac ui as well? The old UI will be removed soon (I hope).
@@ -60,6 +60,15 @@ def get_conn(self):
self.conn = psycopg2.connect(**conn_args)
return self.conn

def validate_conn(self):
try:
conn = self.get_conn()

This comment has been minimized.

@Fokko

Fokko Sep 24, 2018

Contributor

Can you change this to with self.get_conn()? So it closes automagically.

@ron819

This comment has been minimized.

Copy link
Contributor

ron819 commented Sep 25, 2018

@ashb

This comment has been minimized.

Copy link
Member

ashb commented Sep 25, 2018

Yup, duplicate Jira issues. One should be closed as a duplicate of the other.

@ron819

This comment has been minimized.

Copy link
Contributor

ron819 commented Sep 26, 2018

@ashb not sure. This PR offers solution only for PostgreSQL.

@ashb

This comment has been minimized.

Copy link
Member

ashb commented Sep 26, 2018

Oh yes, I was basing my comment on the title of the this Jira - I've changed that to be more specific and linked the two issues.

@BasPH

This comment has been minimized.

Copy link
Contributor

BasPH commented Sep 26, 2018

Thanks. Will see if I can process @Fokko 's comments tonight.

@ron819 ron819 referenced this pull request Nov 14, 2018

Closed

[AIRFLOW-1836] airflow uses OAuth Provider keycloak #2799

0 of 4 tasks complete
@stale

This comment has been minimized.

Copy link

stale bot commented Dec 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 10, 2018

@stale stale bot closed this Dec 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment