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

Update 200_check_usb_layout.sh #3030

Merged
merged 1 commit into from Jul 27, 2023
Merged

Conversation

casantos
Copy link
Contributor

Issue an error message and exit if USB_DEVICE_FILESYSTEM is invalid, instead of setting it to "ext3". It's safer to fail due to configuation errors than to ignore/fix them silently.

Fixes: issue #3029

  • Type: Bug Fix

  • Impact: Low

  • Reference to related issue (URL): ReaR should issue an error message and exit if USB_DEVICE_FILESYSTEM is invalid #3029

  • How was this pull request tested?

    Ran rear mkbackup with a configuration containing

    BACKUP=NETFS
    BACKUP_PROG_COMPRESS_OPTIONS=( )
    BACKUP_PROG_COMPRESS_SUFFIX=
    BACKUP_URL=usb:///dev/disk/by-label/REAR-000
    MODULES=()
    OUTPUT=USB
    USB_BOOTLOADER=grub
    USB_DEVICE_FILESYSTEM=xfs
    USB_DEVICE_PARTED_LABEL=gpt
    USB_UEFI_PART_SIZE=2048
    SECURE_BOOT_BOOTLOADER=/boot/efi/EFI/redhat/shimx64.efi
    
  • Brief description of the changes in this pull request:

Issue an error message and exit if USB_DEVICE_FILESYSTEM is invalid.

Issue an error message and exit if USB_DEVICE_FILESYSTEM is invalid,
instead of setting it to "ext3". It's safer to fail due to configuation
errors than to ignore/fix them silently.

Fixes: issue rear#3029

Signed-off-by: Carlos Santos <casantos@redhat.com>
@jsmeix jsmeix added the enhancement Adaptions and new features label Jul 25, 2023
@jsmeix jsmeix added this to the ReaR v2.8 milestone Jul 25, 2023
@jsmeix
Copy link
Member

jsmeix commented Jul 25, 2023

@casantos
thank you for your contribution
to make ReaR behave more in compliance with
https://github.com/rear/rear/wiki/Coding-Style

I think it is not a bug because conf/default.conf reads

# Only ext3 and ext4 are supported by the format workflow.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

From plain looking at the code changes it looks good to me

@jsmeix jsmeix requested a review from a team July 25, 2023 09:40
@jsmeix jsmeix self-assigned this Jul 25, 2023
@jsmeix
Copy link
Member

jsmeix commented Jul 25, 2023

@rear/contributors
please have a look here (as time permits),
perhaps I may have overlooked something.

I would like to merge it on Thursday afternoon
unless there are objections.

Copy link
Member

@pcahyna pcahyna left a comment

Choose a reason for hiding this comment

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

thank you for the change, I think that silently changing user's settings to something else because we don't like them is a bad idea (except for special defaults like the empty string).

@jsmeix
Copy link
Member

jsmeix commented Jul 25, 2023

Yes, I fully agree with you,
cf. your
#3025 (comment)
and my
#3025 (comment)
that reads (excerpt)

What is still questionable is if ReaR should
try to silently "correct" a user specified value
like "only blank characters" == "no characters",
...
or
if ReaR should not try to "correct" user specified values?

The more I am thinking about it the more I think that
ReaR should not try to "correct" user specified values
but error out when a user specified value is wrong.

Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

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

Good catch, thank you!

@jsmeix jsmeix merged commit f042182 into rear:master Jul 27, 2023
20 checks passed
@casantos casantos deleted the casantos-fix-3029 branch July 28, 2023 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants