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.

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-XXXX] Expose SQLAlchemy's connect_args and make it configurable #6478

Merged
merged 1 commit into from
Nov 1, 2019

Conversation

ZxMYS
Copy link
Contributor

@ZxMYS ZxMYS commented Oct 30, 2019

#6226 # Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
    I didn't create JIRA ticket

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    In our use case we need to configure SQLAlchemy's connect_args (e.g. we want to pass ssl.check_hostname=False to PyMySQL), and Airflow should expose this option and make it configurable.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    test_configure_orm_with_default_values, test_sql_alchemy_connect_args and test_sql_alchemy_invalid_connect_args in the new tests/test_sqlalchemy_config.py.

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":

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.

@ZxMYS ZxMYS changed the title [AIRFLOW-XXXX] Expose SqlAlchemy's connect_args and make it configurable [AIRFLOW-XXXX] Expose SQLAlchemy's connect_args and make it configurable Oct 30, 2019
@kaxil
Copy link
Member

kaxil commented Oct 31, 2019

This can already be done using db = create_engine('postgresql://scott:tiger@localhost/test?argument1=foo&argument2=bar')

Do you have any use case for this?

@ZxMYS
Copy link
Contributor Author

ZxMYS commented Oct 31, 2019

@kaxil user can not pass in a dict in connection string, which is needed if we want to set ssl = { check_hostname: False } for pymysql.
Passing arguments in connection string only works if SqlAlchemy recognizes the arguments and parses them properly - but unfortunately SqlAlchemy doesn't recognize some of the newer or more complex arguments, e.g. it supports ssl_ca, but not ssl_check_hostname.

@aoen
Copy link
Contributor

aoen commented Oct 31, 2019

Can you add your reply to Kaxil as a comment in the code for posterity?

I think we may want a test for this too. It would show how to use it, plus of course add test coverage : ). airflow/tests/test_logging_config.py is an example of how to do this.

@kaxil
Copy link
Member

kaxil commented Oct 31, 2019

Can you add your reply to Kaxil as a comment in the code for posterity?

I think we may want a test for this too. It would show how to use it, plus of course add test coverage : ). airflow/tests/test_logging_config.py is an example of how to do this.

Agree with Dan

@ZxMYS
Copy link
Contributor Author

ZxMYS commented Nov 1, 2019

ok, I updated description in default_airflow.cfg and added some simple unit test cases. Let me know if they are enough :)

@ZxMYS ZxMYS force-pushed the xzhu/connection_args branch 2 times, most recently from 8f53d84 to 7bd17b8 Compare November 1, 2019 01:47
@aoen
Copy link
Contributor

aoen commented Nov 1, 2019

@ZxMYS
Copy link
Contributor Author

ZxMYS commented Nov 1, 2019

Lint issues fixed! I did run ./breeze --static-check pylint -- --files tests/test_sqlalchemy_config.py, not sure why it didn't catch the issues appeared on CI

@ZxMYS ZxMYS force-pushed the xzhu/connection_args branch 2 times, most recently from e9d5765 to 813f182 Compare November 1, 2019 18:42
In many use cases users need to configure SQLAlchemy's connect_args (e.g. pass ssl.check_hostname=False to PyMySQL), and Airflow should expose this option and make it configurable.
@codecov-io
Copy link

Codecov Report

Merging #6478 into master will decrease coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6478      +/-   ##
==========================================
- Coverage   83.95%   83.64%   -0.31%     
==========================================
  Files         635      635              
  Lines       36657    36661       +4     
==========================================
- Hits        30774    30666     -108     
- Misses       5883     5995     +112
Impacted Files Coverage Δ
airflow/settings.py 88.81% <100%> (+0.32%) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/kube_client.py 33.33% <0%> (-41.67%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 70.14% <0%> (-28.36%) ⬇️
airflow/models/taskinstance.py 93.28% <0%> (-0.51%) ⬇️
airflow/utils/dag_processing.py 58.48% <0%> (+0.32%) ⬆️

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 f68c186...10c0829. Read the comment docs.

@aoen aoen merged commit d4a83bc into apache:master Nov 1, 2019
@aoen
Copy link
Contributor

aoen commented Nov 1, 2019

LGTM

@ZxMYS ZxMYS deleted the xzhu/connection_args branch November 1, 2019 21:17
@bhavaniravi
Copy link
Contributor

Is this feature available on 1.10.10?

kaxil pushed a commit that referenced this pull request Jul 7, 2020
…ble (#6478)

In many use cases users need to configure SQLAlchemy's connect_args (e.g. pass ssl.check_hostname=False to PyMySQL), and Airflow should expose this option and make it configurable.

(cherry picked from commit d4a83bc)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
In many use cases users need to configure SQLAlchemy's connect_args (e.g. pass ssl.check_hostname=False to PyMySQL), and Airflow should expose this option and make it configurable.

(cherry picked from commit d4a83bc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants