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

Fix for unreachable devices, Ap devices and unmanaged devices backup #329

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

abimishr
Copy link
Collaborator

@abimishr abimishr commented Aug 5, 2024

Default Code Review Template

Description

Added the fix where devices being AP or being in unmanaged state or being in unreachable state, won't be going for the device config backup. For the same, I added a few conditions by looping through the device list retrieved via 'get device list' API.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • All the sanity checks have been completed and the sanity test cases have been executed

Ansible Best Practices

  • Tasks are idempotent (can be run multiple times without changing state)
  • Variables and secrets are handled securely (e.g., using ansible-vault or environment variables)
  • Playbooks are modular and reusable
  • Handlers are used for actions that need to run on change

Documentation

  • All options and parameters are documented clearly.
  • Examples are provided and tested.
  • Notes and limitations are clearly stated.

self.log("Length of the device list fetched from the API 'get_device_list' is {0}".format(str(device_list)), "INFO")
if len(device_list) == 0:
original_dev_len = len(device_list)
if original_dev_len == 0:
msg = "Couldn't find any devices in the inventory that match the given parameters."
Copy link
Owner

Choose a reason for hiding this comment

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

      msg = "No devices found in the inventory matching the given parameters."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

for dev_info in device_list:
if dev_info.get("collectionStatus") != "Managed":
msg = "Device backup of device with IP address {0} \
is not possible due to collection status not being in Managed state".format(dev_info.get("managementIpAddress"))
Copy link
Owner

Choose a reason for hiding this comment

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

format(ip_address)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

changed_dev_list.append(dev_info)

if len(changed_dev_list) == 0:
msg = "No device IDs got collected either due to devices \
Copy link
Owner

Choose a reason for hiding this comment

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

 ("No device IDs were collected because the devices are either Unified APs, "
  "not in the Managed state, or not reachable.")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

msg = "Couldn't find any devices in the inventory that match the given parameters."
self.log(msg, "CRITICAL")
self.module.fail_json(msg=msg)

dev_id_list = [id.get("id") for id in device_list]
changed_dev_list = []
Copy link
Owner

Choose a reason for hiding this comment

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

Can we say valid_devices instead of changed_dev_list?

If you feel 'changed_dev_list' makes more sense, then ignore this comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

valid_devices makes more sense

plugins/modules/device_configs_backup_workflow_manager.py Outdated Show resolved Hide resolved
self.log(msg, "CRITICAL")
self.module.fail_json(msg=msg)

dev_id_list = [id.get("id") for id in changed_dev_list]
Copy link
Owner

Choose a reason for hiding this comment

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

device_ids instead of dev_id_list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

self.module.fail_json(msg=msg)

dev_id_list = [id.get("id") for id in changed_dev_list]
dev_len = len(dev_id_list)
Copy link
Owner

Choose a reason for hiding this comment

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

valid_device_count instead of dev_len... as we count the number of valid devices

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed

self.module.fail_json(msg=msg)

dev_id_list = [id.get("id") for id in changed_dev_list]
dev_len = len(dev_id_list)
self.log("Device Ids list collected is {0}".format(dev_id_list), "INFO")
Copy link
Owner

Choose a reason for hiding this comment

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

Collected device IDs: {0}

self.log("Device Ids list collected is {0}".format(dev_id_list), "INFO")
self.log("Backup of {0} devices out of {1} devices is possible".format(dev_len, original_dev_len), "INFO")
return dev_id_list
Copy link
Owner

Choose a reason for hiding this comment

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

return device_ids

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

@madhansansel madhansansel merged commit 755930c into madhansansel:main Aug 6, 2024
3 checks passed
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.

2 participants