jason810496 commented on code in PR #54582:
URL: https://github.com/apache/airflow/pull/54582#discussion_r2280771868
##########
providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -688,9 +688,10 @@ def account_id(self) -> str:
def get_session(self, region_name: str | None = None, deferrable: bool =
False) -> boto3.session.Session:
"""Get the underlying
boto3.session.Session(region_name=region_name)."""
- return SessionFactory(
- conn=self.conn_config, region_name=region_name, config=self.config
- ).create_session(deferrable=deferrable)
+ config = self._config or botocore.config.Config()
+ return SessionFactory(conn=self.conn_config, region_name=region_name,
config=config).create_session(
Review Comment:
May I ask why do we need this change?
Since `AwsGenericHook.get_session` will get from
`conn_config.botocore_config or botocore.config.Config()
`.
https://github.com/apache/airflow/blob/6da99f245634029d4c62a0b1d66f053b2d23637e/providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py#L678-L681
Then the `AwsGenericHook.conn_config` property will return
`AwsConnectionWrapper`.
https://github.com/apache/airflow/blob/6da99f245634029d4c62a0b1d66f053b2d23637e/providers/amazon/src/airflow/providers/amazon/aws/hooks/base_aws.py#L617-L643
The `__post_init__` method of `AwsConnectionWrapper` will ensure
initializing the `AwsConnectionWrapper.botocore_config` property.
https://github.com/apache/airflow/blob/6da99f245634029d4c62a0b1d66f053b2d23637e/providers/amazon/src/airflow/providers/amazon/aws/utils/connection_wrapper.py#L244-L249
IMO, the `AwsGenericHook.config` will not be `None` and will be initialized
properly.
##########
providers/amazon/src/airflow/providers/amazon/aws/utils/eks_get_token.py:
##########
@@ -51,9 +53,16 @@ def get_parser():
return parser
+def _ensure_db_session():
+ if not getattr(settings, "engine", None) or not getattr(settings,
"Session", None):
+ configure_orm()
Review Comment:
No strong opinion for the naming, both work with me.
+1 for having the docstring.
##########
providers/amazon/src/airflow/providers/amazon/aws/utils/eks_get_token.py:
##########
@@ -51,9 +53,16 @@ def get_parser():
return parser
+def _ensure_db_session():
+ if not getattr(settings, "engine", None) or not getattr(settings,
"Session", None):
Review Comment:
Having check on `engine` would be enough.
FYI:
https://github.com/apache/airflow/blob/6da99f245634029d4c62a0b1d66f053b2d23637e/airflow-core/src/airflow/settings.py#L420-L421
--
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]
