-
Notifications
You must be signed in to change notification settings - Fork 23.9k
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
Handle openstacksdk < 0.10.0: fix AttributeError #50285
Handle openstacksdk < 0.10.0: fix AttributeError #50285
Conversation
Build succeeded (third-party-check pipeline).
|
module.fail_json( | ||
msg="To utilize this module, the installed version of " | ||
"the openstacksdk library MUST be >=0.10.0") | ||
|
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.
Why this code change is required?
Isn't this already checked in the lines above?
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.
min_version
also defaults to 0.12.0
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.
Another version could be used.
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.
When openstacksdk 0.9.19 is used (assuming caller is using a lower version than the default one), without the additional check, the following exception occurs:
Traceback (most recent call last):
File "~/.ansible/tmp/ansible-tmp-1555747400.49-76880204403181/AnsiballZ_os_security_group.py", line 113, in <module>
_ansiballz_main()
File "~/.ansible/tmp/ansible-tmp-1555747400.49-76880204403181/AnsiballZ_os_security_group.py", line 105, in _ansiballz_main
invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
File "~/.ansible/tmp/ansible-tmp-1555747400.49-76880204403181/AnsiballZ_os_security_group.py", line 48, in invoke_module
imp.load_module('__main__', mod, module, MOD_DESC)
File "/tmp/ansible_os_security_group_payload_0EWiaz/__main__.py", line 163, in <module>
File "/tmp/ansible_os_security_group_payload_0EWiaz/__main__.py", line 115, in main
File "/tmp/ansible_os_security_group_payload_0EWiaz/ansible_os_security_group_payload.zip/ansible/module_utils/openstack.py", line 162, in openstack_cloud_from_module
AttributeError: 'module' object has no attribute 'exceptions'
The current code isn't compatible with openstacksdk < 0.10.0.
Because this is a module_utils
, other module not belonging to the ansible repository might use this method with another value than the default one.
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.
@marsavela @dominikholler: please, could you take a look at this ?
@@ -114,16 +114,22 @@ def openstack_cloud_from_module(module, min_version='0.12.0'): | |||
# Due to the name shadowing we should import other way | |||
import importlib | |||
sdk = importlib.import_module('openstack') | |||
sdk_version = importlib.import_module('openstack.version') |
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.
Why do you think this additional line is helpful?
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 read the stacktrace mentioned in the description of this PR and in the first commit ?
sdk.version.__version__
expression raises an AttributeError
exception when openstacksdk < 0.10.0 is used.
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 read the stacktrace mentioned in the description of this PR and in the first commit ?
Sure, but I did not understand how this is related, but with your additional exlaination, I understand now.
The commit message could be improved by writing something like
"openstack.version.__version__
expression raises an AttributeError exception when openstacksdk < 0.10.0 is used. openstack.version is now imported as a module, which works for all openstacksdk versions.
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.
PR updated:
- commit message updated
- rebased on devel
@@ -114,16 +114,22 @@ def openstack_cloud_from_module(module, min_version='0.12.0'): | |||
# Due to the name shadowing we should import other way | |||
import importlib | |||
sdk = importlib.import_module('openstack') | |||
sdk_version = importlib.import_module('openstack.version') | |||
except ImportError: | |||
module.fail_json(msg='openstacksdk is required for this module') | |||
|
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 current code isn't compatible with openstacksdk < 0.10.0.
Because this is a module_utils, other module not belonging to the ansible repository might use this method with another value than the default one.
Sounds like min_version should be set to max(StrictVersion('0.10.0'), StrictVersion(min_version)) here.
This would avoid the redundancy below, too.
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.
Some modules rely on this default value and don't work with 0.10.0
.
For example using os_security_group
:
- with
devel
branch - the following diff applied
-def openstack_cloud_from_module(module, min_version='0.12.0'): +def openstack_cloud_from_module(module, min_version='0.10.0'):
openstacksdk==0.10.0
installed (same error occurs with0.11.0
)- this task:
- os_security_group: cloud: mycloud state: present name: foo
this exception occurs:
TASK [os_security_group] ***
An exception occurred during task execution. To see the full traceback, use -vvv.
The error was: AttributeError: 'Connection' object has no attribute 'current_project_id'
fatal: [localhost]: FAILED! => {
"changed": false,
"rc": 1
}
MSG:
MODULE FAILURE
See stdout/stderr for the exact error
MODULE_STDERR:
Traceback (most recent call last):
File "$HOME/.ansible/tmp/ansible-tmp-1556617879.09-260636442237090/AnsiballZ_os_security_group.py", line 125, in <module>
_ansiballz_main()
File "$HOME/.ansible/tmp/ansible-tmp-1556617879.09-260636442237090/AnsiballZ_os_security_group.py", line 117, in _ansiballz_main
invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
File "$HOME/.ansible/tmp/ansible-tmp-1556617879.09-260636442237090/AnsiballZ_os_security_group.py", line 54, in invoke_module
imp.load_module('__main__', mod, module, MOD_DESC)
File "/tmp/ansible_os_security_group_payload_uLUg4r/__main__.py", line 163, in <module>
File "/tmp/ansible_os_security_group_payload_uLUg4r/__main__.py", line 123, in main
AttributeError: 'Connection' object has no attribute 'current_project_id'
Then if 0.10.0
is used by default, callers must be modified in order to pass 0.12.0
when required. Is that OK with you ?
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.
Some modules rely on this default value and don't work with 0.10.0.
No doubts about this.
Then if 0.10.0 is used by default, callers must be modified in order to pass 0.12.0 when required. Is that OK with you ?
Sorry, I did not mean to use 0.10.0 as the default, but process the bigger value, either 0.10.0 or the parmeter value, which is 0.12.0 by default.
This way we would use 0.10.0 if < 0.10.0 is requested or 0.12.0 if the default parmater value is used. If the parameter is set explicitly to > 0.10.0, we would use this.
Is this explained understandable?
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.
Yes, thanks!
PR updated.
6d2f527
to
f3e0a72
Compare
Build succeeded (third-party-check pipeline).
|
f3e0a72
to
970d508
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 updates!
Did can you please write in a comment, for which versions of openstack sdk you verified this code?
@@ -114,15 +114,20 @@ def openstack_cloud_from_module(module, min_version='0.12.0'): | |||
# Due to the name shadowing we should import other way | |||
import importlib | |||
sdk = importlib.import_module('openstack') | |||
sdk_version = importlib.import_module('openstack.version') | |||
except ImportError: | |||
module.fail_json(msg='openstacksdk is required for this module') | |||
|
|||
if min_version: |
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.
Maybe we can remove the if min_version:
? min_version has a useful default, and if min_version=None
is passed, it is OK to explode, from my point of view.
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.
It's not OK (see).
Build succeeded (third-party-check pipeline).
|
No error.
|
bot_status |
Componentslib/ansible/module_utils/openstack.py Metadatawaiting_on: maintainer |
We should not attempt to handle openstacksdk < 0.10.0. The Ansible OpenStack modules have never supported <0.10.0 and should not attempt to start doing so now. People who want to use older versions of libraries with Ansible for out-of-tree drivers should be using the shade library, which is what was needed for the Ansible OpenStack modules before sdk 0.10.0. Even still, I'm not sure we even want to attempt to support 0.10.0 when the default min is 0.12.0. Let's just keep the default min as 0.12. That said - I actually like most of this PR - since it prevents people from having modules that call this with a lower min_version than our actual min and gives them a better error message. Let's update it to use 0.12 instead of 0.12 and then I'm good with it. |
msg="To utilize this module, the installed version of" | ||
"the openstacksdk library MUST be >={min_version}".format( | ||
min_version=min_version)) | ||
min_version = max(StrictVersion('0.10.0'), StrictVersion(min_version)) |
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.
Let's change this to 0.12 to match our min.
min_version=min_version)) | ||
min_version = max(StrictVersion('0.10.0'), StrictVersion(min_version)) | ||
else: | ||
min_version = StrictVersion('0.10.0') |
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.
And this.
`openstack.version.__version__` expression raises an `AttributeError` exception when openstacksdk < 0.10.0 is used. `openstack.version` is now imported as a module, which works for all openstacksdk versions. Error was: The full traceback is: Traceback (most recent call last): File "$HOME/.ansible/tmp/ansible-tmp-1545612308.8-46792777824159/AnsiballZ_os_security_group.py", line 113, in <module> _ansiballz_main() File "$HOME/.ansible/tmp/ansible-tmp-1545612308.8-46792777824159/AnsiballZ_os_security_group.py", line 105, in _ansiballz_main invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS) File "$HOME/.ansible/tmp/ansible-tmp-1545612308.8-46792777824159/AnsiballZ_os_security_group.py", line 48, in invoke_module imp.load_module('__main__', mod, module, MOD_DESC) File "/tmp/ansible_os_security_group_payload_keFTIJ/__main__.py", line 163, in <module> File "/tmp/ansible_os_security_group_payload_keFTIJ/__main__.py", line 115, in main File "/tmp/ansible_os_security_group_payload_keFTIJ/ansible_os_security_group_payload.zip/ansible/module_utils/openstack.py", line 121, in openstack_cloud_from_module AttributeError: 'module' object has no attribute 'version'
970d508
to
6e9b003
Compare
Build succeeded (third-party-check pipeline).
|
@emonty thanks for the review ! PR rebased and updated following your feedback ( ready_for_review |
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.
shipit
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.
shipit
bot_status |
Componentslib/ansible/module_utils/openstack.py lib/ansible/plugins/doc_fragments/openstack.py Metadatawaiting_on: maintainer |
@emonty please, could you manually merge this one ? |
SUMMARY
Handle openstacksdk < 0.10.0: fix
AttributeError
Error was:
ISSUE TYPE
COMPONENT NAME
lib/ansible/module_utils/openstack.py
ADDITIONAL INFORMATION