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

Changes for typos, fixed CIDR issue and added delete all feature #132

Merged
merged 4 commits into from
Feb 3, 2024

Conversation

abimishr
Copy link
Collaborator

@abimishr abimishr commented Feb 2, 2024

Problem description:

  1. CIDR based discovery should have Ip passed as IP/{prefix length} instead of passing prefix length separately.
  2. Coding error in the for loop of multi range discovery
  3. Deletion of all the discoveries at one go

Fix: For te fix we have added / based string formation, have taken care of for loop based issues and delete_all is a new parameter that can delete all the discoveries if someone tries to delete all the discoveries

Test cases: Have tested delete all, single deletion and CIDR based discovery

'elements': 'str'}
discovery_spec["discovery_type"] = {'type': 'str', 'required': True}

if (self.config[0].get("delete_all") is True and state == "deleted"):
Copy link
Owner

Choose a reason for hiding this comment

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

Line 370 we checked state is merged. If state is merged, then we do comparision in line 375 as well. Instead can we write below?

        if state == "merged":
            discovery_spec["ip_address_list"] = {'type': 'list', 'required': True,
                                                 'elements': 'str'}
            discovery_spec["discovery_type"] = {'type': 'str', 'required': True}
        else if state == deleted:
            if (self.config[0].get("delete_all") is True:
                self.validated_config = [{"delete_all": True}]
                 self.msg = "Sucessfully collected input for deletion of all the discoveries"
                 self.log(self.msg, "WARNING")
                 return self

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

if len(ip_address_list[0].split("/")) == 2:
ip_address_list = ip_address_list[0]
else:
ip_address_list = str(ip_address_list[0]) + "/" + str("30")
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 give a warning message? Say that prefix length is not given, hence taking 30 as default..

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

discovery_task_id = self.delete_exist_discovery(params=params)
complete_discovery = self.get_task_status(task_id=discovery_task_id)
self.result["changed"] = True
self.result['msg'] = "Discovery Deleted Successfully"
Copy link
Owner

Choose a reason for hiding this comment

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

Successfully deleted discovery.

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

@@ -981,7 +1011,7 @@ def main():
dnac_discovery.msg = "State {0} is invalid".format(state)
dnac_discovery.check_return_status()

dnac_discovery.validate_input().check_return_status()
dnac_discovery.validate_input(state=state).check_return_status()
Copy link
Owner

Choose a reason for hiding this comment

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

Any reasons for this
state=state ?

Copy link
Collaborator Author

@abimishr abimishr Feb 3, 2024

Choose a reason for hiding this comment

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

We are passing the state value here to use in the validation part

Copy link
Owner

Choose a reason for hiding this comment

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

but why do we need to assign state = state when we call the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will it work if I don't pass the state?

'elements': 'str'}
discovery_spec["discovery_type"] = {'type': 'str', 'required': True}

if state == "deleted":
Copy link
Owner

@madhansansel madhansansel Feb 3, 2024

Choose a reason for hiding this comment

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

If the 'state' is merged, there is no possibility for the state to be "deleted". Therefore, it is more efficient to use "elif" instead. By using two "if" statements, we unnecessarily perform the state comparison twice…

 if state == "merged":
            discovery_spec["ip_address_list"] = {'type': 'list', 'required': True,
                                                 'elements': 'str'}
            discovery_spec["discovery_type"] = {'type': 'str', 'required': True}
elif state == "deleted":

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

ip_address_list = ip_address_list[0]
else:
ip_address_list = str(ip_address_list[0]) + "/" + str("30")
self.log("Prefix length is not given, hence taking 30 as default", "WARNING")
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 add bit more detail?
self.log("CIDR notation is being used for discovery and it requires a prefix length to be specified, such as 1.1.1.1/24. As no prefix length was provided, it will default to 30.", "WARNING")

Copy link
Owner

Choose a reason for hiding this comment

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

elif discovery_type == "CIDR":
    if len(ip_address_list) == 1:
        cidr_notation = ip_address_list[0]

        if len(cidr_notation.split("/")) == 2:
            # If CIDR notation is used and prefix length is provided
            ip_address_list = cidr_notation
        else:
            # If CIDR notation is used but prefix length is not given, default to 30
            ip_address_list = f"{cidr_notation}/30"
            self.log("CIDR notation is being used for discovery and it requires a prefix length to be specified, such as 1.1.1.1/24. As no prefix length was provided, it will default to 30.", "WARNING")
    else:
        # Handle error if multiple IP addresses are provided for CIDR discovery
        self.preprocess_device_discovery_handle_error()

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

@madhansansel madhansansel merged commit ea58168 into madhansansel:main Feb 3, 2024
5 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