Add description field to connection (#10840) #10873

abhilash1in · 2020-09-11T00:17:15Z

Summary:

Add a description (string) field to store additional details about a connection.

UI:

CLI:

(airflow) abhilash1in@Abhilashs-MBP airflow % airflow connections add --conn-description "test description" --conn-type mysql --conn-host host --conn-login user --conn-password password test_conn

        Successfully added `conn_id`=test_conn : mysql://user:******@host:

(airflow) abhilash1in@Abhilashs-MBP airflow % airflow connections get test_conn
[2020-09-10 17:02:42,279] {base_hook.py:67} INFO - Using connection to: id: test_conn. Host: host, Port: None, Schema: None, Login: user, Password: XXXXXXXX, extra: None
Id: 48
Conn Id: test_conn
Conn Type: mysql
Description: test description
Host: host
Schema: null
Login: user
Password: password
Port: null
Is Encrypted: true
Is Extra Encrypted: true
Extra: {}
URI: mysql://user:password@host

related: #10840

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

turbaszek · 2020-09-13T19:43:47Z

airflow/models/connection.py

Can you explain it?

Rule R0913 of pylint expected functions and methods to have utmost 10 arguments. The __init__ method of Connection class already had 10 arguments. I added description which made it 11. So, had to disable rule R0913 and I followed the convention here.

Would it be better to replace # pylint: disable=R0913 with #pylint: disable=too-many-arguments and add it at the end of the line class Connection(Base, LoggingMixin): instead of disabling the rule for the whole file?

Would it be better to replace # pylint: disable=R0913 with #pylint: disable=too-many-arguments and add it at the end of the line class Connection(Base, LoggingMixin):

That would be preferable as others will know what exactly is disabled

Done :) Added #pylint: disable=too-many-arguments to __init__ method of Connection class.

turbaszek · 2020-09-13T19:44:19Z

For me this looks good just one comment 👌

abhilash1in · 2020-09-18T02:21:54Z

@kaxil waiting for your review 🙂

abhilash1in · 2020-09-18T02:24:57Z

Can this be added to Airflow 1.10.13 milestone?

potiuk · 2020-09-18T10:23:31Z

@abhilash1in -> this one looks like more of a convenience than an important change and we actually only cherry-pick to 1.10* what is absolutely necessary. We are getting closer to 2.0 release and we also need to have some "nice features" in 2.0 to get people more excited to migrate to it ;). Also we have two versions of the UI in 1.10 and if we add it to the DB we would have to also add it to both UIs. Plus we also have connection retrieval from the secret backends, so I guess @kaxil's review will point it out that we will have to add similar capabilities to secret backends.

I think it's far too little gain and far too much complexity to try to cherry-pick that one.

abhilash1in · 2020-09-18T18:55:31Z

@potiuk that makes sense, thank you. I'm okay with this being added to 2.0.

kaxil

Tests are failing

abhilash1in · 2020-09-19T01:34:24Z

Tests are failing

@kaxil There were two alembic heads since a new migration was pushed to master after I created my migration (both had the same down_revision). Tests were failing because of the multiple heads.

I've added a new commit to merge the alembic heads and rebased my fork to be in sync with master.

kaxil · 2020-09-19T01:35:33Z

airflow/migrations/versions/c2e6d7e1c6d2_merge_61ec_and_e1a.py

Let's update the other migration in place since this PR is not merged instead of merging the heads

Done. Waiting for the tests to finish running.

@kaxil some checks were skipped although nothing else failed, not sure why.

@kaxil can you please review? Would be great if we can close this soon.

mik-laj · 2020-09-20T09:57:08Z

Plus we also have connection retrieval from the secret backends, so I guess @kaxil's review will point it out that we will have to add similar capabilities to secret backends.

Descriptions are only useful in the Web UI, and connections from the secret backends are not visible in Airflow, so it is not necessary to add support for this field in the backend secret.

potiuk · 2020-09-20T19:40:14Z

Descriptions are only useful in the Web UI, and connections from the secret backends are not visible in Airflow, so it is not necessary to add support for this field in the backend secret.

Recently there was a discussion about showing the "names" but not "values" of the connections - so that we can see the connections in the UI even if they are coming from the secret backend. I think for 2.0 we should consciously decide what we do with the connection coming from the secrets and potentially show them in the UI. Otherwise it might be very misleading - the user sees a different "reality" than is "viewed" by the workers.

I believe if we want to add a new "property" of connection - it is very much related to what we are going to do with displaying them in UI (including secret backends)

WDYT @mik-laj - is it good that we do not see secret backend connection, and when we have the connection in db, we see a wrong one ? I think it's rather bad and might be super-confusing for the user.

That's why I think we should first decide what we want to do with the connections and later think about adding new properties to them.

mik-laj · 2020-09-20T20:36:17Z

@potiuk I'm eager to talk about the mailing list, but in my opinion displaying the item list is a significant extension of the backend secret concept. During its development, the topic of the web UI was discussed and it was decided that the backend secret is just a single interface with the get(key) method. We have also decided that we do not need to display these elements in the Web UI, as users can learn about these keys from their administrator.

WDYT @mik-laj - is it good that we do not see secret backend connection, and when we have the connection in db, we see a wrong one ? I think it's rather bad and might be super-confusing for the user.

This is confusing, and I agree that it is worth warning users about the consequences of configuring the backend secret. I think the warning "Your administrator has set up a secret backend. The data displayed in the Web UI may not represent the state available for the workers". I am afraid that reading any of the values by the webserver can be problematic in corporate environments as corporate environments have a very strict secret access policy. The most common environments I take care of are deployed consisting of several GCP projects. The webserver is in a project that cannot activate the API or access it at all.

