search

xcp-ng / host-installer_old

XCP-ng installer
16 stars 15 forks source link

fix installer on intel motherboard xeon server raid #9

Open nagaozen opened 5 years ago

nagaozen commented 5 years ago 
name = re.match("([^_]*)(_\d+)?$", name).group(1)

Throws: AttributeError: 'NoneType' object has no attribute 'groups' because sometimes there are more than numbers after the '_'.

stormi commented 5 years ago

Thanks for this contribution. Can you give me examples of a name that matches correctly and the one you saw did not match and caused the bug?

stormi commented 5 years ago

Ping?

nagaozen commented 5 years ago

Sorry, I was doing this in the office, but had to go forward with another solution. I can't rollback to check the details. The installer is just broken the way it's now. I followed the code to find the bug and prevent future users to end up like me. It's a minor change and the bug is probably introduced by the line 409 for the reasons explained on lines 407 and 408. It's a greedy approach, so I just took a more conservative path, instead of admitting that "mdadm may append an _ followed by a number", I'm taking into account that it might be the case, and update the name if its the case, but keeping the original name otherwise. Here's the error:

WP_20190611_16_24_02_Pro

stormi commented 5 years ago

I trust your change, but I would like to be able to test the regexp myself at least once on the string that made the installer fail, so if you can provide it that would greatly help.

stormi commented 5 years ago

There's probably a way to extract that string from the host even if it's running another solution, since it's hardware-related.

nagaozen commented 5 years ago

You may want to try exempli gratia --name=company_namespace or --name=alpha_n0

stormi commented 5 years ago

I need to understand the context more to be able to try to contribute your patch upstream.

With your patch, you avoid the error, but you don't adapt the regexp to match those devices (which should probably be preferred). Why this choice?

I know this may look like a lot of discussion for a small patch, but I really need to understand the context and the outcome clearly before merging it, especially since I did not write the code in the first place.