-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add endpoint_url in test_connection #32664
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
Please set up pre-commit (see link in the above comment) and fix static check failures. |
test_endpoint_url = self.conn_config.extra_config.get("test_endpoint_url") | ||
conn_info = session.client( | ||
"sts", | ||
endpoint_url=test_endpoint_url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you tested it when test_endpoint_url
is None. My experience with boto3
is they make a difference between a parameter value being None
and a parameter not provided to the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of them returned successful result when I've tested both being None and a parameter not provided to the function.
I've executed code below
conn = Connection(
conn_id="sample_aws_connection",
conn_type="aws",
login="access_key", # Reference to AWS Access Key ID
password="secret_key", # Reference to AWS Secret Access Key
extra={
"test_endpoint_url": "https://sts.us-east-1.amazonaws.com",
"region_name": "us-east-1"
},
)
env_key = f"AIRFLOW_CONN_{conn.conn_id.upper()}"
conn_uri = conn.get_uri()
os.environ[env_key] = conn_uri
conn.test_connection()
and I've compared results of two connection. Both of them returned successful result, and there's difference of RequestID
in ResponseMetadata
and x-amzn-requestid
, date
in HTTPHeaders
conn_info = session.client(
"sts",
endpoint_url=test_endpoint_url,
).get_caller_identity()
conn_info_without_endpoint_url = session.client(
"sts"
).get_caller_identity()
Could you let me know if there's anything I've missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops there's difference in endpoint of sts session. When I've printed endpoint, former returns url of test_endpoint_url
and later returns Global endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code you provided, you dont test when endpoint_url
is None
but I assume you tested it as you said. LGTM then
@vincbeck After making the mentioned change, the test for |
That would be great :) |
@vincbeck I've added unit test for test_endpoint_url. Would you mind to review my code? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll merge it once the CI is green
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
In this pull request, we are resolving an issue related to the failure of the test connection caused by an incorrect STS URL. With this pull request, I've ensure that if the 'test_endpoint_url' parameter is provided, the STS check will be performed using that specific URL. However, if the parameter is not provided, the default URL will be used instead.
related: #32652
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.