-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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 the deprecated parameter in hvac client #31349
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)
|
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.
The raise_on_deleted_version
parameter was introduced in hvac 1.1.0. Using it with an older version will result in an exception because the read_secret_version method does not accept additional kwargs:
https://github.com/briantist/hvac/blob/669ddfc935014f37273655d6b4b9f7e44661ddf0/hvac/api/secrets_engines/kv_v2.py#L104-L110
If we want to provide a value for the raise_on_deleted_version
argument, we need to check the hvac version and add it only for versions >= 1.1.0. We can remove this check when we increase the minimum required hvac version to 1.1.0 (here).
Here is an example to how we provide arguments based on the lib version: airflow/airflow/providers/mongo/hooks/mongo.py Lines 87 to 94 in 7ebda38
|
Thanks for the guidance, I'm looking at how I can provide the argument based on the library version, unfortunately >>> import hvac
>>> hvac.__version__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: module 'hvac' has no attribute '__version__' Would it be acceptable to implement this via the >>> from importlib.metadata import version
>>> version('hvac')
'1.1.0'
>>> version(hvac.__name__) # better to check without using the library name as string
'1.1.0' |
If I am not mistaken, this method is not available in Python 3.7 (it was introduced in Python 3.8), and since #30963 has not been merged yet, we cannot use it. |
Yes, I just saw that. It's available in 3.8+. 3.7 support will end on 2023-06-27, but until then, do you have another suggestion to check the library version? There's this, but I'm not sure whether it slows things down: >>> import pkg_resources
>>> pkg_resources.get_distribution(hvac.__name__).version
'1.1.0' |
Can you try this? import sys
if sys.version_info < (3, 8):
from importlib_metadata import version
else:
from importlib.metadata import version
hvac_version = version("hvac") |
Unfortunately I don't have enough knowledge about how to update the tests to take the new function signatures into account 😞. Can someone else pick that part up? |
It's a great opportunity to learn. |
e1545af
to
2d2d19f
Compare
@hussein-awala, I updated the tests for Though I now see Python 3.7 support is dropped on the main branch. Should I remove this check from the files: import sys
if sys.version_info < (3, 8):
from importlib_metadata import version
else:
from importlib.metadata import version and directly use |
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.
Could you add a new test which patch version
and test with 2 different versions, one older than 1.1.0 and another >= 1.1.0?
@emredjan are you still working on this PR? |
Unfortunately no. I don't currently have the time to figure out how to add the tests. But please feel free to pick it up. Also now that the support for Python 3.7 is dropped, we don't the checks for that in the imports. |
We've not done that before for PRS - but marked it as "good first issue" for someone to pick up |
We have an open issue for this one so added the label there and I guess we can close this one as stale. |
closing as stale. |
Adds an explicit parameter to fix deprecation warnings in
hvac
client, as described in hvac/hvac#907closes: #31347
^ Add meaningful description above
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.