[GitHub] [airflow] dstandish commented on a change in pull request #15013: Enable Connection creation from Vault parameters

[GitBox <gi...@apache.org>]

dstandish commented on a change in pull request #15013:
URL: https://github.com/apache/airflow/pull/15013#discussion_r605388747



##########
File path: tests/providers/hashicorp/secrets/test_vault.py
##########
@@ -59,6 +59,48 @@ def test_get_conn_uri(self, mock_hvac):
         returned_uri = test_client.get_conn_uri(conn_id="test_postgres")
         assert 'postgresql://airflow:airflow@host:5432/airflow' == returned_uri
 
+    @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+    def test_get_connection(self, mock_hvac):
+        mock_client = mock.MagicMock()
+        mock_hvac.Client.return_value = mock_client
+        mock_client.secrets.kv.v2.read_secret_version.return_value = {
+            'request_id': '94011e25-f8dc-ec29-221b-1f9c1d9ad2ae',
+            'lease_id': '',
+            'renewable': False,
+            'lease_duration': 0,
+            'data': {
+                'data': {
+                    'conn_type': 'postgresql',
+                    'login': 'airflow',
+                    'password': 'airflow',
+                    'host': 'host',
+                    'port': '5432',
+                    'schema': 'airflow',
+                },

Review comment:
       probably should include `extra` since it's a case that should be supported
   
   one thought i had is, when storing creds in the "object" style, should we make it so that you can add values here at top level and they'll automatically get added to extra.
   
   so e.g. we could support this:
   ```
                   'data': {
                       'conn_type': 'postgresql',
                       'login': 'airflow',
                       'password': 'airflow',
                       'host': 'host',
                       'port': '5432',
                       'myfield1': 'hello',
                       'myfield2': 'hello again',
                       'schema': 'airflow',
                   },
   ```
   
   and then when bulding the connection object, add these to the extra dict.
   
   it's maybe a little wacky but it would allow for less nesting.  and then when defining the values in vault you wouldn't have to dump to json and store under `extra` as json string.  just a thought 🤷‍♂️
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org