Add module proxmox_backup_info #9437

raoufnezhad · 2024-12-28T10:14:47Z

SUMMARY

This contribution proposes to add a module called proxmox_backup_info to the Ansible community.general collection. The module would retrieve information about backups in a Proxmox VE cluster, allowing users to query and gather details about existing backups.

ISSUE TYPE

New Module/Plugin Pull Request

COMPONENT NAME

proxmox_backup_info

ADDITIONAL INFORMATION

The module would use the Proxmox API to fetch information about backups in the cluster. It would allow users to specify parameters such as the API host, credentials, and optionally filter by VM ID, VM Name, or backup section. The result of the module would be a list of backups with details like backup type, backup id, mode, schedule, next-run time, storage, and associated VM.

The `proxmox_backup_info` module displays information such as backup times, VM name, VM ID, mode, backup type, and backup schedule using the Proxmox Server API.

create test for proxmox_backup_info.py module

ansibullbot · 2024-12-28T10:28:53Z

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

russoz

hi @raoufnezhad thanks for your contribution!

I see you have adjustments to make for the sanity tests. If I may suggest, give it a try to andebox (by yours truly):

https://pypi.org/project/andebox/

You can run this sanity test easily in your local machine by simply running:

$ andebox test -- sanity --docker default --python 3.10 plugins/modules/your_module.py

I left a couple of other comments below as well. Thanks again!

plugins/modules/proxmox_backup_info.py

russoz · 2024-12-29T01:04:00Z

plugins/modules/proxmox_backup_info.py

+options:
+      api_user:
+        description: The user must have access to the proxmox server 
+        required: true


Please use consistent indentation. Here you started with 4 spaces then used 2 in the next level. We have been trying to use 2 at all times, that's not mandatory, but consistency is.

Thank you for your attention.

russoz · 2024-12-29T01:08:40Z

plugins/modules/proxmox_backup_info.py

+    description: The return value provided by proxmox_backup_info.
+    type: dict


That description does not mean much. Please be either more specific or add notes to the documentation block referring to Proxmox API, and/or the return values provided by proxmoxer.

If the result value is not too big nor complex, please consider declaring contains block with a more finely grained level of details.

Co-authored-by: Alexei Znamensky <[email protected]>

felixfontein

Thanks for your contribution!

plugins/modules/proxmox_backup_info.py

felixfontein · 2024-12-30T17:28:30Z

plugins/modules/proxmox_backup_info.py

+    required: false
+    type: int
+  backup_section:
+    description: The backup_section value is true or false. if this be true, this module just return all backup job information.


I think this option means that if it is set to true, then all backups for this 'section' (whatever that is, I don't know the proxmox terminology) will be listed. If set to false, what is listed depends on the other options.

Is that correct? A description that's closer to the above might be more helpful.

plugins/modules/proxmox_backup_info.py

felixfontein · 2024-12-30T17:29:38Z

plugins/modules/proxmox_backup_info.py

+  elements: dict
+  contains:
+    bktype:
+      description: backup type


The descriptions here also need to be full sentences.

Suggested change

description: backup type

description: The type of the backup.

Co-authored-by: Felix Fontein <[email protected]>

russoz · 2024-12-31T07:12:53Z

plugins/modules/proxmox_backup_info.py

+      - The backup_jobs value is true or false.
+      - If set to true, this module just return all backup jobs information.
+      - If set to false, what is listed depends on the other options.
+    required: false


required: false is redundant, please remove it from both here and the argument_spec below.
Also, use semantic markup (such as V()), and boolean parameters may have, by definition, only the values true and false, no need to mention that ;-).

Suggested change

- The backup_jobs value is true or false.

- If set to true, this module just return all backup jobs information.

- If set to false, what is listed depends on the other options.

required: false

- Include backup jobs in the result.

- If V(true), this module just return all backup jobs information.

- If V(false), the result will not include backup jobs.

Or something like that.

Semantic markup is documented in:
https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#semantic-markup-within-module-documentation

russoz

Hi @raoufnezhad thanks again for your contribution!

There's a number of adjustments that need to be made. Please see below, feel free to question anything.

russoz · 2025-01-02T22:54:41Z

plugins/modules/proxmox_backup_info.py

+      - The backup_jobs value is true or false.
+      - If V(true), the module will return all backup jobs information.
+      - If V(false), the module will parse all backup jobs based on VM IDs and return a list of VMs' backup information.


I think the first line is redundant:

Suggested change

- The backup_jobs value is true or false.

- If V(true), the module will return all backup jobs information.

- If V(false), the module will parse all backup jobs based on VM IDs and return a list of VMs' backup information.

