Am 09/02/2023 um 14:18 schrieb Aaron Lauterer:
> The `mkdir` option has two meanings[0][1] which are split up in `create-path`
> and `create-sub-dirs`.
>
> The `create-path` option decides if the path to the storage is
> automatically created or not.
> The `create-sub-dirs` options decides if the default directory
> structure (dump, images, ...) at the storage location is created.
>
> The `mkdir` option is still working but will trigger a warning in the
> logs.
>
> As a side effect, this also fixes #3214 because the `create-path` option
> is now run after the `is_mountpoint` check in the `activate_storage`
> method in DirPlugin.pm.
>
> [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046575.html
> [1] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046576.html
>
> Reviewed-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
> Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
> ---
> This is the continuation of quite the old patch series [2]. We never
> applied it prior to the release of Proxmox VE 7. Hopefully we can get it
> into v8 so that we can think about dropping the 'mkdir' parameter in v9.
>
> I added the T-b: Fabian Grünbichler [3] but feel free to remove it if
> you think that it has been too long ago :)
You added the R-b, not T-b, but that's actually the right one so OK. But you
added it first, but first should always be the authors S-o-b. I.e., trailers
should only be appended, not prefixed or the like.
But that's a nit and could be easily addressed on applying, and actually I
was close on doing so, but IMO it's a bit to late for this now, we should
rather add this early in the 8.x release cycle instead and then we could
actually think of removing it in 9.x - as it would be rather a to short
time span applying it now and dropping it for 9.0 already - especially as
dir based external plugins are affected by this too and dropping it might
even warrant an APIAGE reset.
Plus, I'd do the following changes on top:
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 6fc1f27..560bb62 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -35,14 +35,13 @@ sub properties {
type => 'string', format => 'pve-storage-path',
},
mkdir => {
- description =>
- "Deprecated, use the 'create-path' and " .
- "'create-sub-dirs' options instead.",
+ description => "Create the directory if it doesn't exist and
populate it with default sub-dirs."
+ ." NOTE: Deprecated, use the 'create-path' and
'create-sub-dirs' options instead.",
type => 'boolean',
default => 'yes',
},
- 'create-path' => {
- description => "Create the directory if it doesn't exist.",
+ 'create-base-path' => {
+ description => "Create the base directory, if it doesn't exist.",
type => 'boolean',
default => 'yes',
},
The s/create-path/create-base-path/ is obviously not finished, just to get the
idea
over, and the should_create_path helper should be renamed too, as currently
it's named
to generic and one would even expect it to take a path argument, as otherwise
one asks
"create _what_ path?" should_create_base_path or directly doing a
config_aware_base_mkdir
that also hosts the path creation directly (no hard feelings on that though)
Plus some adapting to newer style guideline stuff, e.g.:
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 634f289..1674e36 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1365,8 +1365,7 @@ sub activate_storage {
warn "${storeid}: 'mkdir' option is deprecated. Use 'create-path' or
'create-sub-dirs' instead.\n"
if defined($scfg->{mkdir});
- return if defined($scfg->{'create-sub-dirs'})
- && !$scfg->{'create-sub-dirs'};
+ return if defined($scfg->{'create-sub-dirs'}) &&
!$scfg->{'create-sub-dirs'};
# FIXME The mkdir option is deprecated and create-sub-dirs should be used
return if defined($scfg->{mkdir}) && !$scfg->{mkdir};
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
