Re: [pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments

["Max Carrara" <m.carrara@proxmox.com>]

On Wed Jul 17, 2024 at 6:02 PM CEST, Thomas Lamprecht wrote:
> Am 17/07/2024 um 11:39 schrieb Max Carrara:
> > These have been around since 2012 - suffice to say they're not needed
> > anymore.
>
> That's really not a good argument though? Just because nobody checked
> those closely for a long time it does not mean that they became
> magically irrelevant.
>
> Look, it can be (and probably _is_) fine to remove them, but stating
> that this is fine just because they were not touched since a while is a
> rather dangerous tactic. Someone had some thoughts when adding this,
> they might be still relevant or not, but it's definitively *not*
> "suffice to say" that they aren't just because they have been around
> since 2012, (iSCSI) portals and local storage still exist and are not
> working really different compared to 12 years ago.
>
> The node restriction FIXME comment can e.g. be removed as we delete any
> such restriction in "parse_config", mentioning that as a reason would
> make doing so fine, saying "it's old and unchanged" doesn't.
>
> The storage portal one could be argued with not being defined elsewhere
> and all use cases being covered by pve-storage-portal-dns, so removing
> it won't hurt, especially as we can always recover it from history.
>
> I think your intention quite surely matched those and meant well, but
> removing something just because it's old is on its own IMO a bit of a
> red flag, so one should get too used to that argumentation style even
> if it's for removing comments, or other stuff that won't change semantics.

I completely agree with you, I probably should've stated a better reason
there. IIRC I removed those two things for a valid reason, but because
the commit was made a while ago, I'm not actually sure anymore what they
were exactly. I guess this proves your point. ;)

In a future RFC / Series, this will definitely be updated. Thanks for
pointing that out.

>
> Anyhow, do not let this demotivate you from your clean-up efforts, they
> are still quite appreciated.
> While removing dead code is good, the argumentation behind should be
> sound, and I only write this long tirade (sorry) as we got bitten by
> some innocent looking changes stemming from a similar argumentation in
> the past.

No worries, no offense taken here - I really appreciate comment.
Sometimes these things do need to be pointed out, because e.g. for me
personally it just wasn't on my radar that a commit like this could
become a tough to debug issue in case things go south. That's probably
because I've never had to deal with debugging such a thing myself.

So again, no worries, I appreciate it!



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