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

pyanaconda: payload: split rsync command for /boot/efi to handle FAT filesystem limitations #6030

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

KKoukiou
Copy link
Contributor

The previous rsync command attempted to preserve attributes (permissions, ownership, symlinks) that are not supported by FAT. This commit splits the rsync process for /boot/efi to avoid these incompatible options and ensure proper handling of FAT-specific filesystems.

Resolves: rhbz#2329379

@pep8speaks
Copy link

pep8speaks commented Nov 29, 2024

Hello @KKoukiou! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-11-29 15:49:40 UTC

@github-actions github-actions bot added the f42 Fedora 42 label Nov 29, 2024
Copy link
Contributor

@M4rtinK M4rtinK 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, thanks! :)

@KKoukiou
Copy link
Contributor Author

/kickstart-tests --testtype smoke

@KKoukiou
Copy link
Contributor Author

Verified it works.

…filesystem limitations

The previous rsync command attempted to preserve attributes (permissions, ownership, symlinks)
that are not supported by FAT. This commit splits the rsync process for `/boot/efi` to avoid
these incompatible options and ensure proper handling of FAT-specific filesystems.

Resolves: rhbz#2329379
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Aside of the note below it looks good to me. Even thought I would rather like to see this abstracted in a method with proper doc-string explaining why.

But I don't want to block our problematic situation on this.

Comment on lines +457 to +458
try:
execWithRedirect(cmd, args)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add

self.report_progress(_("Installing the /boot/efi content..."))

so this is not completely silent operation for users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this interactively, and it tasks <1second to rsync this, it would be worse I believe to get the flashing string change.

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case you can ignore this I guess.

@KKoukiou
Copy link
Contributor Author

/kickstart-tests --testtype smoke

@KKoukiou KKoukiou merged commit b4786a7 into rhinstaller:master Nov 29, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f42 Fedora 42
Development

Successfully merging this pull request may close these issues.

4 participants