--- Begin Message ---
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 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
MfG
--
Lou Lecrivain
WDZ GmbH
________________________________________
De : Hannes Dürr <[email protected]>
Envoyé : mardi 10 décembre 2024 10:32
À : Proxmox VE development discussion <[email protected]>
Cc : Lecrivain, Lou (WDZ) <[email protected]>
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
> [email protected]
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
--- End Message ---
_______________________________________________
pve-devel mailing list
[email protected]
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
