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

From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Max Carrara <m.carrara@proxmox.com>
Subject: Re: [pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments
Date: Wed, 17 Jul 2024 18:02:46 +0200	[thread overview]
Message-ID: <d5bb00b0-c85d-4415-a8d8-8606670f296b@proxmox.com> (raw)
In-Reply-To: <20240717094034.124857-2-m.carrara@proxmox.com>

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.

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.

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