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 2021/11/24 20:36:57 UTC
[GitHub] [airflow] uranusjr opened a new pull request #19815: Amazon provider remove deprecation, second try
uranusjr opened a new pull request #19815:
URL: https://github.com/apache/airflow/pull/19815
The first commit is (squashed) from #19725, and the second on from #19789. Hopefully tests can run now...
--
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] dimon222 commented on a change in pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#discussion_r757049653
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -184,7 +184,13 @@ def get_iam_token(self, conn: Connection) -> Tuple[str, str, int]:
# Pull the custer-identifier from the beginning of the Redshift URL
# ex. my-cluster.ccdre4hpd39h.us-east-1.redshift.amazonaws.com returns my-cluster
cluster_identifier = conn.extra_dejson.get('cluster-identifier', conn.host.split('.')[0])
- client = aws_hook.get_client_type('redshift')
+ session, endpoint_url = aws_hook._get_credentials()
+ client = session.client(
+ "redshift",
+ endpoint_url=endpoint_url,
+ config=aws_hook.config,
+ verify=aws_hook.verify,
+ )
Review comment:
This changed the behavior for mocking
We could try change in `test_get_conn_rds_iam_redshift`
```
@mock.patch('airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook.get_client_type')
```
to
```
@mock.patch('airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook._get_credentials')
```
--
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 pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#issuecomment-978307336
cc @dimon222
--
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] dimon222 edited a comment on pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#issuecomment-979332442
Postgres test requires modification to skip mocking of get_client_type maybe? The error is weird.
>tests/providers/postgres/hooks/test_postgres.py::TestPostgresHookConn::test_get_conn_rds_iam_redshift: botocore.exceptions.ClientError: An error occurred (InvalidClientTokenId) when calling the GetClusterCredentials operation: The security token included in the request is invalid.
--
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] dimon222 commented on a change in pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
dimon222 commented on a change in pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#discussion_r757117351
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -184,7 +184,13 @@ def get_iam_token(self, conn: Connection) -> Tuple[str, str, int]:
# Pull the custer-identifier from the beginning of the Redshift URL
# ex. my-cluster.ccdre4hpd39h.us-east-1.redshift.amazonaws.com returns my-cluster
cluster_identifier = conn.extra_dejson.get('cluster-identifier', conn.host.split('.')[0])
- client = aws_hook.get_client_type('redshift')
+ session, endpoint_url = aws_hook._get_credentials()
+ client = session.client(
+ "redshift",
+ endpoint_url=endpoint_url,
+ config=aws_hook.config,
+ verify=aws_hook.verify,
+ )
Review comment:
I guess because we expect tuple it can't patch it. Not sure about how such thing should be handled in unit test framework. Any expert here?
--
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] dimon222 commented on pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
dimon222 commented on pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#issuecomment-979332442
Postgres test requires modification to skip mocking of get_client_type maybe? The error is weird.
--
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] potiuk merged pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #19815:
URL: https://github.com/apache/airflow/pull/19815
--
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 change in pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#discussion_r757275334
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -184,7 +184,13 @@ def get_iam_token(self, conn: Connection) -> Tuple[str, str, int]:
# Pull the custer-identifier from the beginning of the Redshift URL
# ex. my-cluster.ccdre4hpd39h.us-east-1.redshift.amazonaws.com returns my-cluster
cluster_identifier = conn.extra_dejson.get('cluster-identifier', conn.host.split('.')[0])
- client = aws_hook.get_client_type('redshift')
+ session, endpoint_url = aws_hook._get_credentials()
+ client = session.client(
+ "redshift",
+ endpoint_url=endpoint_url,
+ config=aws_hook.config,
+ verify=aws_hook.verify,
+ )
Review comment:
We can just tell the mock to return a tuple :) I’ll update.
--
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] github-actions[bot] commented on pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#issuecomment-979978571
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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] dimon222 edited a comment on pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
dimon222 edited a comment on pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#issuecomment-979332442
Postgres test requires modification to skip mocking of get_client_type maybe? The error is weird.
```
tests/providers/postgres/hooks/test_postgres.py::TestPostgresHookConn::test_get_conn_rds_iam_redshift: botocore.exceptions.ClientError: An error occurred (InvalidClientTokenId) when calling the GetClusterCredentials operation: The security token included in the request is invalid.
```
--
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 change in pull request #19815: Amazon provider remove deprecation, second try
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19815:
URL: https://github.com/apache/airflow/pull/19815#discussion_r757297106
##########
File path: airflow/providers/postgres/hooks/postgres.py
##########
@@ -184,7 +184,13 @@ def get_iam_token(self, conn: Connection) -> Tuple[str, str, int]:
# Pull the custer-identifier from the beginning of the Redshift URL
# ex. my-cluster.ccdre4hpd39h.us-east-1.redshift.amazonaws.com returns my-cluster
cluster_identifier = conn.extra_dejson.get('cluster-identifier', conn.host.split('.')[0])
- client = aws_hook.get_client_type('redshift')
+ session, endpoint_url = aws_hook._get_credentials()
+ client = session.client(
+ "redshift",
+ endpoint_url=endpoint_url,
+ config=aws_hook.config,
+ verify=aws_hook.verify,
+ )
Review comment:
This should do it.
--
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