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.

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

Added disk selection and exclusion of deactivated disks #87

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GMasta2000
Copy link

I have transferred the changes that I made in 4.3 for disk filtering and additionally added default values for these parameters in config.py.

Copy link
Contributor

@kobihk kobihk left a comment

Choose a reason for hiding this comment

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

@GMasta2000 Thanks for creating this PR for the master branch
to fix issue #86

backup.py Outdated
@@ -333,6 +353,32 @@ def main(argv):

vm = vm[0]

# Get the Attachments Disk
disk_attachments = vms_service.vm_service(vm.id).disk_attachments_service().list()
disksBackup = []
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the python convention
https://peps.python.org/pep-0008/#function-and-variable-names
like:

Suggested change
disksBackup = []
disks_backup = []

backup.py Outdated
# Get the Attachments Disk
disk_attachments = vms_service.vm_service(vm.id).disk_attachments_service().list()
disksBackup = []
BootableOnly = config.get_bootable_only()
Copy link
Contributor

Choose a reason for hiding this comment

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

same here ^^

Suggested change
BootableOnly = config.get_bootable_only()
bootable_only = config.get_bootable_only()

backup.py Outdated
Comment on lines 378 to 380
# print disk_attachment.id
# break
# print disks_id[0].__dict__
Copy link
Contributor

Choose a reason for hiding this comment

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

if this code is not necessary maybe you can remove it

Suggested change
# print disk_attachment.id
# break
# print disks_id[0].__dict__

backup.py Outdated

for disk_attachment in disksBackup:
if disk_attachment.bootable == True:
logger.info("Finding bootable disk: %s", disk_attachment.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it will be better to use the new python3 format like:

Suggested change
logger.info("Finding bootable disk: %s", disk_attachment.id)
logger.info(f"Finding bootable disk: {disk_attachment.id}")

IMHO it looks better and more readable.

backup.py Outdated
if disk_attachment.bootable == True:
logger.info("Finding bootable disk: %s", disk_attachment.id)
else:
logger.info("Finding disk: %s", disk_attachment.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

same ^^

Suggested change
logger.info("Finding disk: %s", disk_attachment.id)
logger.info(f"Finding disk: {disk_attachment.id}")

# Backup deactivated disks, set to False to skip deactivated disks backup (Recommend).
# This True - causes an error deleting a snapshot of the VM when the state is on.
with_disks_deactivated=False
# an array of exclude disks id,exampel ["6d811aa25-52a1-45c3-9f40-1c7c868182c9","6e78d7c2-5786-4dz2-a966-fd308c23f08e"]
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Suggested change
# an array of exclude disks id,exampel ["6d811aa25-52a1-45c3-9f40-1c7c868182c9","6e78d7c2-5786-4dz2-a966-fd308c23f08e"]
# an array of exclude disks id, example ["6d811aa25-52a1-45c3-9f40-1c7c868182c9","6e78d7c2-5786-4dz2-a966-fd308c23f08e"]

@GMasta2000
Copy link
Author

@kobihk Thanks, I changed the variables.

Copy link
Contributor

@kobihk kobihk left a comment

Choose a reason for hiding this comment

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

Thank you @GMasta2000

@kobihk
Copy link
Contributor

kobihk commented Sep 19, 2022

Hi @wefixit-AT
Could you please CR this PR?
Thanks in advance,
Kobi

@kobihk
Copy link
Contributor

kobihk commented Oct 19, 2022

Hi @wefixit-AT
Could you please take a look at this PR?
Thanks in advance,
Kobi

@kobihk
Copy link
Contributor

kobihk commented Oct 30, 2022

Hi @wefixit-AT ^^???

richard-tns added a commit to richard-tns/oVirtBackup that referenced this pull request Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants