public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Lou Lecrivain via pve-devel <pve-devel@lists.proxmox.com>
To: <h.duerr@proxmox.com>, <pve-devel@lists.proxmox.com>
Cc: Lou.Lecrivain@wdz.de
Subject: Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
Date: Tue, 10 Dec 2024 13:19:41 +0000	[thread overview]
Message-ID: <mailman.119.1733836792.332.pve-devel@lists.proxmox.com> (raw)
In-Reply-To: <9553725a-a0a4-44e4-87f6-cc5d2d347424@proxmox.com>

[-- Attachment #1: Type: message/rfc822, Size: 15642 bytes --]

From: <Lou.Lecrivain@wdz.de>
To: <h.duerr@proxmox.com>, <pve-devel@lists.proxmox.com>
Subject: RE: Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
Date: Tue, 10 Dec 2024 13:19:41 +0000
Message-ID: <BE1P281MB2116C1CA8BA34E5C86FA59D9853D2@BE1P281MB2116.DEUP281.PROD.OUTLOOK.COM>

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 <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


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

  reply	other threads:[~2024-12-10 13:19 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-27 16:17 Lou Lecrivain via pve-devel
2024-12-10  9:32 ` Hannes Dürr
2024-12-10 13:19   ` Lou Lecrivain via pve-devel [this message]
     [not found]   ` <BE1P281MB2116C1CA8BA34E5C86FA59D9853D2@BE1P281MB2116.DEUP281.PROD.OUTLOOK.COM>
2024-12-12 15:42     ` Hannes Dürr

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=mailman.119.1733836792.332.pve-devel@lists.proxmox.com \
    --to=pve-devel@lists.proxmox.com \
    --cc=Lou.Lecrivain@wdz.de \
    --cc=h.duerr@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal