[AIRFLOW-2840] - add update connections cli option #3684

caddac · 2018-08-02T14:27:48Z

Make sure you have checked all steps below.

Jira

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-2840

Description

Here are some details about my PR, including screenshots of any UI changes:
Adds an Update Connection CLI option. This is useful for updating connections in a CI/CD environment where deleting and adding connections could cause DAGS to fail.

Tests

My PR adds the following unit tests:
My PR adds a update connection step to an existing unit test in test/core.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":
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

codecov-io · 2018-08-02T15:15:37Z

Codecov Report

Merging #3684 into master will increase coverage by 0.82%.
The diff coverage is 67.39%.

@@            Coverage Diff            @@
##           master   #3684      +/-   ##
=========================================
+ Coverage   76.68%   77.5%   +0.82%     
=========================================
  Files         199     205       +6     
  Lines       16189   15817     -372     
=========================================
- Hits        12414   12259     -155     
+ Misses       3775    3558     -217

Impacted Files	Coverage Δ
airflow/bin/cli.py	`64.52% <67.39%> (-0.31%)`	⬇️
airflow/operators/slack_operator.py	`0% <0%> (-97.37%)`	⬇️
airflow/sensors/s3_key_sensor.py	`31.03% <0%> (-68.97%)`	⬇️
airflow/sensors/s3_prefix_sensor.py	`41.17% <0%> (-58.83%)`	⬇️
airflow/utils/helpers.py	`67.07% <0%> (-17.31%)`	⬇️
airflow/example_dags/example_python_operator.py	`78.94% <0%> (-15.79%)`	⬇️
airflow/hooks/mysql_hook.py	`78% <0%> (-12.39%)`	⬇️
airflow/sensors/sql_sensor.py	`90.47% <0%> (-9.53%)`	⬇️
airflow/utils/sqlalchemy.py	`73.91% <0%> (-7.52%)`	⬇️
airflow/configuration.py	`83.95% <0%> (-5.47%)`	⬇️
... and 96 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 64b94d8...757c1b5. Read the comment docs.

ashb

connection ID is (by design) non-unique - its a mechanism of load balancing across multiple DBs for instance - you can have three conns with an ID of "mysql" and Airflow will pick a random one each time.

Can you explicitly handle/detect this case (probably by refusing to update any of them?) and add tests that cover it too? Given it's relatively infrequent to do this I think it is okay to just refuse to update any of the conns in this case.

caddac · 2018-08-04T22:55:15Z

Would updating all of the connections be acceptable? Maybe with a flag to indicate this behavior? Or is refusing to update preferable?

caddac · 2018-08-04T23:12:13Z

I've also noticed the cli option for adding connections doesn't support adding connections with duplicate conn_id, should it?

bolkedebruin · 2018-08-05T07:22:55Z

Please move the core of the code out of the cli (I.e. split between api/Commons and changes to the connection model) and make sure it works through the REST API as well. Direct access to the database should be limited in the cli as we will move away from this in the future.

bolkedebruin · 2018-08-05T11:43:40Z

O sorry @ashb I think I accidentally removed the label

ashb · 2018-08-05T12:45:36Z

@bolkedeburin no worries on the label, I was just using them as a way of tracking in the PRs page which ones we've looked at - entirely manual for now.

I think the default should refuse to update multiple, with an --update-all flag

caddac · 2018-08-12T15:48:12Z

@ashb @bolkedebruin I've been working on refactoring the connection api and wanted to run the implementation by you before going too far. My thought is in api/common/experimental/ there will be 4 files: add_connection.py, list_connection.py, delete_connection.py, update_connection.py where the DB access will occur. These functions will take explicitly defined arguments based on what they are doing (not dicts or something) and return Connections (or in the case of delete, conn_id(s)).

Then each client (cli, local, json) would be responsible for parsing arguments and constructing meaningful print/return statements to the user (e.g. the cli client would formulate the message Successfully added conn_id={conn_id} : {uri} where as the json client would just return http 200 and the connection as json).

thoughts?

ashb · 2018-08-14T08:39:06Z

Something like that sounds sensible - removing duplication is good. Don't forget about the Flask web UI(s - right now we still have two)

bolkedebruin · 2018-08-14T08:40:58Z

I wouldn't put the CRUD operations in separate files. Just use api/common/experimental/connections.py for simplicity

caddac · 2018-10-30T00:09:32Z

Can you give some feedback on the json_api? I'm not really sure what I should be doing there or how to test it. Thanks!

ashb

Haven't had time to give this a full re-review yet, sorry.

airflow/api/client/local_client.py

ron819 · 2018-11-03T10:25:44Z

Regarding @ashb first comment from Aug about multiple connections. There is also a bug to consider: https://issues.apache.org/jira/browse/AIRFLOW-2784 Your tests can't assume that all conn_id with same name are for the same conn type until this bug is fixed.

