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.

Sign up for GitHub

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

Jump to bottom

Refactor all Proxmox modules to use shared module_utils. #4029

Merged
merged 7 commits into from
Feb 7, 2022
Merged

Refactor all Proxmox modules to use shared module_utils. #4029

merged 7 commits into from
Feb 7, 2022

Conversation

reitermarkus
Copy link
Contributor

SUMMARY

Use shared module_utils for all Proxmox modules.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

proxmox*

ADDITIONAL INFORMATION

Extracted from #4027 for easier review.

@ansibullbot
Copy link
Collaborator

cc @Ajpantuso @Kogelvis @Thulium-Drake @UnderGreen @helldorado @joshainglis @karmab @paginabianca @tleguern
click here for bot help

@ansibullbot ansibullbot added cloud feature This issue/PR relates to a feature request module module module_utils module_utils new_contributor Help guide this first time contributor plugins plugin (any type) needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 12, 2022
@ansibullbot ansibullbot added tests tests unit tests/unit labels Jan 12, 2022
@reitermarkus reitermarkus force-pushed the proxmox-refactor branch 3 times, most recently from d68190c to 3af0e9c Compare January 12, 2022 13:17
@ansibullbot

This comment has been minimized.

Sign in to view
@reitermarkus reitermarkus force-pushed the proxmox-refactor branch 4 times, most recently from c3cc543 to 855e1bc Compare January 12, 2022 13:50
@ansibullbot ansibullbot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jan 12, 2022
@ansibullbot

This comment has been minimized.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Jan 12, 2022
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jan 12, 2022
@reitermarkus reitermarkus force-pushed the proxmox-refactor branch 4 times, most recently from a7b47dc to 722f34e Compare January 12, 2022 14:23
@ansibullbot ansibullbot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jan 12, 2022
@reitermarkus reitermarkus force-pushed the proxmox-refactor branch 2 times, most recently from 8aa98d9 to d37dbbd Compare January 20, 2022 14:19
@ansibullbot

This comment has been minimized.

Sign in to view
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jan 20, 2022
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 20, 2022
@felixfontein
Copy link
Collaborator

Ok, so what's the current state of this? @m-rtijn @reitermarkus do you need to discuss more?

@m-rtijn
Copy link
Contributor

m-rtijn commented Jan 22, 2022

Ok, so what's the current state of this? @m-rtijn @reitermarkus do you need to discuss more?

The code looks OK now, but I have not yet had the time to test it out.

@felixfontein
Copy link
Collaborator

@m-rtijn any update? There will be a release on Tuesday morning, would be nice if this could be included.

@m-rtijn
Copy link
Contributor

m-rtijn commented Jan 31, 2022

@m-rtijn any update? There will be a release on Tuesday morning, would be nice if this could be included.

Sorry, life got in the way. I'm currently not in the position to test it on an actual proxmox machine, but if everyone else thinks it's OK it's fine for me too.

@felixfontein
Copy link
Collaborator

@Ajpantuso @Kogelvis @Thulium-Drake @UnderGreen @helldorado @joshainglis @karmab @paginabianca @tleguern what do you think?

@Thulium-Drake
Copy link
Contributor

Thulium-Drake commented Feb 2, 2022 •
edited
Loading

Looks good to me, also works fine for the modules I have tested (not a lot, I only invested a lot of time in creating playbooks to make snapshots :-) and I have a test play for creating and deleting an LXC container)
@reitermarkus Thanks for the hard work!

@felixfontein
Copy link
Collaborator

felixfontein commented Feb 2, 2022 •
edited
Loading

@Thulium-Drake thanks for your feedback!

I guess I'll wait until beginning of next week (probably like Monday morning in my timezone), and if nobody complains by then, I'll merge.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 7, 2022
@felixfontein felixfontein merged commit a61bdba into ansible-collections:main Feb 7, 2022
@patchback
Copy link

patchback bot commented Feb 7, 2022 •
edited
Loading

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/a61bdbadd590668d4bbceb002a94963b32908327/pr-4029

Backported as #4164

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 7, 2022
@reitermarkus @patchback
Refactor all Proxmox modules to use shared module_utils. (#4029)
f87feff 
* Refactor Proxmox modules to use `module_utils`.

* Fix tests.

* Rename `node_check`.

* Add `ignore_missing` to `get_vm`.

* Refactor `proxmox` module.

* Add changelog entry.

* Add `choose_first_if_multiple` parameter for deprecation.

(cherry picked from commit a61bdba)
@patchback patchback bot mentioned this pull request Feb 7, 2022
Merged
@felixfontein
Copy link
Collaborator

felixfontein commented Feb 7, 2022 •
edited
Loading

@reitermarkus thanks for working on this!
@tleguern @m-rtijn @Thulium-Drake thanks for reviewing this!

felixfontein pushed a commit that referenced this pull request Feb 7, 2022
@patchback @reitermarkus
Refactor all Proxmox modules to use shared module_utils. (#4029) (#…
a678029 
…4164)

* Refactor Proxmox modules to use `module_utils`.

* Fix tests.

* Rename `node_check`.

* Add `ignore_missing` to `get_vm`.

* Refactor `proxmox` module.

* Add changelog entry.

* Add `choose_first_if_multiple` parameter for deprecation.

(cherry picked from commit a61bdba)

Co-authored-by: Markus Reiter <me@reitermark.us>
@felixfontein felixfontein mentioned this pull request Feb 25, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein left review comments

@m-rtijn m-rtijn m-rtijn left review comments

@tleguern tleguern tleguern approved these changes

Assignees
No one assigned
Labels
cloud feature This issue/PR relates to a feature request module_utils module_utils module module plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@reitermarkus @ansibullbot @m-rtijn @felixfontein @Thulium-Drake @tleguern