You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/02/18 13:43:03 UTC
[GitHub] [airflow] potiuk commented on a change in pull request #19857: Enable json serialization for secrets backend
potiuk commented on a change in pull request #19857:
URL: https://github.com/apache/airflow/pull/19857#discussion_r810003012
##########
File path: airflow/models/connection.py
##########
@@ -385,3 +389,17 @@ def get_connection_from_secrets(cls, conn_id: str) -> 'Connection':
)
raise AirflowNotFoundException(f"The conn_id `{conn_id}` isn't defined")
+
+ @classmethod
+ def from_json(cls, value, conn_id=None) -> 'Connection':
+ kwargs = json.loads(value)
+ extra = kwargs.pop('extra', None)
+ if extra:
+ kwargs['extra'] = extra if isinstance(extra, str) else json.dumps(extra)
+ conn_type = kwargs.pop('conn_type', None)
+ if conn_type:
+ kwargs['conn_type'] = cls._normalize_conn_type(conn_type)
+ port = kwargs.pop('port', None)
+ if port:
+ kwargs['port'] = int(port)
Review comment:
I think we should explicity catch ValueError here and provide a bit more meaningful message. Otherwise users will have dig in the code what really caused this mysterious ValueError raised in from_json() function
##########
File path: airflow/secrets/environment_variables.py
##########
@@ -30,8 +31,21 @@ class EnvironmentVariablesBackend(BaseSecretsBackend):
"""Retrieves Connection object and Variable from environment variable."""
def get_conn_uri(self, conn_id: str) -> Optional[str]:
- environment_uri = os.environ.get(CONN_ENV_PREFIX + conn_id.upper())
- return environment_uri
+ """
+ Return URI representation of Connection conn_id
+ :param conn_id: the connection id
+ :return: deserialized Connection
+ """
+ warnings.warn(
Review comment:
I tihnk we do not have to have the second warning here. It is already raised in "get_connection" when get_conn_uri is executed as fallback..
##########
File path: tests/cli/commands/test_connection_command.py
##########
@@ -308,35 +308,32 @@ def test_cli_connections_export_should_export_as_json(self, mock_file_open, mock
)
connection_command.connections_export(args)
- expected_connections = json.dumps(
- {
- "airflow_db": {
- "conn_type": "mysql",
- "description": "mysql conn description",
- "host": "mysql",
- "login": "root",
- "password": "plainpassword",
- "schema": "airflow",
- "port": None,
- "extra": None,
- },
- "druid_broker_default": {
- "conn_type": "druid",
- "description": "druid-broker conn description",
- "host": "druid-broker",
- "login": None,
- "password": None,
- "schema": None,
- "port": 8082,
- "extra": "{\"endpoint\": \"druid/v2/sql\"}",
- },
+ expected_connections = {
+ "airflow_db": {
+ "conn_type": "mysql",
+ "description": "mysql conn description",
+ "host": "mysql",
+ "login": "root",
+ "password": "plainpassword",
+ "schema": "airflow",
+ "port": None,
+ "extra": None,
},
- indent=2,
- )
+ "druid_broker_default": {
+ "conn_type": "druid",
+ "description": "druid-broker conn description",
+ "host": "druid-broker",
+ "login": None,
+ "password": None,
+ "schema": None,
+ "port": 8082,
+ "extra": "{\"endpoint\": \"druid/v2/sql\"}",
+ },
+ }
mock_splittext.assert_called_once()
mock_file_open.assert_called_once_with(output_filepath, 'w', -1, 'UTF-8', None)
- mock_file_open.return_value.write.assert_called_once_with(expected_connections)
+ assert json.loads(mock_file_open.return_value.write.call_args[0][0]) == expected_connections
Review comment:
Nice :). UX is also important for developers.
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -40,10 +47,46 @@ def build_path(path_prefix: str, secret_id: str, sep: str = "/") -> str:
"""
return f"{path_prefix}{sep}{secret_id}"
+ def get_conn_value(self, conn_id: str) -> Optional[str]:
+ """
+ Retrieve from Secrets Backend a string value representing the Connection object.
+
+ If the client your secrets backend uses already returns a python dict, you should override
+ ``get_connection`` instead.
+
+ :param conn_id: connection id
+ """
+ raise NotImplementedError
+
+ def _deserialize_connection(self, conn_id, value):
Review comment:
I think we should we make it `deserialize_connection` and explicity document it as overrideable by Secret Backend implementation. And add description on that to the Secret Backends. This is a great way to handle custom serialization methods if needed.
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -26,8 +26,15 @@
class BaseSecretsBackend(ABC):
"""Abstract base class to retrieve Connection object given a conn_id or Variable given a key"""
- def __init__(self, **kwargs):
- pass
+ def __init__(self, connection_serialization_format: Optional[str] = None, **kwargs):
+ from airflow.configuration import conf # must be local to avoid circular import
Review comment:
Oh yeah. I wish we spit conf and avoid those.
##########
File path: airflow/secrets/base_secrets.py
##########
@@ -52,15 +95,29 @@ def get_connection(self, conn_id: str) -> Optional['Connection']:
"""
Return connection object with a given ``conn_id``.
+ Tries ``get_conn_value`` first and if not implemented, tries ``get_conn_uri``
+
:param conn_id: connection id
"""
- from airflow.models.connection import Connection
-
- conn_uri = self.get_conn_uri(conn_id=conn_id)
- if not conn_uri:
+ value = None
+
+ # TODO: after removal of ``get_conn_uri`` we should not catch NotImplementedError here
+ with suppress(NotImplementedError):
+ value = self.get_conn_value(conn_id=conn_id)
Review comment:
Nice way of handling back-compatibility!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org