- If V(true), the module will return all backup jobs information.

- If V(false), the module will parse all backup jobs based on VM IDs and return a list of VMs' backup information.

With the other two it is already clear what the option does.

russoz · 2025-01-02T22:55:35Z

plugins/modules/proxmox_backup_info.py

+  vm_id:
+    description:
+      - The ID of the Proxmox VM.
+      - If vm_id is defined, the returned list will contain backup jobs that have been parsed and filtered based on vm_id value.


Suggested change

- If vm_id is defined, the returned list will contain backup jobs that have been parsed and filtered based on vm_id value.

- If defined, the returned list will contain backup jobs that have been parsed and filtered based on O(vm_id) value.

russoz · 2025-01-02T22:55:55Z

plugins/modules/proxmox_backup_info.py

+  vm_name:
+    description:
+      - The name of the Proxmox VM.
+      - If vm_name is defined, the returned list will contain backup jobs that have been parsed and filtered based on vm_name value.


Suggested change

- If vm_name is defined, the returned list will contain backup jobs that have been parsed and filtered based on vm_name value.

- If defined, the returned list will contain backup jobs that have been parsed and filtered based on O(vm_name) value.

russoz · 2025-01-02T22:56:29Z

plugins/modules/proxmox_backup_info.py

+  - "Marzieh Raoufnezhad (@raoufnezhad) <[email protected]>"
+  - "Maryam Mayabi (@mmayabi) <[email protected]>"
+
+options:


Suggested change

options:

version_added: 10.3.0

options:

russoz · 2025-01-02T22:57:37Z

plugins/modules/proxmox_backup_info.py

+      returned: on success
+      type: str
+    enabled:
+      description: 1 if backup is enabled else 0.


Suggested change

description: 1 if backup is enabled else 0.

description: V(1) if backup is enabled else V(0).

russoz · 2025-01-02T23:04:53Z

plugins/modules/proxmox_backup_info.py

+    vmid:
+      description: The VM vmid.
+      returned: on success
+      type: int


I understand the Proxmox VMID can be an UUID, in which case it is not an int but a str. Could you please double check and confirm here? Thx

russoz · 2025-01-02T23:09:53Z

plugins/modules/proxmox_backup_info.py

+class ProxmoxBackupInfoAnsible(ProxmoxAnsible):
+
+    # Get all backup information
+    def getJobsList(self):


This is not very pythonic. Please use:

Suggested change

def getJobsList(self):

def get_jobs_list(self):

instead. Same for other methods and variables. Python only uses CamelCase in class names.

Personally, I would even remove the "list" part in the name, because if we are getting "jobs" (plural), I think it is clear that it is a list of them.

russoz · 2025-01-02T23:17:57Z

tests/unit/plugins/modules/test_proxmox_backup_info.py

+
+    def test_get_specific_backup_information_by_vmid(self):
+        with pytest.raises(AnsibleExitJson) as exc_info:
+            vmid = "101"


The vmid parameter is int, so this should be:

Suggested change

vmid = "101"

vmid = 101

russoz · 2025-01-02T23:25:17Z

plugins/modules/proxmox_backup_info.py

+    def specificVmBackupInfo(self, vm_name_id):
+        fullBackupInfo = self.vmsBackupInfo()
+        vmBackupJobs = []
+        for vm in fullBackupInfo:
+            if (vm["vm_name"] == vm_name_id or int(vm["vmid"]) == vm_name_id):
+                vmBackupJobs.append(vm)
+        return vmBackupJobs


I don't think this is right. If vm_name_id="100" (str), then int(vm["vmid"]) == vm_named_id is always going to be False, even then int(vm["vmid"]) == 100.

>>> 3 == "3" False

Additionally, there is the naming convention (camelCase vs. snake_case), and the more pythonic way of doing this would be:

Suggested change

def specificVmBackupInfo(self, vm_name_id):

fullBackupInfo = self.vmsBackupInfo()

vmBackupJobs = []

for vm in fullBackupInfo:

if (vm["vm_name"] == vm_name_id or int(vm["vmid"]) == vm_name_id):

vmBackupJobs.append(vm)

return vmBackupJobs

return [vm for vm in full_backup_info if vm["vm_name"] == vm_name_id or int(vm["vmid"]) == vm_name_id]

