New provision workflow feature #62

syed-khadeerahmed · 2024-12-12T08:19:56Z

Description

Please include a summary of the changes and the related issue. Also, include relevant motivation and context.

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

…talyst-center-ansible-intg into new_provision_workflow_feature

madhansansel · 2024-12-16T15:23:11Z

plugins/modules/provision_workflow_manager.py

+                  to be validated.
+          - uuid (str): The UUID of the device to check for site assignment.
+        Returns:
+          - boolean:  True if the device is assigned to a site, False otherwise.


You are returning tuple..

- tuple: (bool, Optional[str]) - True and the site name if the device is assigned to a site. - False and None if not assigned or in case of an error.

fixed the code

madhansansel · 2024-12-16T15:38:22Z

plugins/modules/provision_workflow_manager.py

+
+            self.log("Response collected from the API 'get_site_assigned_network_device' {0}".format(site_response))
+            site_name = None
+            if site_response.get("response").get("siteNameHierarchy"):


site_api_response = self.dnac_apply['exec']( family="site_design", function='get_site_assigned_network_device', params={"id": uuid} )

Getting site_response in two places.. Also else is not required as we return from if statement.. We can log a message before returning..

site_response = site_api_response.get("response") if site_response: site_name = site_response.get("siteNameHierarchy") if site_name: self.log("Device with UUID {0} is assigned to site: {1}".format(uuid, site_name), "INFO") return True, site_name self.log("Device with UUID {0} is not assigned to any site.".format(uuid), "INFO") return False, None

fixed the code

madhansansel · 2024-12-16T17:24:41Z

plugins/modules/provision_workflow_manager.py

-                    self.log("Managed AP Location must be a floor", "CRITICAL")
-                    self.module.fail_json(msg="Managed AP Location must be a floor", response=[])
+        ap_locations = wireless_params[0].get("managedAPLocations")
+        if not (ap_locations and isinstance(ap_locations, list)):


Make it more readable..

if not ap_locations or not isinstance(ap_locations, list):

fixed the code

madhansansel · 2024-12-16T17:27:35Z

plugins/modules/provision_workflow_manager.py

+                self.floor_names.append(ap_loc)
+
+            if site_type == "building":
+                Bulding_name = ap_loc + ".*"


"building_name" Not "Building_name"

fixed the code

madhansansel · 2024-12-16T17:27:47Z

plugins/modules/provision_workflow_manager.py

+                floors = self.get_site(Bulding_name)
+
+                for item in floors['response']:
+


no need of new line

fixed the code

madhansansel · 2024-12-16T17:28:45Z

plugins/modules/provision_workflow_manager.py

+            site_type = self.get_site_type(site_name_hierarchy=ap_loc)
+            self.log("Checking site type for AP location '{0}', resolved type: '{1}'".format(ap_loc, site_type), "DEBUG")
+
+            if site_type not in ["floor", "building"]:


The site_type could be either floor or building or otherwise.. Can we simplify it?

if site_type == "floor": self.log("Adding '{0}' to floor names list".format(ap_loc), "DEBUG") self.floor_names.append(ap_loc) elif site_type == "building": self.log("Building site type detected for '{0}'. Retrieving floor details.".format(ap_loc), "DEBUG") building_name = ap_loc + ".*" floors = self.get_site(building_name) for item in floors['response']: if item.get('type') == 'floor': self.log("Floor found: '{0}' for building '{1}'".format(item['nameHierarchy'], ap_loc), "DEBUG") self.floor_names.append(item['nameHierarchy']) elif 'additionalInfo' in item: for additional_info in item['additionalInfo']: if 'attributes' in additional_info and additional_info['attributes'].get('type') == 'floor': self.log("Floor found in additionalInfo: '{0}' for building '{1}'".format(item['siteNameHierarchy'], ap_loc), "DEBUG") self.floor_names.append(item['siteNameHierarchy']) else: self.log("Invalid site type detected for '{0}'. Managed AP Location must be building or floor.".format(ap_loc), "CRITICAL") self.module.fail_json(msg="Managed AP Location must be building or floor", response=[]) self.log("Final list of floor names: {0}".format(self.floor_names), "DEBUG")