Recently, I even had to deal with a issues related to it. In one case, the webserver still tried to make connections to the backend secret, but it never succeeded.
#10365

prakshalj0512 · 2020-09-20T22:54:33Z

@abhilash1in It'd be great if you could add this same field for Variables.

abhilash1in · 2020-10-04T04:40:45Z

@turbaszek @kaxil @potiuk @mik-laj Can someone please review and close this PR?

mik-laj · 2020-10-04T11:58:31Z

@kaxil do you think this PR requires changes to the secret backend? as I think the backends are working now, the additional columns have no effect and there is no obstacle to merging this change.

kaxil · 2020-10-11T20:43:32Z

@kaxil do you think this PR requires changes to the secret backend? as I think the backends are working now, the additional columns have no effect and there is no obstacle to merging this change.

Yup, it will continue to work

kaxil · 2020-10-11T20:45:42Z

Rebased the PR to the latest master, will merge as soon as CI is green

mik-laj · 2020-10-12T09:08:45Z

One tests failed:

FAILED tests/utils/test_db.py::TestDb::test_only_single_head_revision_in_migrations

abhilash1in · 2020-10-12T16:07:40Z

Rebased to the latest master.

mik-laj · 2020-10-13T15:17:04Z

I also rebased to latest master.

github-actions · 2020-10-13T18:56:20Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

kaxil · 2020-10-13T20:06:13Z

airflow/migrations/versions/61ec73d9401f_add_description_field_to_connection.py

Suggested change

Revises: bef4f3d11e8b

Revises: 98271e7606e2

kurtqq · 2020-11-17T18:29:42Z

This will be very helpful!
@abhilash1in any chance you can finalize?

mik-laj · 2020-11-17T20:37:10Z

@abhilash1in Can you do a rebase?

abhilash1in · 2020-11-17T20:38:34Z

@abhilash1in Can you do a rebase?

On it. Resolving merge conflicts. Should be done soon.

kaxil · 2020-11-17T21:10:49Z

You might also have to update alembic revision ids

github-actions · 2020-11-17T21:40:43Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

abhilash1in · 2020-11-18T00:38:02Z

@kaxil @mik-laj can you please take a look at the failing workflow? Not sure why there is an error Error: read ECONNRESET in Upload artifact for coverage step.

mik-laj · 2020-11-18T03:10:00Z

@abhilash1in Please ignore it. This is just a transistent problem. This should be fixed soon.

abhilash1in · 2020-11-18T07:22:52Z

@kaxil @mik-laj All checks passed. Rebased again to latest master. Waiting for checks to run again. Please feel free to rebase again if required and please merge this PR as early as possible.

boring-cyborg bot added area:CLI area:webserver Webserver related Issues labels Sep 11, 2020

abhilash1in marked this pull request as ready for review September 11, 2020 05:39

turbaszek reviewed Sep 13, 2020

View reviewed changes

turbaszek approved these changes Sep 14, 2020

View reviewed changes

turbaszek requested a review from kaxil September 14, 2020 06:52

kaxil approved these changes Sep 19, 2020

View reviewed changes

kaxil requested changes Sep 19, 2020

View reviewed changes

kaxil reviewed Sep 19, 2020

View reviewed changes

turbaszek requested a review from kaxil September 19, 2020 09:36

kaxil approved these changes Oct 11, 2020

View reviewed changes

kaxil reviewed Oct 13, 2020

View reviewed changes

mik-laj approved these changes Nov 17, 2020

View reviewed changes

Add description field to connection (#10840)

cea3d1a

kaxil merged commit bf6da16 into apache:master Nov 18, 2020

abhilash1in deleted the add-connection-description branch November 18, 2020 17:51

ashb mentioned this pull request Nov 24, 2020

Connection description migration breaks on MySQL 8 #12588

Closed

Add description field to connection (#10840) #10873

Add description field to connection (#10840) #10873

Uh oh!

Conversation

abhilash1in commented Sep 11, 2020

Summary:

UI:

CLI:

related: #10840

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turbaszek Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turbaszek commented Sep 13, 2020

Uh oh!

abhilash1in commented Sep 18, 2020

Uh oh!

abhilash1in commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhilash1in commented Sep 18, 2020

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

abhilash1in commented Sep 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj commented Sep 20, 2020

Uh oh!

potiuk commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mik-laj commented Sep 20, 2020

Uh oh!

prakshalj0512 commented Sep 20, 2020

Uh oh!

abhilash1in commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mik-laj commented Oct 4, 2020

Uh oh!

kaxil commented Oct 11, 2020

Uh oh!

kaxil commented Oct 11, 2020

Uh oh!

mik-laj commented Oct 12, 2020

Uh oh!

abhilash1in commented Oct 12, 2020

Uh oh!

mik-laj commented Oct 13, 2020

Uh oh!

github-actions bot commented Oct 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurtqq commented Nov 17, 2020

Uh oh!

mik-laj commented Nov 17, 2020

Uh oh!

abhilash1in commented Nov 17, 2020

Uh oh!

kaxil commented Nov 17, 2020

turbaszek Sep 13, 2020 •

edited

Loading

abhilash1in commented Sep 18, 2020 •

edited

Loading

potiuk commented Sep 18, 2020 •

edited

Loading

potiuk commented Sep 20, 2020 •

edited

Loading

abhilash1in commented Oct 4, 2020 •

edited

Loading

abhilash1in commented Nov 18, 2020 •

edited

Loading