Skip to content

[AIRFLOW-1700] Support conn_type overwrite when url scheme is invalid#2682

Closed
fenglu-db wants to merge 2 commits into
apache:masterfrom
fenglu-db:master
Closed

[AIRFLOW-1700] Support conn_type overwrite when url scheme is invalid#2682
fenglu-db wants to merge 2 commits into
apache:masterfrom
fenglu-db:master

Conversation

@fenglu-db

Copy link
Copy Markdown
Contributor

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

https://issues.apache.org/jira/browse/AIRFLOW-1700

Description

When creating a new connection via airflow cli, the connection type is inferred from the conn-uri argument (i.e., conn_type = url scheme). However, for connection types like "hive_cli" and "google_cloud_platform", urlparse (by design) was unable to get the connection type as '_' is not a valid scheme character.

Tests

  • updated tests.core:CliTests.test_cli_connections_add_delete and local unit test passed without any issue.

@fenglu-db

Copy link
Copy Markdown
Contributor Author

@criccomini PTAL and can we also get this into 1.9.0-alpha1?
Thank you.

@mrkm4ntr

Copy link
Copy Markdown
Contributor

Sorry, this duplicates with #2525.

@codecov-io

codecov-io commented Oct 11, 2017

Copy link
Copy Markdown

Codecov Report

Merging #2682 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2682      +/-   ##
==========================================
+ Coverage   71.67%   71.68%   +<.01%     
==========================================
  Files         154      154              
  Lines       11814    11814              
==========================================
+ Hits         8468     8469       +1     
+ Misses       3346     3345       -1
Impacted Files Coverage Δ
airflow/bin/cli.py 53.45% <100%> (ø) ⬆️
airflow/models.py 87.14% <100%> (+0.04%) ⬆️

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 8512776...b18a5bc. Read the comment docs.

@fenglu-db

Copy link
Copy Markdown
Contributor Author

Ack, missed that when I searched open issues re:conn_type. Thanks for the link.

@mrkm4ntr @criccomini
I like the ability to parse connection details from URI, it unfortunately breaks when scheme contains '_'. Instead of making every connection field accept cmd args (as in #2525), shall we just make conn-type overwritable? My concern is that It might be confusing when conn details can be set in multiple ways. I can also revise my PR to take conn_type only when scheme is None. IOW, conn_uri takes precedence over conn-type. LMK.

@mrkm4ntr

Copy link
Copy Markdown
Contributor

@fenglu-g In my first commit, it was a fix like your suggestion to only add conn_type and overwrite it only if the schema is None. However, as discussed here, I changed to the current fix because it is not preferable the schema of URI to be None.
I think that it is better to be able to give conn_uri which does not cause an error I think that it is better to be able to give conn_uri which does not cause an error even if contains _, or to give the same parameter as when registering from UI., or to give the same parameter as when registering from UI.

Anyway, if we continue discussion any further, I think I should do it here.

@fenglu-db
fenglu-db force-pushed the master branch 2 times, most recently from ce5ccc7 to 157ee7f Compare October 17, 2017 16:00
@fenglu-db

Copy link
Copy Markdown
Contributor Author

The discussion is moved to #2525, closing this PR now.

@fenglu-db fenglu-db closed this Oct 17, 2017
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.

3 participants