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

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

On 12/10/24 14:19, Lou.Lecrivain@wdz.de wrote:
> Hello Hannes,
>
> Thank you also very much for the feedback, I appreciate it. It's my first time writing Perl and contributing to Proxmox :) I was initially in contact with your colleague S. Hanreich, who told me to start with the Netbox plugin. Given how significant the differences are in the end, I guess it would be easier to build from the base plugin, indeed.
>
> I'll make the necessary changes (code + test + documentation) plus squash and edit a few of these commits and will send you a v2 as soon as possible.
>
> I just have a few questions that I'd like an answer to, namely:
>
> - Is the IPAM IP range support mandatory? Nautobot doesn't support IP ranges natively, only prefix allocations. So far, I have a workaround but it's not really clean... What would be best? ditch the workaround and "die", or keep it?
We have discussed this internally and would like to see IP range support 
implemented as well. It is a pity that the nautobot API does not provide 
a specific endpoint for this, but your workaround seems good enough.
> - We also have a noop/early return in del_subnet, inherited from Netbox Plugin (doesn't actually delete the subnet in the IPAM). Do we agree that the desired operation would to be to error if subnet is not empty? cf:
> https://git.proxmox.com/?p=pve-network.git;a=blame;f=PVE/Network/SDN/Ipams/NetboxPlugin.pm;hb=70b035064290a014759ce62e0093df00cd7d62fe#l69
IMO yes, i'd prefer some error message telling me that it was not 
possible to remove the Subnet or IP (currently the same behavior) 
instead of just not deleting it.
> MfG
> --
> Lou Lecrivain
> WDZ GmbH
>
> ________________________________________
> De : Hannes Dürr<h.duerr@proxmox.com>
> Envoyé : mardi 10 décembre 2024 10:32
> À : Proxmox VE development discussion<pve-devel@lists.proxmox.com>
> Cc : Lecrivain, Lou (WDZ)<Lou.Lecrivain@wdz.de>
> Objet : [!!ACHTUNG extern!!] - Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
>   
> 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