[pve-devel] applied: [PATCH common v3 1/1] SectionConfig: add helper to delete keys from a section config entry

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

Am 08/03/2023 um 07:53 schrieb Thomas Lamprecht:
> Am 17/01/2023 um 12:46 schrieb Dominik Csapak:
>> this is a pattern we have very often, so we can implement that here and
>> reuse it
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>  src/PVE/SectionConfig.pm | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
>> index 0a9f1cd..e354655 100644
>> --- a/src/PVE/SectionConfig.pm
>> +++ b/src/PVE/SectionConfig.pm
>> @@ -543,4 +543,19 @@ sub assert_if_modified {
>>      PVE::Tools::assert_if_modified($cfg->{digest}, $digest);
>>  }
>>  
>> +sub delete_from_config {
>> +    my ($config, $option_schema, $new_options, $to_delete) = @_;
>> +
>> +    for my $k ($to_delete->@*) {
>> +	my $d = $option_schema->{$k} || die "no such option '$k'\n";
>> +	die "unable to delete required option '$k'\n" if !$d->{optional};
> 
> Should we use the (here already imported but not used) raise_param_exc ?

Well, without any reply here I'll still apply it for now, as said signature
change should not be required anyway.

> As that would then also make the magic "mark-field-as-invalid" work if
> used in API calls?
> 
> FWIW, we could use a cumulative approach, e.g. something like like:
> 
> my $errors = {};
> 
> for ... {
>    eval {
>        die "unable to delete fixed option\n" if $d->{fixed};
>        ...
>    };
>    if (my $err = $@) {
>        $errors->{$k} = $err;
>    } else {
>        delete ...
>    }
> }
> 
> raise_param_exc($errors) if scalar($errors->%*);

I actually thought shortly that if, this would need to be:

raise_param_exc({ delete => $joined_error_string });

As deletes normally come in via the 'delete' key, but from top of my head I don't
know if we ever use our field invalidation magic for deletes..