Code Review Feedback about nautobot-app-firewall-models HOT 4 CLOSED

• Need to migrate from Travis CI to GitHub Actions
• README.md still says the minimum Nautobot version is 1.1.0, but it's actually 1.3.0 correct?
• PolicyDeivceM2MNestedSerializer should be PolicyDeviceM2MNestedSerializer
• Noted in nautobot_firewall_models/api/serializers.py, maybe also elsewhere - it's preferable to disable pylint rules by name rather than by number (pylint: disable=too-many-ancestors not pylint: disable=R0201)
• In migration 0002_custom_status, is the relative path to nautobot_firewall_models/migrations/services.yml valid in a production installation? Or should it be something like os.path.join(os.path.abspath(file), "nautobot_firewall_models/migrations/services.yml")?
• As a best practice, would be good to define help_text on all model fields to make them more self-explanatory.
• Would be good to provide more verbose docstrings on model classes - """FQDN model""" for example isn't very informative/helpful.
• Unless there's a requirement to limit name/slug fields to 50 characters, it would be better to use 100 characters for consistency with Nautobot core.
• Should FQDN.name allow up to 254 characters (assuming name is the fqdn value, which I'm not 100% sure on)?
• Is there a pattern as to which models include a slug field and which ones do not?
• For models that have a slug, it could be preferable to have their absolute (detail) URL use the slug rather than the PK as the URL parameter.
• Object detail page templates should extend the generic object_detail.html instead of base.html, which should allow a significant reduction in boilerplate
• This should also allow the test_has_advanced_tab view tests to be enabled and pass.
• """I hate writing docs strings easter egg.""" - caught you! :-)
• Filtersets can inherit from nautobot.extras.filters.NautobotFilterSet to reduce boilerplate
• What's the "TODO: Refactor" for PolicyRuleFilterSet?
  - [ ] Any plan to support bulk-import for any of these models?

from nautobot-app-firewall-models.

Only caveat to bulk-import is going to be the amount of M2M fields I feel it would be better served through integrating ntc-templates or capirca to parse and import. That would come post initial release.

from nautobot-app-firewall-models.

Only caveat to bulk-import is going to be the amount of M2M fields I feel it would be better served through integrating ntc-templates or capirca to parse and import. That would come post initial release.

on the import side, I investigated if Capirca had some mechanism, and it has not. My idea was to use Batfish, to load the config and import the firewall rules from it.

from nautobot-app-firewall-models.

Only caveat to bulk-import is going to be the amount of M2M fields I feel it would be better served through integrating ntc-templates or capirca to parse and import. That would come post initial release.

That's a great point. Bulk-import is definitely not a hard requirement from my end of things, and I can absolutely see your point that there may be other, better ways to do this for this particular data model.

from nautobot-app-firewall-models.