Re: [pve-devel] [PATCH v3 manager 2/4] api: vzdump: add call to get currently configured vzdump defaults

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 02.04.21 14:27, Fabian Ebner wrote:
> Am 01.04.21 um 15:40 schrieb Thomas Lamprecht:
>> On 11.03.21 10:22, Fabian Ebner wrote:
>>> +    user => 'all',
>>> +    },
>>> +    proxyto => 'node',
>>> +    parameters => {
>>> +    additionalProperties => 0,
>>> +    properties => {
>>> +        node => get_standard_option('pve-node'),
>>> +        storage => get_standard_option('pve-storage-id', { optional => 1 }),
>>> +    },
>>> +    },
>>> +    returns => {
>>> +    type => 'object',
>>> +    additionalProperties => 0,
>>> +    properties => PVE::VZDump::Common::json_config_properties(),
>>
>> above may suggest that all those properties are returned, but we delete some
>> out flat, so even if one would access this as root@pam they won't get all of
>> those and may get confused due to API schema/return value mismatch.
>>
>> Maybe we could derive a hash from above list at module scope, and delete those
>> keys that never will be returned. That list could also be reused below 
for
>> filtering out those unwanted keys too then.
>>
> 
> The only key that is always dropped is the deprecated 'size' key. For privileged users, all others are returned. And the schema says that all properties are optional. We could create a new hash and drop the 'optional' 
for those with a default value that are never dropped, but not sure if that's worth it...

hmm, yeah in that case it's probably not worth it...

> 
>>> +    },
>>> +    code => sub {
>>> +    my ($param) = @_;
>>> +
>>> +    my $node = extract_param($param, 'node');
>>> +    my $storage = extract_param($param, 'storage');
>>> +
>>> +    my $rpcenv = PVE::RPCEnvironment::get();
>>> +    my $authuser = $rpcenv->get_user();
>>> +
>>> +    my $res = PVE::VZDump::read_vzdump_defaults();
>>> +
>>> +    $res->{storage} = $storage if defined($storage);
>>> +
>>> +    if (!defined($res->{dumpdir}) && !defined($res->{storage})) {
>>> +        $res->{storage} = 'local';
>>> +    }
>>> +
>>> +    if (defined($res->{storage})) {
>>> +        $rpcenv->check_any(
>>> +        $authuser,
>>> +        "/storage/$res->{storage}",
>>> +        ['Datastore.Audit', 'Datastore.AllocateSpace'],
>>> +        );
>>> +
>>> +        my $info = PVE::VZDump::storage_info($res->{storage});
>>> +        foreach my $key (qw(dumpdir prune-backups)) {
>>
>> style nit, for new code use `for`, `foreach` has no additional value/functionality
>> over `for` since a long time (if ever, actually not too sure from top of my head).
>>
> 
> Will do, but might be worth updating the style guide:
> "use either of foreach or for when looping over a list of elements."
> 
> https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices

good catch, done!