fixed the code

madhansansel · 2024-12-16T17:29:53Z

plugins/modules/provision_workflow_manager.py

+            self.module.fail_json(msg=msg, response=[])
+
+        self.floor_names = []
+        for ap_loc in self.validated_config.get("managed_ap_locations"):


self.log("Processing AP location: {0}".format(ap_loc), "DEBUG")

fixed the code

madhansansel · 2024-12-16T17:30:24Z

plugins/modules/provision_workflow_manager.py

+        self.floor_names = []
+        for ap_loc in self.validated_config.get("managed_ap_locations"):
+            site_type = self.get_site_type(site_name_hierarchy=ap_loc)
+            self.log("Checking site type for AP location '{0}', resolved type: '{1}'".format(ap_loc, site_type), "DEBUG")


Instead
self.log("Resolved site type for AP location '{0}': '{1}'".format(ap_loc, site_type), "DEBUG")

fixed the code

madhansansel · 2024-12-16T17:34:19Z

plugins/modules/provision_workflow_manager.py

+                    self.log(self.result['msg'], "INFO")
+                    return self
+
+                if self.status == 'failed':


Why do we check only for failed? What happens if status is exited? Do we print any message?

fixed the code

madhansansel · 2024-12-16T17:34:41Z

plugins/modules/provision_workflow_manager.py

+                    self.log("Exception occurred during 'delete_provisioned_devices': {0}".format(str(fail_reason)), "ERROR")
+                    self.msg = "Error in delete provisioned device '{0}' due to {1}".format(self.device_ip, str(fail_reason))
+                    self.log(self.msg, "ERROR")
+                    self.status = "failed"


What is the use of setting again to failed.. you compared it in line 1644..

the above self.status comes from the function - check_tasks_response_status but the below self.status is used to end the code as failed

syed-khadeerahmed added 8 commits December 4, 2024 16:14

provision bugs fixed

dd04f03

working on provision bugs

34d4ee6

Working on the fix

36817bf

bugs coding in progress

6352806

Merge branch 'main' of https://github.com/cisco-en-programmability/ca…

84723cc

…talyst-center-ansible-intg into new_provision_workflow_feature

bugs fixed

71cbe8c

bugs fixed

89bdeb6

bugs fixed

7cc5fac

syed-khadeerahmed requested a review from madhansansel as a code owner December 12, 2024 08:19

syed-khadeerahmed added 4 commits December 12, 2024 13:52

sanity fixed

dc27a9b

minor change in log

f4ae2e8

added new API to get the device site

8ccfd7a

added new API to get the device site

a9c2e08

madhansansel reviewed Dec 16, 2024

View reviewed changes

syed-khadeerahmed added 3 commits December 17, 2024 10:24

addressed review comments

97fb183

review comments fixed

c1ba82b

review comments fixed

3385f94

madhansansel approved these changes Dec 17, 2024

View reviewed changes

madhansansel merged commit 88b09a4 into cisco-en-programmability:main Dec 17, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New provision workflow feature #62

New provision workflow feature #62

syed-khadeerahmed commented Dec 12, 2024 •

edited

Loading

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

madhansansel Dec 16, 2024

syed-khadeerahmed Dec 17, 2024

		floors = self.get_site(Bulding_name)

		for item in floors['response']:

New provision workflow feature #62

New provision workflow feature #62

Conversation

syed-khadeerahmed commented Dec 12, 2024 • edited Loading

Description

Type of Change

Checklist

Ansible Best Practices

Documentation

Screenshots (if applicable)

Notes to Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syed-khadeerahmed commented Dec 12, 2024 •

edited

Loading