[pve-devel] [PATCH v3 guest-common 04/19] helpers: add method to represent config as a table

[Thomas Lamprecht]

On 10/18/19 7:27 AM, Fabian Grünbichler wrote:
> On October 17, 2019 7:48 pm, Thomas Lamprecht wrote:
>> On 10/14/19 10:28 AM, Oguz Bektas wrote:
>>> in vm_pending API, this method is used to represent the configuration as
>>> a table with current, pending and delete columns.
>>>
>>> by adding it as a guesthelper, we can also use it for container pending
>>> changes.
>>>
>>> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
>>> ---
>>>  PVE/GuestHelpers.pm | 35 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 35 insertions(+)
>>>
>>> diff --git a/PVE/GuestHelpers.pm b/PVE/GuestHelpers.pm
>>> index a36b9bc..a1ec76f 100644
>>> --- a/PVE/GuestHelpers.pm
>>> +++ b/PVE/GuestHelpers.pm
>>> @@ -142,4 +142,39 @@ sub format_pending {
>>>      }
>>>  }
>>>  
>>> +sub conf_table_with_pending {
>>
>> nothing to do with table itself, it just returns a array of
>> hashes with current and pending value..
>>
>> I'd rather like to not bind method names to something their caller
>> may do with them, but to something they actually do their self.
>>
>> Albeit a good name is not to easy here... Maybe:
>> get_current_config_with_pending
>>
> 
> it does not really 'get' the config either though ;)

it sure does, gets config with pending from config and pending info
as source ;)

> 
> I don't really know how to call
> hash1 hash2 => list of items derived from hash1 and hash2
> 
> except that it's some kind of 'transformation'. it's not a mapping 
> operation (it could be seen as sequence of three mappings), it's not a 
> flattening, it's not folding.
> 
> there is in fact no information added to the input, it's just presented 
> in a different structure.
> 
> 'transform_to_list_with_pending' was about as far as we got off-list, 
> and Oguz didn't like that at all :-P

neither me ^^

I say my get is good above, nothing wrong with a get, has not to be
GET over HTTP to make it true ;)

Else, "assemble_config_with_pending" or "config_with_pending_list",
are options, but I don't want to start a full bike-shed, so chose the
least unfavorable one and let's continue, it's not like our existing
method name game is AAA, it just should be callee related not caller
.. ;)

> 
>>> +    my ($conf, $pending_delete_hash) = @_;
>>> +
>>> +    my $res = [];
>>> +
>>> +    foreach my $opt (keys %$conf) {
>>> +	next if ref($conf->{$opt});
>>> +	my $item = { key => $opt };
>>> +	$item->{value} = $conf->{$opt} if defined($conf->{$opt});
>>> +	$item->{pending} = $conf->{pending}->{$opt} if defined($conf->{pending}->{$opt});
>>> +	$item->{delete} = ($pending_delete_hash->{$opt}->{force} ? 2 : 1) if exists $pending_delete_hash->{$opt};
>>> +
>>> +	push @$res, $item;
>>> +    }
>>> +
>>> +    foreach my $opt (keys %{$conf->{pending}}) {
>>> +	next if $opt eq 'delete';
>>> +	next if ref($conf->{pending}->{$opt}); # just to be sure
>>> +	next if defined($conf->{$opt});
>>> +	my $item = { key => $opt };
>>> +	$item->{pending} = $conf->{pending}->{$opt};
>>> +
>>> +	push @$res, $item;
>>> +    }
>>> +
>>> +    while (my ($opt, $force) = each %$pending_delete_hash) {
>>> +	next if $conf->{pending}->{$opt}; # just to be sure
>>> +	next if $conf->{$opt};
>>> +	my $item = { key => $opt, delete => ($force ? 2 : 1)};
>>> +	push @$res, $item;
>>> +    }
>>> +
>>> +    return $res;
>>> +}
>>> +
>>>  1;