create new proxmox_backup_schedule module #9592
Conversation
|
@raoufnezhad this PR contains the following merge commits: Please rebase your branch to remove these commits. |
ansibullbot
added
merge_commit
felixfontein
added
check-before-release
There was a problem hiding this comment.
hi @raoufnezhad thanks for your contribution
There is a number of comments - some changes that must be done, others are just suggestions, and others I am trying to understand what's going on.
| --- | ||
| module: proxmox_backup_schedule | ||
|
|
||
| short_description: Scheduling VM Backups and Removing it. |
There was a problem hiding this comment.
| short_description: Scheduling VM Backups and Removing it. | |
| short_description: Scheduling VM backups and removing them |
|
|
||
| version_added: 10.3.0 | ||
|
|
||
| description: The proxmox_backup_schedule module modify backup jobs such as set or delete vmid. |
There was a problem hiding this comment.
| description: The proxmox_backup_schedule module modify backup jobs such as set or delete vmid. | |
| description: The module modifies backup jobs such as set or delete C(vmid). |
| backup_action: | ||
| description: | ||
| - If V(update_vmid), the module will update backup job with new VM ID. | ||
| - If V(delete_vmid), the module will remove the VM ID from all backup jobs where the VM ID has existed. | ||
| required: true | ||
| type: str |
There was a problem hiding this comment.
If only these two options are valid, make them choices. Also a little fix on the indentation:
| backup_action: | |
| description: | |
| - If V(update_vmid), the module will update backup job with new VM ID. | |
| - If V(delete_vmid), the module will remove the VM ID from all backup jobs where the VM ID has existed. | |
| required: true | |
| type: str | |
| backup_action: | |
| description: | |
| - If V(update_vmid), the module will update backup job with new VM ID. | |
| - If V(delete_vmid), the module will remove the VM ID from all backup jobs where the VM ID has existed. | |
| required: true | |
| choices: [update_vmid, delete_vmid] | |
| type: str |
| description: The backup job ID. | ||
| required: false | ||
| type: str | ||
| backup_action: |
There was a problem hiding this comment.
This kind of parameter that controls the action being taken by the module, in Ansible is usually named state. I do not see that as mandatory, but you might want to consider it.
| if vm_id in vms_id: | ||
| bksetInfo = [] | ||
| return bksetInfo | ||
| else: | ||
| bk_id_vmids = bk_id_info['vmid'] + ',' + str(vm_id) | ||
| self.set_vmid_backup(backup_id, bk_id_vmids) | ||
| return True |
There was a problem hiding this comment.
This seems odd: if vm_id is in the list it returns a (empty) list object, else it returns a boolean object (True). The return type of the method should be consistent.
| backup_schedule: | ||
| description: | ||
| - If V(update_vmid), the backup_schedule will return True after adding the VM ID to the backup job. | ||
| - If V(delete_vmid), the backup_schedule will return a list of backup job IDs where the VM ID has existed after removing it. | ||
| returned: always, but can be empty | ||
| type: dict |
There was a problem hiding this comment.
Firstly, not a good idea to have a RV that results in different types - trust me, I have done that, and still needs fixing (module xfconf).
Secondly, you are declaring the type as dict, which neither of the returned values are.
I would suggest you make two different RVs, one for the update case, another for the delete case, then make them of the right type.
| if vm_name: | ||
| vm_id = proxmox.vmname_2_vmid(vm_name) | ||
|
|
||
| if (backup_action == 'update_vmid'): |
There was a problem hiding this comment.
redundant:
| if (backup_action == 'update_vmid'): | |
| if backup_action == 'update_vmid': |
| if (backup_action == 'update_vmid'): | ||
| result['backup_schedule'] = proxmox.backup_update_vmid(vm_id, backup_id) | ||
|
|
||
| if (backup_action == 'delete_vmid'): |
There was a problem hiding this comment.
redundant:
| if (backup_action == 'delete_vmid'): | |
| if backup_action == 'delete_vmid': |
| for backupItem in backupList: | ||
| vmids = list(backupItem['vmid'].split(',')) | ||
| if vm_id in vmids: | ||
| if len(vmids) > 1 : |
There was a problem hiding this comment.
| if len(vmids) > 1 : | |
| if len(vmids) > 1: |
@felixfontein I would have thought this was something the validation would pick up. Any thoughts on that?
| description: | ||
| - The name of the Proxmox VM. | ||
| - Mutually exclusive with O(vm_id). | ||
| required: false |
There was a problem hiding this comment.
please remove the required: false from the docs, it is redundant
SUMMARY
This contribution proposes to add a module called proxmox_backup_schedule to the Ansible community.general collection. The module would schedule VM backups and removing it in a Proxmox VE cluster.
ISSUE TYPE
New Module/Plugin Pull Request
COMPONENT NAME
proxmox_backup_schedule
ADDITIONAL INFORMATION
The proxmox_backup_schedule module modify backup jobs such as set or delete vmid.