Review feedback

[glennmatthews]

Docs

• Can we flesh out the intro paragraph to more than just "A plugin for Nautobot"? Provide some details about what this plugin is for and why you might want it.
• Having "Home" and "Readme" as separate pages seems less than ideal to me - can they be merged in some way?
• "Build Status" badges don't appear to render in GitHub pages:
  • 
• "TODO: /slash command screenshot" (README) and "Screenshots: TODO" (rendered docs)
• Please review the "Local Poetry Development Environment" versus "Docker Development Environment" sections for correctness and completeness
  • Installing invoke and poetry is a prerequisite for both environments, yet it's only mentioned in the Docker section?
  • Creating a creds.env is a prerequisite for both environments, yet it's only mentioned in the Local section?
  • The local development example invoke.yml: if you're running with local: true I would expect that compose_files should be unnecessary since you're not using docker?
  • Since the Docker environment is simpler to start with than the local environment, perhaps it should be listed first?
• What's the significance of GETTING_STARTED.md - should it be linked or referenced from either README.md or from the rendered docs, or is it redundant in some way?
• Why do the GitHub Pages docs look to have completely different content than the files in docs/ -- changelog.md, plugin.md, and index.rst all appear to have content completely different from the rendered docs?

Development Environment

• Consider moving from Python 3.6 to 3.7 as the default due to CVE-2021-23727 (Celery vulnerability only fixed in 3.7 and later)
• What's the significance of custom_logging_levels.py - why not just set the logging directly in development/nautobot_config.py?
• There are a lot of settings in development/nautobot_config.py that are just matching Nautobot defaults as of 1.1.x. This may just be a cookiecutter thing but this file could be pruned substantially if desired.

Code

• Given that nautobot_chatops and nautobot_ssot use lowercase settings in PLUGINS_CONFIG (enable_slack, hide_example_jobs, etc.) it would probably be good for consistency to also use lowercase settings for this plugin, rather than uppercase ones.
  • Documentation is missing for the DEFAULT_DEVICE_ROLE and DEFAULT_DEVICE_STATUS config settings.
  • Documentation is missing for the DEFAULT_INTERFACE_* config settings as well.
• template_content.py has a hard-coded ipfabric.demo.networktocode.com that probably should be replaced with the appropriate config variable? Looks like there may also be some hard-coded query parameters in that URL to review.
  • You also have a hard-coded URL in the diagram.html template file.
• If this is building atop the existing nautobot-chatops-ipfabric plugin, I'm surprised to see that this plugin is registering a top-level ipfabric command of its own - how does this coexist with the other plugin, or does it clobber it?
  • Is it possible to use this plugin without chatops? If so the nautobot-chatops and nautobot-chatops-ipfabric should be optional packaging dependencies, but right now they're declared as required in pyproject.toml.
• Dependency on netutils should be updated to ^1.0 as otherwise this plugin won't be usable with Nautobot 1.2.
• nautobot_ssot_ipfabric/api folder can be removed if you don't have or plan to have any REST API for this plugin.
• Same for nautobot_ssot_ipfabric/migrations/ since you have no models to migrate.
• Looks like the sys_id and pk attributes on your diffsync models are unused and can be removed
• Interface.create() and Interface.update() are calling device_obj.validated_save() - this doesn't appear to be necessary?
• For my own education, what does tonb mean?
• Is there a reason you're not using get_or_create() in your create_site and create_status functions, given that you're using it in the other create_* functions?
• I'm slightly concerned about the performance of the ipfabric adapter if you have a large number of devices with a large number of interfaces - you're calling load_device_interfaces once per device, and in that function, every time it's called you're iterating over all interfaces to find the ones applicable to the device in question. It might be more efficient to preprocess the get_interface_inventory response once to map each interface to its associated device so that you don't have to iterate over the entire list every time.
• Nautobot adapter, line 59 - shouldn't this be something like self.get(self.interface, f"{interface_record.name}__{interface_record.device.name}")? I don't see where self.name is coming from.
  • Similarly, at line 114, shouldn't that be self.get(self.vlan, f"{vlan_record.name}__{vlan_record.site.name}")?
  • Actually, for all of the load_* functions, why do you even need to try to load an existing record at all? The whole point of these functions is to load fresh data, right? There should never be an existing record matching what you're processing, I think?
• It'd be good if the adapter test cases actually checked correctness of the loaded data values, rather than just simple hasattr checks