-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storaged: btrfs: show btrfs filesystem "root" subvolume as "top-level" #21127
Conversation
If we want this italic, it might not be too easy and we have to special case it for btrfs. |
Two different naive approaches, without much code modification:
This is assuming that "name" is not translated here, however, and that subvolumes are not able to be called "top-level" manually, and that someone has done so. Is this the best way to do it? I'm sure it's not. It's a way without modifying anything. It might spark some other idea too (perhaps a better way to determine this, as you know the code better), so I'm sharing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Also, I'm fine if we merge this as-is and treat styling as a follow-up. This is already an improvement.
However: One quick question: Is top-level ever supposed to be translated? It's the btrfs internal name for it, right? Do btrfs docs translate it? Do the command line tools translate it?
It's still in English as "top-level", at least in https://wiki.debianforum.de/Btrfs : |
d5ff3ae
to
18b510e
Compare
18b510e
to
ac37d11
Compare
Cockpit renamed the btrfs "root subvolume" to "top-level" to avoid confusion with "/". cockpit-project/cockpit#21127
And anaconda PR rhinstaller/anaconda-webui#492 |
ac37d11
to
a52afcd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Styling "top-level" differently than other names might need some work in pages.jsx
. I think we assume that page.name
is a string, and can't be a React component. We might need to allow that, or invent page.name_className
to put some custom classes into the DOM.
a52afcd
to
1ed3f41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Showing this as "/" confuses users as they might think it is the root partition and official btrfs terminology names it "top-level". https://btrfs.readthedocs.io/en/latest/Subvolumes.html Relates: cockpit-project#19920
When re-trying non-destructive tests the testFstabOptions test fails as /etc/fstab is not restored to its pristine state. CoreOS does not run storage tests so should not restore fstab/crypttab and the ws-container scenario should not stop cockpit as it isn't installed on the host itself.
1ed3f41
to
083a065
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! :-)
@@ -452,7 +462,7 @@ const BtrfsSubvolumeCard = ({ card, volume, subvol, snapshot_origin, mismount_wa | |||
backing_block={block} content_block={block} subvol={subvol} />}> | |||
<CardBody> | |||
<DescriptionList className="pf-m-horizontal-on-sm"> | |||
<StorageDescription title={_("Name")} value={subvol.pathname} /> | |||
<StorageDescription title={_("Name")} value={subvol.id === 5 ? "top-level" : subvol.pathname} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test.
Showing this as "/" confuses users as they might think it is the root partition and official btrfs terminology names it "top-level".
https://btrfs.readthedocs.io/en/latest/Subvolumes.html
Relates: #19920