tui: show the disk on which an existing installation or backup is found #20
Conversation
| @@ -218,9 +218,11 @@ def get_admin_interface_configuration(answers): | |||
| def get_installation_type(answers): | |||
| entries = [] | |||
| for x in answers['upgradeable-products']: | |||
| entries.append(("Upgrade %s" % str(x), (x, x.settingsAvailable()))) | |||
There was a problem hiding this comment.
I think the design goal was so that you don't need to see internal device names when installing upgrading/restoring. That way it looks cleaner and doesn't show (unstable) Linux device names. You can press F5 to see the full details of the existing installation, including the disk it is installed on. Does that help?
There was a problem hiding this comment.
Right, it does give the information. However, the goal of this patch is to help a user avoid making wrong decisions without realizing it, and I'm not sure that relying on an optional display would help much.
An example use-case where it would help, very much tied to patch series we did not submit yet, is when a system has been installed on a software RAID device (we have in XCPng a patch allowing the user to build a RAID1 for use as primary disk). I'm working on a patch-series that makes RAID assembly "opt-in, off by default" (see #16), and after this behavior change such a RAID install will not be listed at first; then if any install is found on another disk, when the disk is not mentioned in the entry itself, the user might think the install listed is the one it expects to find, and I think we don't want that to happen.
So maybe I should just move this commit back into my RAID-improvements PR-to-be (which incidentally already exists as a PR against XCPng, and on which you're welcome to comment already), with better context.
There was a problem hiding this comment.
OK, I see you have a reason for doing this. In that case I think it is generally fine to show some extra information there.
I'd like to ensure that it shows some kind of stable identifier rather than something like /dev/sda and /dev/sdb which might switch around on successive boots. Does it do this?
It might also be useful to include the disk size since that is often an easy way of disambiguating the disks in a system.
There was a problem hiding this comment.
I suspect we should in fact use the same label as is already used in the primary-disk selection menu, and it already includes disk size. It does not use anything more stable than /dev/sda though, but with size as disambiguation it should be fine, and if disks are always presented by the same code it will be easy to improve it when deemed necessary.
There was a problem hiding this comment.
I'm not really happy with the results on a small 80x25 textmode:
With a longer description (faked here for testing), the screen overflow looks really bad:
Unfortunately, that old newt library does not seem to support multiline list entries. Looks like for we'll have to be satisfied with just adding the size :/.
ydirson
force-pushed
the
for-xs/show-product-disk
branch
2 times, most recently
from
f4d464b
to
cd9fef7
Compare
…for user Signed-off-by: Yann Dirson <[email protected]>
Adding disk vendor and model would be useful, but can take too much horizontal space, especially for good old 80x25 resolution, so we use a shorter string with just the disk size for disambiguation. Signed-off-by: Yann Dirson <[email protected]>
ydirson
force-pushed
the
for-xs/show-product-disk
branch
from
cd9fef7
to
97052fc
Compare
|
@GeraldEV any reason to prefer rebase over merge? Merge strategy would link the merge commit back to the PR, and also keeps the code in the exact same state that was tested by submitter (eg. making |
No description provided.