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

[Martin Verges]

>
> 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'
>

Thanks, I added that for the future. I'm more used to the much more user
friendly github / gitlab.


> 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..
>

A lot of people use proxmox and Ceph and your versions are ok for most of
them. However we provide our own croit Ceph debian repository that a lot of
people use to get newer, better Ceph versions into your software.
One of your proxmox users came to us, complaining that our package named
"ceph version 14.2.5-1-g23e76c7aa6
(23e76c7aa6e15817ffb6741aafbc95ca99f24cbb) nautilus (stable)" breaks the
proxmox software. I investigated and found out that the git commit in our
version string is the issue. Therefore the patch fixes the issue with
different named Ceph releases but not a fix to proxmox itself.

I hope you accept the patch in favor to the users using proxmox with our
Ceph builds.

--
Martin Verges
Managing director

Mobile: +49 174 9335695
E-Mail: martin.verges at croit.io
Chat: https://t.me/MartinVerges

croit GmbH, Freseniusstr. 31h, 81247 Munich
CEO: Martin Verges - VAT-ID: DE310638492
Com. register: Amtsgericht Munich HRB 231263

Web: https://croit.io
YouTube: https://goo.gl/PGE1Bx


Am Mi., 8. Jan. 2020 um 11:39 Uhr schrieb Thomas Lamprecht <
t.lamprecht at proxmox.com>:

> 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
>
>