Skip to content
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 error 'authorization on proxmox cluster failed with exception: in… #61358

Open
wants to merge 1 commit into
base: devel
from

Conversation

@zorgzerg
Copy link

commented Aug 27, 2019

SUMMARY
With version 6 of proxmox, the proxmox.version.get()['version'] return something like 6.0-4 instead of something like 5.0. The cast as a float is not possible anymore, but the variable PVE_MAJOR_VERSION should only contain the first number of the version string. So I split the line with the "-" separator and get first part.

ISSUE TYPE
Bugfix Pull Request
COMPONENT NAME
proxmox.py

Recreated PR

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@limakzi

This comment has been minimized.

Copy link

commented Aug 27, 2019

@zorgzerg Accordingly to mine and @jocelynj dicussion, #59356 has way better environment variable mapping. We have agreed on having such variable mapping in this pull-request.
Would you be so kind and apply them?

@@ -347,7 +347,7 @@ def create_instance(module, proxmox, vmid, node, disk, storage, cpus, memory, sw
kwargs.update(kwargs['mounts'])
del kwargs['mounts']
if 'pubkey' in kwargs:
if float(proxmox.version.get()['version']) >= 4.2:
if float(proxmox.version.get()['version'].split('-')[0]) >= 4.2:

This comment has been minimized.

Copy link
@limakzi

limakzi Aug 27, 2019

Have a look at #59356. You can find there a variable mapping.

This comment has been minimized.

Copy link
@zorgzerg

zorgzerg Aug 28, 2019

Author

What do you mean by "variable mapping"? Here we are checking for version 4.2, if I convert the type to int, we can get undefined behavior when working with proxmox 4.1, for example

@zorgzerg

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

@limakzi
Honestly, I see no reason why this should be changed. But if you insist - I will do

Fix error 'authorization on proxmox cluster failed with exception: in…
…valid literal for float(): 6.0-X' during LXC creation for Proxmox 6.0

@zorgzerg zorgzerg force-pushed the zorgzerg:devel branch from 73d43ae to 1cdbaf0 Aug 28, 2019

@jocelynj

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

From my point of view, the latest commit correctly fixes the issue with latest proxmox version.

@gundalow

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

@limakzi Is this OK now?

@gundalow gundalow added the pr_day label Sep 3, 2019

@limakzi

This comment has been minimized.

Copy link

commented Sep 6, 2019

@gundalow , @jocelynj Actually, I do not like this code. :-(

  • First of all, I would not use float as version mapping. tuple of int are way, way better.

  • Secondly, I do not like that we gather facts about version (and their functionalities) in various functions. Wouldn't it be better to have one method that would define, what is supported and what not?

What do you think about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.