caddac · 2018-12-02T04:49:01Z

@ron819 I'm going to address this bug in my PR.

@ashb I'm still looking for some clarity on what is going on with the json_client. Even looking at the pool examples that currently exist there, I'm still not sure what exactly I need to do. Seems like some kind of http handling, but I assumed that was being handled by the www and www_rbac code. Any guidance you can give would be great.

caddac · 2018-12-03T18:28:41Z

@ashb Think I got the json_client figured out, will try to get my changes committed tonight.

ashb · 2018-12-04T09:13:42Z

Great! Sorry I didn’t respond - paid work has been busy :)

ashb

A few small changes/comments. Looking good overall.

One thing that wasn't clear from a read of the code: does the API return the passwords on list or not? (I don't think it should, as right now none of the current methods of viewing connection in the CLI or WebUI give the value of the password)

ashb · 2019-01-11T16:18:05Z

airflow/bin/cli.py

-            return
+        conns = api_client.list_connections()
+        # format it for the cli
+        conns = list(map(lambda c: (c['conn_id'], c['conn_type'],


Minor point: we don't need the list() here

ashb · 2019-01-11T16:18:33Z

airflow/bin/cli.py

+        try:
+            print(api_client.delete_connection(args.conn_id, args.delete_all))
+        except MissingArgument as ma:
+            print(ma)


We should exit with a Non-zero code here.

is raising the exception good enough? or is there a specific way to exit with non-zero code?

Raising the ma exception would work but would provide a messy output (i.e. it would include the stack trace which is not useful here):

https://github.com/apache/airflow/blob/9f3eae5e82aef4550027e5c3163715dad67bdb57/airflow/bin/cli.py#L1295-L1296

except MissingArgument as ma: raise SystemExit(ma)

Should probably do the trick (and not include any errant stack-traces. If that still includes a stack trace, or errors then SystemExit(str(ma)))

ashb · 2019-01-11T16:20:14Z

airflow/bin/cli.py

-                   '--add flag and --conn_uri flag: {invalid!r}\n')
-            msg = msg.format(invalid=invalid_args)
+        try:
+            new_conn = api_client.add_connection(


Might be easier to do new_conn = api_client.add_connection(vars(args))? (that does rely on the names from argparser matching the named args in the api client, but they do right now)

Not sure on this one.

ashb · 2019-01-11T16:20:28Z

airflow/bin/cli.py

+                conn_extra=args.conn_extra)
+
+        except MissingArgument as ma:
+            print(ma)


Non-zero exit code here too

tests/api/common/experimental/connection_tests.py

ashb · 2019-01-11T16:27:35Z

tests/api/common/experimental/connection_tests.py

+            password="airflow",
+            schema="airflow",
+            port="5432"
+        ))


From a quick look it appears that all the test cases use this connection - could we create it once in setUp() to reduce the amount of code in each test function?

tests/core.py

ashb · 2019-01-11T16:32:18Z

tests/api/common/experimental/connection_tests.py

+
+        session.query(models.Pool).delete()
+        session.query(models.Variable).delete()
+        session.query(models.Connection).delete()


This is causing your test failures:

AirflowException: The conn_id sqlite_default isn't defined

from elsewhere in the tests.

Adds an Update Connection CLI option.\nThis is useful for updating connections in a CI/CD environment\n where deleting and adding connections\n could cause DAGS to fail. \nAlso exposes connection api via rest

caddac · 2019-01-12T05:43:40Z

I see 5 tests failing, 1 of which is my test_delete_connection test. This test succeeds locally (on docker) for both python 2.7 and 3.6. The other tests also fail locally for me (on docker) from master. Not sure where to go from here, any suggestions @ashb ?

mik-laj · 2019-01-12T17:12:42Z

tests/core.py

+            "Successfully added `conn_id`=new3 : postgres://airflow:airflow@host:5432/airflow",
+            "Successfully added `conn_id`=new4 : postgres://airflow:airflow@host:5432/airflow",
+            "Successfully added `conn_id`=new5 : hive_metastore://airflow:airflow@host:9083/airflow",
+            "Successfully added `conn_id`=new6 : google_cloud_platform://:@:",


According to the "RFC 3986: Uniform Resource Identifier (URI): Generic Syntax" specification, this is not a valid address.
Schema part must be compatible with the format:

scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )

I created a issue https://issues.apache.org/jira/browse/AIRFLOW-3616 and working on PR: PolideaInternal#30
which deals with this topic.

mik-laj

Please improve compatibility with RFC 3986 Section 3.1

mik-laj · 2019-01-12T17:19:10Z

airflow/api/client/json_client.py

