[pve-devel] [PATCH ceph 1/2] fix Ceph version string handling

[Thomas Lamprecht]

On 1/7/20 5:12 PM, Martin Verges wrote:
> Signed-off-by: Martin Verges <martin.verges at croit.io>
> ---
>  PVE/Storage/RBDPlugin.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 0a33ec0..b0420fe 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -126,7 +126,7 @@ my $krbd_feature_update = sub {
>  my $ceph_version_parser = sub {
>      my $ceph_version = shift;
>      # FIXME this is the same as pve-manager PVE::Ceph::Tools get_local_version
> -    if ($ceph_version =~ /^ceph.*\s(\d+(?:\.\d+)+(?:-pve\d+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
> +    if ($ceph_version =~ /^ceph.*\s(\d+(?:\.\d+)+(?:-[^\s]+)?)\s+(?:\(([a-zA-Z0-9]+)\))?/) {
>  	my ($version, $buildcommit) = ($1, $2);
>  	my $subversions = [ split(/\.|-/, $version) ];
>  
> 

Generally it's nice if the repository this patch is for is included in the
PATCH tag, e.g.: "[PATCH storage 1/2]" for this one, and "[PATCH manager 2/2]"
for the other one.

You could set that in the local git config, so you do not need to edit it manually
when sending a patch, e.g:
# cd /path/to/pve-storage
# git config --local format.subjectprefix 'PATCH storage'

I mean, I can figure out 99% of the time for what repo a patch is intended for, it's
still nice to have, though.

But, the thing I really would like to have is a rationale for the patch, i.e., why
is it required, what situation IN PVE does it fix. Doesn't need to be long, short
sentence in the commit message's body can be enough.
We always version with pve\d+, and that part is even optional in the regex, so why
do you need this? I.e., why is this a "fix", as you name it, and not a
> ceph version parser: accept non-pve Ceph builds extra version part
>
> we have packages which sets this to "foo" because reason "bar", etc...

commit message?
But, as it's optional, and thus does not fails version parsing for tests or similar
builds, I'm not sure this is required at all..

cheers,
Thomas