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/09/29 00:57:08 UTC
[GitHub] [airflow] dstandish opened a new pull request, #26765: Allow generation of connection URI to work when no conn type
dstandish opened a new pull request, #26765:
URL: https://github.com/apache/airflow/pull/26765
Previously if get_uri was called it would fail with `NoneType not iterable`, because of the check `if '-' in conn_type`.
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #26765:
URL: https://github.com/apache/airflow/pull/26765#discussion_r1059241664
##########
airflow/models/connection.py:
##########
@@ -206,13 +206,16 @@ def _parse_from_uri(self, uri: str):
def get_uri(self) -> str:
"""Return connection in URI format"""
- if "_" in self.conn_type:
+ if self.conn_type and "_" in self.conn_type:
self.log.warning(
"Connection schemes (type: %s) shall not contain '_' according to RFC3986.",
self.conn_type,
)
- uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
+ if self.conn_type:
+ uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
Review Comment:
sounds right
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26765:
URL: https://github.com/apache/airflow/pull/26765#discussion_r1059236954
##########
airflow/models/connection.py:
##########
@@ -206,13 +206,16 @@ def _parse_from_uri(self, uri: str):
def get_uri(self) -> str:
"""Return connection in URI format"""
- if "_" in self.conn_type:
+ if self.conn_type and "_" in self.conn_type:
self.log.warning(
"Connection schemes (type: %s) shall not contain '_' according to RFC3986.",
self.conn_type,
)
- uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
+ if self.conn_type:
+ uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
Review Comment:
Is this `str` cast still needed now this is only called when `self.conn_type` is not None?
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #26765:
URL: https://github.com/apache/airflow/pull/26765#discussion_r984540662
##########
tests/always/test_connection.py:
##########
@@ -737,3 +737,6 @@ def test_extra_warnings_non_json(self):
def test_extra_warnings_non_dict_json(self):
with pytest.warns(DeprecationWarning, match='not parse as a dictionary'):
Connection(conn_id='test_extra', conn_type='none', extra='"hi"')
+
+ def test_get_uri_no_conn_type(self):
+ assert Connection().get_uri() == 'none://'
Review Comment:
What's the usecase for this? Cos this doesn't seem like a useful URL, not to mention the column has this defined as `nullable=False`!
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #26765:
URL: https://github.com/apache/airflow/pull/26765#discussion_r1059241716
##########
airflow/models/connection.py:
##########
@@ -206,13 +206,16 @@ def _parse_from_uri(self, uri: str):
def get_uri(self) -> str:
"""Return connection in URI format"""
- if "_" in self.conn_type:
+ if self.conn_type and "_" in self.conn_type:
self.log.warning(
"Connection schemes (type: %s) shall not contain '_' according to RFC3986.",
self.conn_type,
)
- uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
+ if self.conn_type:
+ uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
Review Comment:
```suggestion
uri = f"{self.conn_type.lower().replace('_', '-')}://"
```
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #26765:
URL: https://github.com/apache/airflow/pull/26765#discussion_r984175671
##########
airflow/models/connection.py:
##########
@@ -206,13 +206,13 @@ def _parse_from_uri(self, uri: str):
def get_uri(self) -> str:
"""Return connection in URI format"""
- if '_' in self.conn_type:
+ if self.conn_type and '_' in self.conn_type:
self.log.warning(
"Connection schemes (type: %s) shall not contain '_' according to RFC3986.",
self.conn_type,
)
- uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
+ uri = f"{str(self.conn_type or 'none').lower().replace('_', '-')}://"
Review Comment:
yeah... almost... good point :) i think it was late and i had NoneType on the brain from the error
--
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
[GitHub] [airflow] dstandish merged pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
dstandish merged PR #26765:
URL: https://github.com/apache/airflow/pull/26765
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26765:
URL: https://github.com/apache/airflow/pull/26765#discussion_r982984699
##########
airflow/models/connection.py:
##########
@@ -206,13 +206,13 @@ def _parse_from_uri(self, uri: str):
def get_uri(self) -> str:
"""Return connection in URI format"""
- if '_' in self.conn_type:
+ if self.conn_type and '_' in self.conn_type:
self.log.warning(
"Connection schemes (type: %s) shall not contain '_' according to RFC3986.",
self.conn_type,
)
- uri = f"{str(self.conn_type).lower().replace('_', '-')}://"
+ uri = f"{str(self.conn_type or 'none').lower().replace('_', '-')}://"
Review Comment:
Technically the same? 😉
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #26765: Allow generation of connection URI to work when no conn type
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #26765:
URL: https://github.com/apache/airflow/pull/26765#discussion_r984748965
##########
tests/always/test_connection.py:
##########
@@ -737,3 +737,6 @@ def test_extra_warnings_non_json(self):
def test_extra_warnings_non_dict_json(self):
with pytest.warns(DeprecationWarning, match='not parse as a dictionary'):
Connection(conn_id='test_extra', conn_type='none', extra='"hi"')
+
+ def test_get_uri_no_conn_type(self):
+ assert Connection().get_uri() == 'none://'
Review Comment:
this is not a useful URI of course...
i could have written the test as `Connection(host='blah', login='blah').get_uri() == 'none://blah@blah'` and the objection would not apply. but, these other attrs have nothing to do with the test. we're merely verifying that the URI generation doesn't fail and `none` is the scheme.
the point here is, if you just want to generate a URI for an airflow connection, let's make it easy. airflow doesn't care whether your uri is `none://blah@blah` or `abc://blah@blah` -- it will still work when you set that conn in env vars or secrets backend.
--
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