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

Add Ciena driver #570

Merged
merged 3 commits into from Dec 17, 2023
Merged

Add Ciena driver #570

merged 3 commits into from Dec 17, 2023

Conversation

jgroom33
Copy link
Contributor

Also alphabetize the lists

Copy link
Contributor

@einarnn einarnn left a comment

Choose a reason for hiding this comment

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

In general it's not clear to me what this device handler adds. A device handler is typically implemented if there is something special that is needed. For example, the IOS XE device handler adds a save_config operation:

https://github.com/ncclient/ncclient/blob/master/ncclient/devices/iosxe.py#L36-L39

This Ciena device handler doesn't seem to add any custom behaviour...?

Comment on lines 13 to 16
def get_capabilities(self):
return [
"urn:ietf:params:netconf:base:1.0",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This has the effect of restricting what capabilities are offered by ncclient to a Ciena device. Is that your intention? If so, it is not actually necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will remove.

@jgroom33
Copy link
Contributor Author

jgroom33 commented Sep 4, 2023

No additional capabilities are required. The driver is necessary because the device does not support a namespace (nc) on the base namespace. So, this code is necessary:
def get_xml_base_namespace_dict(self):
return {None: BASE_NS_1_0}

I attempted to use the default driver and override the namespace of the root, but could not figure it out. It seems the implementation of this library in Ansible provides limited ability to override any methods until some driver is specified.

@jgroom33
Copy link
Contributor Author

jgroom33 commented Sep 4, 2023

Several of the drivers in this repo seem to require the namespace modification.
As an alternative to creating another vendor specific driver, I think there should be another generic driver that only has the namespace modification.
Do you think this is a better approach?

@einarnn
Copy link
Contributor

einarnn commented Oct 16, 2023

@jgroom33, apologies for long delay. Having a single base driver that allows a vendor-specific driver to add in just namespace modifications may make sense. But it probably should be a per-vendor class ultimately as non-vendor engineers would likely not know what the vendor-specific namespace tweaks would need to be.

@jgroom33
Copy link
Contributor Author

@einarnn

For reference, The following drivers implement the null namespace for BASE_NS_1_0:

  • alu
  • hpcomware
  • ericsson
  • huawei
  • h3c
  • sros
  • huaweiyang

I'm not familiar with the other devices, but the only reason I opened this PR for the Ciena device is to null the BASE_NS_1_0 namespace. I suspect it might be similar for other vendors.

What I'm proposing is one of these options:

  1. delete this PR. Open a new PR with a new driver Generic_BASE_NS_1_0 that handles the namespace nullify
  2. delete this PR. update the generic driver so it nullifies BASE_NS_1_0
  3. Merge this PR

What would be your preference? I'm good with any of the above.

@einarnn
Copy link
Contributor

einarnn commented Dec 17, 2023

@jgroom33, sorry for delay. I will merge this PR.

@einarnn einarnn merged commit f8207de into ncclient:master Dec 17, 2023
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.

None yet

2 participants