(it won't work like that because of the type issue mentioned above - needs adjustments)

Not goin to be a huge problem here, but from a performance point of view it is much better to do the test outside the loop than inside it.

if is int return [... if vm[vmid] == param] if is str return [... if vm[name] == param]

Or, make two separate functions, one for the ID another for the name, which is cleaner to read, IMHO.

russoz · 2025-01-02T23:31:01Z

plugins/modules/proxmox_backup_info.py

+        vm_id=dict(type='int', required=False),
+        vm_name=dict(type='str', required=False),
+        backup_jobs=dict(type='bool', default=False, required=False)


Redundant

Suggested change

vm_id=dict(type='int', required=False),

vm_name=dict(type='str', required=False),

backup_jobs=dict(type='bool', default=False, required=False)

vm_id=dict(type='int'),

vm_name=dict(type='str'),

backup_jobs=dict(type='bool', default=False)

raoufnezhad added 2 commits December 28, 2024 13:17

Create proxmox_backup_info.py

8439fd1

The `proxmox_backup_info` module displays information such as backup times, VM name, VM ID, mode, backup type, and backup schedule using the Proxmox Server API.

Create test_proxmox_backup_info.py

2eb2e8a

create test for proxmox_backup_info.py module

This comment was marked as outdated.

Sign in to view

ansibullbot added module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit labels Dec 28, 2024

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Dec 28, 2024

russoz reviewed Dec 29, 2024

View reviewed changes

Update plugins/modules/proxmox_backup_info.py

2b1c48f

Co-authored-by: Alexei Znamensky <[email protected]>

This comment was marked as outdated.

Sign in to view

raoufnezhad added 3 commits December 29, 2024 12:12

check tests proxmox_backup_info.py

469afec

check tests test_proxmox_backup_info.py

5ede419

Update check tests test_proxmox_backup_info.py

193a13c

This comment was marked as outdated.

Sign in to view

raoufnezhad added 5 commits December 29, 2024 12:44

Update check tests proxmox_backup_info.py

f0f76c4

Update authors proxmox_backup_info.py

1fd1106

Update active maintainers for proxmox_backup_info module

0be21d8

Update add proxmox_backup_info module in proxmox group

deba349

edit timestamp to UTC test_proxmox_backup_info.py

4fcdd00

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 29, 2024

felixfontein reviewed Dec 29, 2024

View reviewed changes

raoufnezhad added 2 commits December 30, 2024 10:05

Update vm name or vmid to VM name or VM id proxmox_backup_info.py

57791ab

update documentation in proxmox_backup_info.py

8e8dfb0

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 30, 2024

Update backup_section decription in proxmox_backup_info.py

87d99c8

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 30, 2024

felixfontein reviewed Dec 30, 2024

View reviewed changes

raoufnezhad and others added 4 commits December 31, 2024 09:02

Update plugins/modules/proxmox_backup_info.py

95c5fdd

Co-authored-by: Felix Fontein <[email protected]>

change backup_section to backup_jobs

86dc5ed

change backup_section to backup_jobs

cca7ead

remove whitespace in line 35 and 36

b4d2fd8

russoz reviewed Dec 31, 2024

View reviewed changes

improve descriptions

68abb06

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 31, 2024

check again proxmox_backup_info.py module

61d475a

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 31, 2024

russoz reviewed Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add module proxmox_backup_info #9437

Add module proxmox_backup_info #9437

raoufnezhad commented Dec 28, 2024 •

edited by felixfontein

Loading

This comment was marked as outdated.

ansibullbot commented Dec 28, 2024

russoz left a comment

russoz Dec 29, 2024

raoufnezhad Dec 30, 2024

russoz Dec 29, 2024

This comment was marked as outdated.

This comment was marked as outdated.

felixfontein left a comment

felixfontein Dec 30, 2024

felixfontein Dec 30, 2024

russoz Dec 31, 2024

russoz left a comment

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

russoz Jan 2, 2025

		description: The return value provided by proxmox_backup_info.
		type: dict

	description: backup type
	description: The type of the backup.

	- If vm_id is defined, the returned list will contain backup jobs that have been parsed and filtered based on vm_id value.
	- If defined, the returned list will contain backup jobs that have been parsed and filtered based on O(vm_id) value.

	- If vm_name is defined, the returned list will contain backup jobs that have been parsed and filtered based on vm_name value.
	- If defined, the returned list will contain backup jobs that have been parsed and filtered based on O(vm_name) value.

	description: 1 if backup is enabled else 0.
	description: V(1) if backup is enabled else V(0).

Add module proxmox_backup_info #9437

Are you sure you want to change the base?

Add module proxmox_backup_info #9437

Conversation

raoufnezhad commented Dec 28, 2024 • edited by felixfontein Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

This comment was marked as outdated.

ansibullbot commented Dec 28, 2024

russoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raoufnezhad commented Dec 28, 2024 •

edited by felixfontein

Loading