uranusjr commented on a change in pull request #17448:
URL: https://github.com/apache/airflow/pull/17448#discussion_r699094690
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -75,49 +80,100 @@ def __init__(
config_prefix: str = 'airflow/config',
profile_name: Optional[str] = None,
sep: str = "/",
+ full_url_mode: bool = True,
**kwargs,
):
super().__init__()
if connections_prefix is not None:
- self.connections_prefix = connections_prefix.rstrip("/")
+ self.connections_prefix = connections_prefix.rstrip(sep)
else:
self.connections_prefix = connections_prefix
if variables_prefix is not None:
- self.variables_prefix = variables_prefix.rstrip('/')
+ self.variables_prefix = variables_prefix.rstrip(sep)
else:
self.variables_prefix = variables_prefix
if config_prefix is not None:
- self.config_prefix = config_prefix.rstrip('/')
+ self.config_prefix = config_prefix.rstrip(sep)
else:
self.config_prefix = config_prefix
self.profile_name = profile_name
self.sep = sep
+ self.full_url_mode = full_url_mode
self.kwargs = kwargs
@cached_property
def client(self):
"""Create a Secrets Manager client"""
- session = boto3.session.Session(
- profile_name=self.profile_name,
- )
+ session = boto3.session.Session(profile_name=self.profile_name)
+
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = secret['extra']
+ conn_string = f"{conn_string}?{urlencode(extra_dict)}"
+ return conn_string
+
+ return conn_string
Review comment:
```suggestion
def _get_extra(self, secret, conn_string):
try:
extra_dict = secret['extra']
except KeyError:
return conn_string
conn_string = f"{conn_string}?{urlencode(extra_dict)}"
return conn_string
```
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -75,49 +80,100 @@ def __init__(
config_prefix: str = 'airflow/config',
profile_name: Optional[str] = None,
sep: str = "/",
+ full_url_mode: bool = True,
**kwargs,
):
super().__init__()
if connections_prefix is not None:
- self.connections_prefix = connections_prefix.rstrip("/")
+ self.connections_prefix = connections_prefix.rstrip(sep)
else:
self.connections_prefix = connections_prefix
if variables_prefix is not None:
- self.variables_prefix = variables_prefix.rstrip('/')
+ self.variables_prefix = variables_prefix.rstrip(sep)
else:
self.variables_prefix = variables_prefix
if config_prefix is not None:
- self.config_prefix = config_prefix.rstrip('/')
+ self.config_prefix = config_prefix.rstrip(sep)
else:
self.config_prefix = config_prefix
self.profile_name = profile_name
self.sep = sep
+ self.full_url_mode = full_url_mode
self.kwargs = kwargs
@cached_property
def client(self):
"""Create a Secrets Manager client"""
- session = boto3.session.Session(
- profile_name=self.profile_name,
- )
+ session = boto3.session.Session(profile_name=self.profile_name)
+
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = secret['extra']
+ conn_string = f"{conn_string}?{urlencode(extra_dict)}"
+ return conn_string
+
+ return conn_string
+
+ def get_uri_from_secret(self, secret):
+ possible_words_for_conn_fields = {
+ 'user': ['user', 'username', 'login', 'user_name'],
+ 'password': ['password', 'pass', 'key'],
+ 'host': ['host', 'remote_host', 'server'],
+ 'port': ['port'],
+ 'schema': ['database', 'schema'],
+ 'conn_type': ['conn_type', 'conn_id', 'connection_type', 'engine'],
+ }
Review comment:
Should we provide a mechanism for users to specify a key not in this
list?
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -75,49 +80,100 @@ def __init__(
config_prefix: str = 'airflow/config',
profile_name: Optional[str] = None,
sep: str = "/",
+ full_url_mode: bool = True,
**kwargs,
):
super().__init__()
if connections_prefix is not None:
- self.connections_prefix = connections_prefix.rstrip("/")
+ self.connections_prefix = connections_prefix.rstrip(sep)
else:
self.connections_prefix = connections_prefix
if variables_prefix is not None:
- self.variables_prefix = variables_prefix.rstrip('/')
+ self.variables_prefix = variables_prefix.rstrip(sep)
else:
self.variables_prefix = variables_prefix
if config_prefix is not None:
- self.config_prefix = config_prefix.rstrip('/')
+ self.config_prefix = config_prefix.rstrip(sep)
else:
self.config_prefix = config_prefix
self.profile_name = profile_name
self.sep = sep
+ self.full_url_mode = full_url_mode
self.kwargs = kwargs
@cached_property
def client(self):
"""Create a Secrets Manager client"""
- session = boto3.session.Session(
- profile_name=self.profile_name,
- )
+ session = boto3.session.Session(profile_name=self.profile_name)
+
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = secret['extra']
+ conn_string = f"{conn_string}?{urlencode(extra_dict)}"
+ return conn_string
+
+ return conn_string
Review comment:
```suggestion
def _format_uri_with_extra(self, secret, conn_string):
try:
extra_dict = secret['extra']
except KeyError:
return conn_string
conn_string = f"{conn_string}?{urlencode(extra_dict)}"
return conn_string
```
Or this should do what the function name says and _just_ get extra, and
leave the formatting part to `get_uri_from_secret`:
```suggestion
def _get_encoded_extra(self, secret):
try:
extra_dict = secret['extra']
except KeyError:
return None
return urlencode(extra_dict)
```
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -75,49 +80,100 @@ def __init__(
config_prefix: str = 'airflow/config',
profile_name: Optional[str] = None,
sep: str = "/",
+ full_url_mode: bool = True,
**kwargs,
):
super().__init__()
if connections_prefix is not None:
- self.connections_prefix = connections_prefix.rstrip("/")
+ self.connections_prefix = connections_prefix.rstrip(sep)
else:
self.connections_prefix = connections_prefix
if variables_prefix is not None:
- self.variables_prefix = variables_prefix.rstrip('/')
+ self.variables_prefix = variables_prefix.rstrip(sep)
else:
self.variables_prefix = variables_prefix
if config_prefix is not None:
- self.config_prefix = config_prefix.rstrip('/')
+ self.config_prefix = config_prefix.rstrip(sep)
else:
self.config_prefix = config_prefix
self.profile_name = profile_name
self.sep = sep
+ self.full_url_mode = full_url_mode
self.kwargs = kwargs
@cached_property
def client(self):
"""Create a Secrets Manager client"""
- session = boto3.session.Session(
- profile_name=self.profile_name,
- )
+ session = boto3.session.Session(profile_name=self.profile_name)
+
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = secret['extra']
+ conn_string = f"{conn_string}?{urlencode(extra_dict)}"
+ return conn_string
+
+ return conn_string
+
+ def get_uri_from_secret(self, secret):
+ possible_words_for_conn_fields = {
+ 'user': ['user', 'username', 'login', 'user_name'],
+ 'password': ['password', 'pass', 'key'],
+ 'host': ['host', 'remote_host', 'server'],
+ 'port': ['port'],
+ 'schema': ['database', 'schema'],
+ 'conn_type': ['conn_type', 'conn_id', 'connection_type', 'engine'],
+ }
+
+ conn_d = {}
+ for conn_field, possible_words in
possible_words_for_conn_fields.items():
+ try:
+ conn_d[conn_field] = [v for k, v in secret.items() if k in
possible_words][0]
+ except IndexError:
+ conn_d[conn_field] = ''
+
+ conn_type = conn_d['conn_type']
+ user = conn_d['user']
+ password = conn_d['password']
+ host = conn_d['host']
+ port = conn_d['port']
+ schema = conn_d['schema']
+ conn_string = f'{conn_type}://{user}:{password}@{host}:{port}/{schema}'
Review comment:
This is actually one of the situations where `str.format` works better:
```suggestion
conn_string =
f'{conn_type}://{user}:{password}@{host}:{port}/{schema}'.format(**conn_d)
```
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -75,49 +80,100 @@ def __init__(
config_prefix: str = 'airflow/config',
profile_name: Optional[str] = None,
sep: str = "/",
+ full_url_mode: bool = True,
**kwargs,
):
super().__init__()
if connections_prefix is not None:
- self.connections_prefix = connections_prefix.rstrip("/")
+ self.connections_prefix = connections_prefix.rstrip(sep)
else:
self.connections_prefix = connections_prefix
if variables_prefix is not None:
- self.variables_prefix = variables_prefix.rstrip('/')
+ self.variables_prefix = variables_prefix.rstrip(sep)
else:
self.variables_prefix = variables_prefix
if config_prefix is not None:
- self.config_prefix = config_prefix.rstrip('/')
+ self.config_prefix = config_prefix.rstrip(sep)
else:
self.config_prefix = config_prefix
self.profile_name = profile_name
self.sep = sep
+ self.full_url_mode = full_url_mode
self.kwargs = kwargs
@cached_property
def client(self):
"""Create a Secrets Manager client"""
- session = boto3.session.Session(
- profile_name=self.profile_name,
- )
+ session = boto3.session.Session(profile_name=self.profile_name)
+
return session.client(service_name="secretsmanager", **self.kwargs)
- def get_conn_uri(self, conn_id: str) -> Optional[str]:
+ def _get_extra(self, secret, conn_string):
+ if 'extra' in secret:
+ extra_dict = secret['extra']
+ conn_string = f"{conn_string}?{urlencode(extra_dict)}"
+ return conn_string
+
+ return conn_string
+
+ def get_uri_from_secret(self, secret):
+ possible_words_for_conn_fields = {
+ 'user': ['user', 'username', 'login', 'user_name'],
+ 'password': ['password', 'pass', 'key'],
+ 'host': ['host', 'remote_host', 'server'],
+ 'port': ['port'],
+ 'schema': ['database', 'schema'],
+ 'conn_type': ['conn_type', 'conn_id', 'connection_type', 'engine'],
+ }
+
+ conn_d = {}
+ for conn_field, possible_words in
possible_words_for_conn_fields.items():
+ try:
+ conn_d[conn_field] = [v for k, v in secret.items() if k in
possible_words][0]
+ except IndexError:
+ conn_d[conn_field] = ''
+
+ conn_type = conn_d['conn_type']
+ user = conn_d['user']
+ password = conn_d['password']
+ host = conn_d['host']
+ port = conn_d['port']
+ schema = conn_d['schema']
+ conn_string = f'{conn_type}://{user}:{password}@{host}:{port}/{schema}'
Review comment:
This is actually one of the situations where `str.format` works better:
```suggestion
conn_string =
"{conn_type}://{user}:{password}@{host}:{port}/{schema}".format(**conn_d)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
