-
Notifications
You must be signed in to change notification settings - Fork 16.3k
fix(providers-weaviate): Correct HTTP port logic in WeaviateHook #55457
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
fix(providers-weaviate): Correct HTTP port logic in WeaviateHook #55457
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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
356259a to
37ef803
Compare
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.
thanks for the fix. wondering if it would be better to have one parameterized test case to specifically test for this port configuration. For example,
if port is not specified and http_secure is False, then 80
if port is not specified and http_secure is True, then 443
if port is set to 8000 and http_secure is False, then 8000
etc.
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.
Thanks for the review. I think that it would, but I must admit that I am not that good with testing frameworks so I have not risked including it at the moment.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
closes: #55433
Summary
This PR fixes a bug in the
WeaviateHookwhere theportspecified in the Airflow connection is ignored for non-HTTPS configurations, causing the hook to always default to port 80.The root cause is an operator precedence issue in the
http_portassignment. This change corrects the logic by adding parentheses to ensure the ternary operator is evaluated correctly, thus respecting the user-configured port.Proposed Change
The logic in
airflow/providers/weaviate/hooks/weaviate.pyis corrected as follows:Before:
After:
Tests
Documentation