Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM

["Hannes Dürr" <h.duerr@proxmox.com>]

Thanks for contributing and sending the patch series, we really 
appreciate it!

At first glance it looks quite good, I have a few suggestions for changes:

*  The plugin is based on the Netbox plugin, I would suggest changing it
     to the base plugin. I know Nautobot is a fork of Netbox, but this
     dependency doesn't make things easier in my eyes, unless I'm missing
     something, please let me know.

*  The commit history is currently a bit unnecessarily long and does not
     build up well. What I mean is:
     - in 1/12 you build the plugin and copy/paste some stuff into it
     - in 3/12 you delete everything for now
     -> just leave it out
     or
     - in 6/12 you introduce a typo in an url
     - in 7/12 you fix the typo
     Such changes don't need to be committed in the first place,
     so you don't need to fix them 2 commits later.

*  The commit messages could be a bit longer and more explanatory for
     coming changes: e.g. “api endpoint change no longer needed” -> Why is
     the endpoint change no longer needed? Is there more background
     information on this, e.g. a link?

*  Comments can be helpful to make complex code easier to understand
     after a long time. Comments like '#helper' or '#impl' are not really
     necessary in my opinion. I know our codebase already contains such
     comments elsewhere, but I think we can leave them out here :)

*  Apart from the tests, the documentation (pve-docs/pvesdn.adoc) is also
     missing.

On 11/27/24 17:17, Lou Lecrivain via pve-devel wrote:
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel