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

create new proxmox_backup_schedule module #9592

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

create new proxmox_backup_schedule module #9592

wants to merge 3 commits into from
+469 −1

Conversation

raoufnezhad
Copy link
Contributor

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.

@ansibullbot
Copy link
Collaborator

@raoufnezhad this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html new_contributor Help guide this first time contributor new_plugin New plugin labels Jan 21, 2025
@felixfontein 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 Jan 21, 2025
russoz
russoz reviewed Jan 23, 2025
View reviewed changes
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.

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.

plugins/modules/proxmox_backup_schedule.py Outdated
---
module: proxmox_backup_schedule
short_description: Scheduling VM Backups and Removing it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
short_description: Scheduling VM Backups and Removing it.
short_description: Scheduling VM backups and removing them

plugins/modules/proxmox_backup_schedule.py Outdated
version_added: 10.3.0
description: The proxmox_backup_schedule module modify backup jobs such as set or delete vmid.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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).

plugins/modules/proxmox_backup_schedule.py Outdated
Comment on lines 43 to 46
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

If only these two options are valid, make them choices. Also a little fix on the indentation:

Suggested change
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

plugins/modules/proxmox_backup_schedule.py
description: The backup job ID.
required: false
type: str
backup_action:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

plugins/modules/proxmox_backup_schedule.py Outdated
Comment on lines 152 to 156
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

plugins/modules/proxmox_backup_schedule.py Outdated
Comment on lines 95 to 100
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

plugins/modules/proxmox_backup_schedule.py Outdated
if vm_name:
vm_id = proxmox.vmname_2_vmid(vm_name)

if (backup_action == 'update_vmid'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant:

Suggested change
if (backup_action == 'update_vmid'):
if backup_action == 'update_vmid':

plugins/modules/proxmox_backup_schedule.py Outdated
if (backup_action == 'update_vmid'):
result['backup_schedule'] = proxmox.backup_update_vmid(vm_id, backup_id)

if (backup_action == 'delete_vmid'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant:

Suggested change
if (backup_action == 'delete_vmid'):
if backup_action == 'delete_vmid':

plugins/modules/proxmox_backup_schedule.py Outdated
for backupItem in backupList:
vmids = list(backupItem['vmid'].split(','))
if vm_id in vmids:
if len(vmids) > 1 :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have hoped so too, maybe core made the pep8 check more lenient and it isn't caught anymore with newer ansible-test versions?

plugins/modules/proxmox_backup_schedule.py Outdated
description:
- The name of the Proxmox VM.
- Mutually exclusive with O(vm_id).
required: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove the required: false from the docs, it is redundant

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 26, 2025
@felixfontein
Copy link
Collaborator

Note that merging the other PR introduced two conflicts.

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed new_contributor Help guide this first time contributor labels Jan 27, 2025
@ansibullbot
Copy link
Collaborator

@raoufnezhad this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jan 27, 2025
@ansibullbot
Copy link
Collaborator

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

@ansibullbot ansibullbot added plugins plugin (any type) tests tests unit tests/unit and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 31, 2025
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger 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 31, 2025
@ansibullbot
Copy link
Collaborator

The test extra-docs failed with 3 errors:

plugins/modules/proxmox_backup_schedule.py:0:0: Unable to normalize proxmox_backup_schedule: return due to: 1 validation error for PluginReturnSchema
                                                return -> backup_schedule -> type:0:0:
                                                  Input should be 'any', 'bits', 'bool', 'bytes', 'complex', 'dict', 'float', 'int', 'json', 'jsonarg', 'list', 'path', 'sid', 'str', 'pathspec' or 'pathlist' (type=literal_error; expected='any', 'bits', 'bool', 'bytes', 'complex', 'dict', 'float', 'int', 'json', 'jsonarg', 'list', 'path', 'sid', 'str', 'pathspec' or 'pathlist'):0:0:

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/proxmox_backup_schedule.py:0:0: return-syntax-error: RETURN.backup_schedule.type: not a valid value for dictionary value @ data['backup_schedule']['type']. Got 'bool | list'

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/proxmox_backup_schedule.py:0:0: return-syntax-error: RETURN.backup_schedule.type: not a valid value for dictionary value @ data['backup_schedule']['type']. Got 'bool | list'

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/proxmox_backup_schedule.py:0:0: return-syntax-error: RETURN.backup_schedule.type: not a valid value for dictionary value @ data['backup_schedule']['type']. Got 'bool | list'

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/proxmox_backup_schedule.py:0:0: return-syntax-error: RETURN.backup_schedule.type: not a valid value for dictionary value @ data['backup_schedule']['type']. Got 'bool | list'

click here for bot help

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

@russoz russoz russoz left review comments

Assignees
No one assigned
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_plugin New plugin 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.
5 participants
@raoufnezhad @ansibullbot @felixfontein @russoz @mmayabi