Allow Proxmox Snapshot Restoring

[EinDev]

SUMMARY

Allows the restoration of a proxmox snapshot
ISSUE TYPE

Feature Pull Request

COMPONENT NAME

This Change allows restoring a snapshot in Proxmox
ADDITIONAL INFORMATION

This is the result of #4375
I did not mark snapname as required, as it has a default value and the behaviour didn't change. Or should i still mark it as required?

[ansibullbot]

cc @Ajpantuso @Thulium-Drake @joshainglis @karmab @tleguern
click here for bot help

[felixfontein]

You still have to adjust the documentation of snapname.

[EinDev]

You still have to adjust the documentation of snapname.

I did not mark snapname as required, as it has a default value and the behaviour didn't change. Or should i still mark it as required?

[felixfontein]

I did not mark snapname as required, as it has a default value and the behaviour didn't change. Or should i still mark it as required?

No need to make it required, but you should document what it is used for. Right now the documentation says Name of the snapshot that has to be created..

[EinDev]

@felixfontein Alright, sorry. I have adjusted the documentation. Thank you for your patience

[felixfontein]

Besides this, looks good! If nobody complains, I'll merge in a week.

[EinDev]

If you create a virtualenv with andebox and ansible (ansible-core should suffice for this case), you should be able to activate it and simply run:

(venv) $ andebox test -- units --docker default --python 3.8 tests/unit/plugins/modules/cloud/misc/test_proxmox_snap.py -v

Thank you for the tip, but your solution did not work out for me. As you do not have issues enabled in your repo, i just post the error message here, but to ensure we are not misusing this thread i suggest you contact me here.

collection = community.general
directory  = /tmp/andebox.ovf03rzv/ansible_collections/community/general
Removing temporary directory: /tmp/andebox.ovf03rzv/ansible_collections/community/general
Traceback (most recent call last):
  File "/home/myuser/.local/bin/andebox", line 52, in <module>
    main()
  File "/home/myuser/.local/bin/andebox", line 46, in main
    box.run()
  File "/home/myuser/.local/bin/andebox", line 40, in run
    action.run(args)
  File "/home/myuser/.local/lib/python3.8/site-packages/andeboxlib/actions/ansibletest.py", line 38, in run
    rc = subprocess.call(["ansible-test"] + args.ansible_test_params, cwd=collection_dir)
  File "/usr/lib/python3.8/subprocess.py", line 340, in call
    with Popen(*popenargs, **kwargs) as p:
  File "/usr/lib/python3.8/subprocess.py", line 858, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1704, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: 'ansible-test'

Regarding the original answer (@felixfontein): Thank you very much. I will try to find help there :)

[EinDev]

Update: I managed to get it working by just (un-)installing the pip package for ansible a few times. I will begin adding unit test(s?) now.

Also notice that the proxmox_* modules require the installation of a specific python package for proxmox called proxmoxer.

Absolutely, but as far as i see the current tests do not represent a real use case because of this:
A SystemExit Exception gets raised, but gets catched and ignored.

Is it possible to include the proxmoxer package and directly test the resulting http request? From a technical standpoint yes - but that would require the proxmoxer installed in the ansible-test environment.

Is that possible or possibly unwanted?

[felixfontein]

Absolutely, but as far as i see the current tests do not represent a real use case because of this: A SystemExit Exception gets raised, but gets catched and ignored.

I'm not sure what we should do with that exception other than ignoring it? If the exception isn't raised, something went wrong. If the exception is raised, we need to read the stdout and interpret it as JSON, and look at the results.

Is it possible to include the proxmoxer package and directly test the resulting http request? From a technical standpoint yes - but that would require the proxmoxer installed in the ansible-test environment.

That's possible. The unit tests for the GitLab modules do that for example. (Making sure that proxmoxer is installed requires it to be added to tests/unit/requirements.txt.)

[EinDev]

I've added the unit tests for restoring the snapshot now, but please check it. I hope this is what you want.
I was just a bit confused about an exception, but didn't realize that it was a exception showing the success in this case.

I'm still new to testing, so please don't expect a lot ;)

[EinDev]

I do not have any time planned for this anymore. It works for me, the tests work for me.
I just added this PR because i wanted to share my improvement, if you do not accept it it is fine. It would be a shame though ;)

I will close this PR now, if you feel like you want to merge/improve it feel free to reopen it :)

[felixfontein]

I'm not sure why you don't simply commit suggestion #4377 (comment) ? I would have merged it with the two suggestions (one of which you already commited a few minutes ago) applied.

[EinDev]

I'm sorry, i must have overseen that. Thanks for the heads up :)

[felixfontein]

Will merge tomorrow if nobody complains by then.

[russoz]

LGTM

[patchback]

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/dbad1e0f1123cbb8694fa6571770176c394fe711/pr-4377

Backported as #4571

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

[felixfontein]

@EinDev thanks for your contribution!
@Thulium-Drake @russoz thanks for reviewing!