+        :return: The updated Connection
+        """
+        endpoint = '/api/experimental/connections/'
+        url = urljoin(self._api_base_url, endpoint)


This should be done in the _request method. Can you correct this or will we do it in a separate PR?

mik-laj · 2019-01-12T17:27:08Z

airflow/www/api/experimental/endpoints.py

+        _log.error(err)
+        response = jsonify(error="{}".format(err))
+        response.status_code = err.status_code
+        return response


This code snippet is repeated in any method. Should we think about how to avoid repeating this piece of code? Maybe introducing a decorator?

caddac · 2019-01-26T02:03:55Z

will reopen when working

ashb requested changes Aug 4, 2018

View reviewed changes

ashb added the needs-fixes label Aug 4, 2018

bolkedebruin removed the needs-fixes label Aug 5, 2018

feng-tao added the needs-fixes label Aug 7, 2018

ashb reviewed Nov 2, 2018

View reviewed changes

airflow/api/client/local_client.py Show resolved Hide resolved

ashb mentioned this pull request Dec 1, 2018

[AIRFLOW-3390] Add connections to api. #4232

Closed

6 tasks

bdronneau mentioned this pull request Jan 4, 2019

Fixes: remove admin connections before re-creating it MeilleursAgents/airflow-role#2

Merged

ashb requested changes Jan 11, 2019

View reviewed changes

ashb reviewed Jan 11, 2019

View reviewed changes

[AIRFLOW-2840] - add update connections cli option

5ffef07

Adds an Update Connection CLI option.\nThis is useful for updating connections in a CI/CD environment\n where deleting and adding connections\n could cause DAGS to fail. \nAlso exposes connection api via rest

mik-laj reviewed Jan 12, 2019

View reviewed changes

mik-laj requested changes Jan 12, 2019

View reviewed changes

mik-laj reviewed Jan 12, 2019

View reviewed changes

caddac closed this Jan 26, 2019

caddac mentioned this pull request Feb 9, 2019

[Airflow-2840] - add update connection cli option #4679

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-2840] - add update connections cli option #3684

[AIRFLOW-2840] - add update connections cli option #3684

caddac commented Aug 2, 2018

codecov-io commented Aug 2, 2018 •

edited

ashb left a comment

caddac commented Aug 4, 2018

caddac commented Aug 4, 2018

bolkedebruin commented Aug 5, 2018 •

edited

bolkedebruin commented Aug 5, 2018

ashb commented Aug 5, 2018

caddac commented Aug 12, 2018 •

edited

ashb commented Aug 14, 2018

bolkedebruin commented Aug 14, 2018 •

edited

caddac commented Oct 30, 2018 •

edited

ashb left a comment

ron819 commented Nov 3, 2018

caddac commented Dec 2, 2018

caddac commented Dec 3, 2018 •

edited

ashb commented Dec 4, 2018

ashb left a comment

ashb Jan 11, 2019

ashb Jan 11, 2019

caddac Jan 11, 2019

ashb Jan 11, 2019

ashb Jan 11, 2019

ashb Jan 11, 2019

ashb Jan 11, 2019

ashb Jan 11, 2019

caddac commented Jan 12, 2019

mik-laj Jan 12, 2019

mik-laj left a comment

mik-laj Jan 12, 2019

mik-laj Jan 12, 2019

caddac commented Jan 26, 2019

[AIRFLOW-2840] - add update connections cli option #3684

[AIRFLOW-2840] - add update connections cli option #3684

Conversation

caddac commented Aug 2, 2018

Jira

Description

Tests

Commits

Documentation

Code Quality

codecov-io commented Aug 2, 2018 • edited

Codecov Report

ashb left a comment

Choose a reason for hiding this comment

caddac commented Aug 4, 2018

caddac commented Aug 4, 2018

bolkedebruin commented Aug 5, 2018 • edited

bolkedebruin commented Aug 5, 2018

ashb commented Aug 5, 2018

caddac commented Aug 12, 2018 • edited

ashb commented Aug 14, 2018

bolkedebruin commented Aug 14, 2018 • edited

caddac commented Oct 30, 2018 • edited

ashb left a comment

Choose a reason for hiding this comment

ron819 commented Nov 3, 2018

caddac commented Dec 2, 2018

caddac commented Dec 3, 2018 • edited

ashb commented Dec 4, 2018

ashb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caddac commented Jan 12, 2019

Choose a reason for hiding this comment

mik-laj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caddac commented Jan 26, 2019

codecov-io commented Aug 2, 2018 •

edited

bolkedebruin commented Aug 5, 2018 •

edited

caddac commented Aug 12, 2018 •

edited

bolkedebruin commented Aug 14, 2018 •

edited

caddac commented Oct 30, 2018 •

edited

caddac commented Dec 3, 2018 •

edited