On 12/10/24 14:19, [email protected] 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<[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
_______________________________________________
pve-devel mailing list
[email protected]
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
