Re: [pve-devel] [PATCH installer v2 16/17] fix #5536: post-hook: add utility for sending notifications after auto-install

From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Aaron Lauterer <a.lauterer@proxmox.com>,
	Christoph Heiss <c.heiss@proxmox.com>
Subject: Re: [pve-devel] [PATCH installer v2 16/17] fix #5536: post-hook: add utility for sending notifications after auto-install
Date: Wed, 24 Jul 2024 10:24:55 +0200	[thread overview]
Message-ID: <d360b143-d97b-437a-98df-7327961bd590@proxmox.com> (raw)
In-Reply-To: <c5b5a1f9-128f-4bba-b75f-e891287c7961@proxmox.com>

Am 23/07/2024 um 16:57 schrieb Aaron Lauterer:
> There are quite a few preparation changes in other sub-crates 
> (auto-installer, installer-common).
> I've only gotten through them for now and haven't looked at the actual 
> post-hook crate stuff.
> 
> Wouldn't it be nicer to split the preparation patches into their own 
> commit? It would make the patch smaller and the history would be a bit 
> clearer as we don't mix adding the post-hook crate with all the 
> preparation that is needed.

This isn't a binary thing, sometimes it can be nicer to have some
preparatory stuff up-front, most often if it's not only affecting new code
and code changes limited to the thing a patch wants to do, otherwise it can
also be nice to see what is needed to make it work, especially as often
preparatory patches cannot be reviewed in isolation anyway and one needs to
look into the patches that they're preparing for anyway to actually be able
to determine if the thing as a whole makes sense the way it's implemented,
split and used.

As mentioned, this is often subjective, both have some advantages and
disadvantages, IMO the best heuristic is the mentioned "how much is existing
code impacted and how much is this related to the semantic thing the patch
wants to do", all else is a bit of a balance that different reviewers might
also disagree a bit with, so it's also fine to reject a split or merge if
you, as patch author, really think it would make it worse – ideally with
some argumentation and avoiding hard lines to avoid "kerfuffle" or getting
less reviews in the future ;-)

unrelated and definitively not only applying just to you, only as a
gentle reminder: I would like to see that replies on the mailing list get
unrelated stuff trimmed out and use of inline/bottom posting, that makes
it quicker to read and reply and also to digest (longer) threads.

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