Skip to content
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

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

jelly
Copy link
Member

@jelly jelly commented Oct 16, 2024

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


image

@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 16, 2024
@jelly jelly requested a review from garrett October 16, 2024 15:02
@jelly
Copy link
Member Author

jelly commented Oct 16, 2024

If we want this italic, it might not be too easy and we have to special case it for btrfs.

@garrett
Copy link
Member

garrett commented Oct 17, 2024

Two different naive approaches, without much code modification:

  • One passes all the names to a data attribute and CSS specifically acts upon the attribute (with a simple class selector child below it, so it won't try to match anything but a parent of the class, instead of everything): b33a512

  • The other explicitly only handles "top-level" and sets a class: 3a660b1

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.

garrett
garrett previously approved these changes Oct 17, 2024
Copy link
Member

@garrett garrett left a 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?

@garrett
Copy link
Member

garrett commented Oct 17, 2024

It's still in English as "top-level", at least in https://wiki.debianforum.de/Btrfs :

image

@jelly jelly force-pushed the top-level-as-root branch from d5ff3ae to 18b510e Compare October 17, 2024 17:53
@jelly jelly force-pushed the top-level-as-root branch from 18b510e to ac37d11 Compare October 29, 2024 12:56
jelly added a commit to jelly/anaconda-webui that referenced this pull request Oct 29, 2024
Cockpit renamed the btrfs "root subvolume" to "top-level" to avoid
confusion with "/".

cockpit-project/cockpit#21127
@jelly
Copy link
Member Author

jelly commented Oct 29, 2024

And anaconda PR rhinstaller/anaconda-webui#492

@jelly jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 29, 2024
@jelly jelly requested a review from mvollmer October 29, 2024 15:11
@jelly jelly marked this pull request as ready for review October 29, 2024 15:11
@jelly jelly force-pushed the top-level-as-root branch from ac37d11 to a52afcd Compare October 29, 2024 15:11
mvollmer
mvollmer previously approved these changes Oct 30, 2024
Copy link
Member

@mvollmer mvollmer left a 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.

mvollmer
mvollmer previously approved these changes Oct 31, 2024
Copy link
Member

@mvollmer mvollmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

jelly added 2 commits October 31, 2024 10:35
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.
Copy link
Member

@mvollmer mvollmer left a 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} />
Copy link
Contributor

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.

@jelly jelly merged commit b66d6c8 into cockpit-project:main Oct 31, 2024
86 of 87 checks passed
@jelly jelly deleted the top-level-as-root branch October 31, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants