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

Device Config backup workflow module #382

Merged

Conversation

sonalideepthi777
Copy link

@sonalideepthi777 sonalideepthi777 commented Sep 24, 2024

Description

Update the Device backup config workflow module to support for replacement of APIs, and improved functionality with version-based routing.

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.

Screenshots (if applicable)

Notes to Reviewers

@@ -394,6 +398,11 @@ class Device_configs_backup(DnacBase):
def __init__(self, module):
super().__init__(module)
self.skipped_devices_list = []
self.payload = module.params
self.keymap = {}
self.dnac_version = int(self.payload.get(
Copy link
Owner

Choose a reason for hiding this comment

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

setting dnac version is already done in dnac.py.. Can you check?

https://github.com/madhansansel/dnacenter-ansible/blob/main/plugins/module_utils/dnac.py
self.payload = module.params
self.dnac_version = int(self.payload.get("dnac_version").replace(".", ""))
# Dictionary to store multiple versions for easy maintenance and scalability
# To add a new version, simply update the 'dnac_versions' dictionary with the new version string as the key
# and the corresponding version number as the value.
self.dnac_versions = {
"2.3.5.3": 2353,
"2.3.7.6": 2376,
"2.2.3.3": 2233
# Add new versions here, e.g., "2.4.0.0": 2400
}

    # Dynamically create variables based on dictionary keys
    for version_key, version_value in self.dnac_versions.items():
        setattr(self, "version_" + version_key.replace(".", "_"), version_value)

Copy link
Author

Choose a reason for hiding this comment

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

Removed

site_exists = True
else:
self.log("No response received from the 'get_site' API call.", "WARNING")
response = self.get_site(site_name)
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need this API at all. Check get_site_id API and use it.. if you need site_exists also, then change the get_site_id API

Copy link
Author

Choose a reason for hiding this comment

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

Removed that API and directly using get_site_id

Copy link
Owner

Choose a reason for hiding this comment

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

We can directly use get_site_id rite?
Why do we use get_site first, and use get the site id from the response?

Copy link
Author

Choose a reason for hiding this comment

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

I removed and using get_site_id directly

Copy link
Owner

Choose a reason for hiding this comment

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

Can you update the code?

Copy link
Owner

Choose a reason for hiding this comment

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

We still don't need this API. What is the purpose of the API?

Copy link
Author

Choose a reason for hiding this comment

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

Updated

self.status = "failed"
self.msg = ("An exception occurred: Site '{0}' does not exist in the Cisco Catalyst Center.".format(site_name))
self.result['response'] = self.msg
self.log(self.msg, "ERROR")
self.check_return_status()

return (site_exists, site_id)

def get_device_ids_from_site(self, site_name, site_id):
Copy link
Owner

Choose a reason for hiding this comment

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

Move this API to dnac.py and make it common....

Copy link
Author

Choose a reason for hiding this comment

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

Moved, I added two functions in dnac.py: one for getting device IDs from a site and another for retrieving the IP address from a device id.

self.log("Reachable devices not found at Site: {0}".format(site_name), "WARNING")
self.msg = "No reachable devices found at Site: {0}".format(site_name)
self.update_result("ok", False, self.msg, "INFO")
self.module.exit_json(**self.result)
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to exit from the module if devices not found at site?

Copy link
Author

Choose a reason for hiding this comment

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

If the device ID is not available, then the IP address cannot be retrieved in this task

@madhansansel madhansansel merged commit c6c188c into madhansansel:main Sep 26, 2024
4 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