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

proxmox module_utils: fix get_vm int parse handling #4945

Merged
merged 5 commits into from
Jul 21, 2022

Conversation

miyuk
Copy link
Contributor

@miyuk miyuk commented Jul 12, 2022

SUMMARY

When target vm is not exist and get vm by only target vm name, proxmox module raise int parse error.

This PR fix only pattern of vmid is None.
I did't implement other pattern because I guess vmid is only int or None.

Closes #4944

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

proxmox

ADDITIONAL INFORMATION
none

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module_utils module_utils new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Jul 12, 2022
@felixfontein
Copy link
Collaborator

Thanks for your contribution! I'm wondering why vmid should ever be None when passed into this function. It might be better to fix that, instead of changing the function to also handle None?

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-4 labels Jul 12, 2022
@miyuk
Copy link
Contributor Author

miyuk commented Jul 12, 2022

@felixfontein thanks for your reply.

It might be better to fix that, instead of changing the function to also handle None?

maybe yes.
However, I guess better fix get_vm() than caller because that is parse int error.

If fix caller, affected code is below (proxmox_kvm)

@felixfontein
Copy link
Collaborator

But this error happens only because an invalid value is passed to this function, which shouldn't have happened in the first place.

@felixfontein
Copy link
Collaborator

(This fix could mask other errors in other uses of this function, since it is a generic utility function that is used by multiple modules, potentially even ones outside of this repository.)

@miyuk
Copy link
Contributor Author

miyuk commented Jul 13, 2022

thanks.
I understood your opinion.
I will fix caller. so please wait abount a week.

@miyuk
Copy link
Contributor Author

miyuk commented Jul 13, 2022

fix on module source code.
please review my code.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added cloud module module and removed small_patch Hopefully easy to review labels Jul 13, 2022
@github-actions
Copy link

github-actions bot commented Jul 13, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@felixfontein
Copy link
Collaborator

The change looks good, it still needs a changelog fragment though.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. If nobody complains, I'll merge this for the next release :)

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jul 21, 2022
@felixfontein felixfontein merged commit c57204f into ansible-collections:main Jul 21, 2022
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug cloud module_utils module_utils module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

community.general.proxmox: failed get vm by int parse error if not specify vmid
4 participants