[pve-devel] [PATCH container] Fix #1326: Multi destinations support.

[Pavel Andreev]

Hi Dominik,

please, also see inline.
>
>> +    return  $cmdname eq 'add' ? [] : [ PVE::Status::status_ids($cfg) ];
>> +}
>> +
>> +1;
>
> hmm, is there a specific reason you created this file?
> if it is just for the 'complete_status_server' sub,
> this could be defined in the plugin.pm i guess
>
> anyway, you did not include the file in the makefile, which would not
> throw any errors when building, but anyone with a status.cfg
> would run into perl errors
>

There is no specific reason but I followed Storage code you pointed to. 
In there these functions are in separate Storage.pm but indeed can be in 
Plugin.pm.
Makefile yes, missed.

>> -    type => {
>> -        description => "Plugin type.",
>> -        type => 'string', format => 'pve-configid',
>> -    },
>
> why did you remove the type format?
>
>> +        type => { description => "Plugin type." },
>> +             server => get_standard_option('pve-status-server-id',
>> +            { completion => \&PVE::Status::complete_status_server }),
>
> since there is no commandline tool for adding/editing/removing the 
> status servers, a bash-completion makes really no sense?
>

just followed Storage approach.

> also we already have a server property just below the disable property?
> so the name would have to be different like "name"

In fact it doesn't interfere but you're right like "server_name" would 
be better.

>>  sub parse_section_header {
>>      my ($class, $line) = @_;
>>
>> -    if ($line =~ m/^(\S+):\s*$/) {
>> -    my $type = lc($1);
>> +     if ($line =~ m/^(\S+):\s*(\S+)\s*$/) {
>> +    my ($type, $serverid) = (lc($1), $2);
>>      my $errmsg = undef; # set if you want to skip whole section
>> -    eval { PVE::JSONSchema::pve_verify_configid($type); };
>> +    eval { parse_status_server_id($serverid); };
>
> again you remove the check of the type, but this is important
>

also borrowed from corresponding Storage parse function or am I missed 
something?

>>      $errmsg = $@ if $@;
>>      my $config = {}; # to return additional attributes
>> -    return ($type, $type, $errmsg, $config);
>> +    return ($type, $serverid, $errmsg, $config);
>>      }
>>      return undef;
>>  }
>
> this change is sadly not backwards-compatible, because now you have to
> define a name, but i guess it should be optional, as to not break the 
> old configs (also i guess most people will only ever have 1 influxdb 
> or 1 graphite export) 

That's good point and one of the reasons I suggested secondary_server 
option instead. I'll check how this can be fixed.

Thank you,

Pavel