Skip to content

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Sep 4, 2013

No description provided.

@jrafanie
Copy link
Member Author

jrafanie commented Sep 4, 2013

@brandondunne @movitto Please review... this is the bug I ran into.

In my testing, the input fstab would produce the output below:

input:


#
# /etc/fstab
# Created by anaconda on Wed Mar 13 13:26:01 2013
#
# Accessible filesystems, by reference, are maintained under '/dev/disk'
# See man pages fstab(5), findfs(8), mount(8) and/or blkid(8) for more info
#
/dev/mapper/vg_jrrhel64-lv_root /                       ext4    defaults        1 1
UUID=1d5c65f1-b3be-43a5-b113-ebc26e0227e1 /boot                   ext4    defaults        1 2
/dev/mapper/vg_jrrhel64-lv_swap swap                    swap    defaults        0 0
tmpfs                   /dev/shm                tmpfs   defaults        0 0
devpts                  /dev/pts                devpts  gid=5,mode=620  0 0
sysfs                   /sys                    sysfs   defaults        0 0
proc                    /proc                   proc    defaults        0 0

output:

    0 0
/dev/mapper/vg_jrrhel64-lv_root / ext4 defaults 1 1
UUID=1d5c65f1-b3be-43a5-b113-ebc26e0227e1 /boot ext4 defaults 1 2
/dev/mapper/vg_jrrhel64-lv_swap swap swap defaults 0 0
tmpfs /dev/shm tmpfs defaults 0 0
devpts /dev/pts devpts gid=5,mode=620 0 0
sysfs /sys sysfs defaults 0 0
proc /proc proc defaults 0 0

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 26d1697 on jrafanie:fix_empty_fstab_lines into 07a33b8 on ManageIQ:master.

@Fryguy
Copy link
Member

Fryguy commented Sep 4, 2013

Looks good 👍

Copy link
Member

Choose a reason for hiding this comment

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

@jrafanie I think the reject is harder to follow than a collect. Also, since we have active support, shoun't we use .starts_with? and .blank??

Just a thought...
contents.lines.collect {|line| line.strip!; line unless line.blank? || line.starts_with?("#")}.compact

Copy link
Member Author

Choose a reason for hiding this comment

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

@brandondunne The only benefit to using blank? in your example is if we're keeping whitespace.
line.strip! effectively makes the line.blank? into a .nil? call since we've removed the possibility for empty strings.

If we were going to use blank?, I would think something like this:

contents.lines.to_a.collect {|line| line unless line.blank? || line.starts_with?("#")}.compact

This retains any leading or trailing whitespace in the line though.

Either way, I think the real solution is to teach the parsing and writing code to only make changes where you make changes in the FSTabEntry objects.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 4cef28b on jrafanie:fix_empty_fstab_lines into 07a33b8 on ManageIQ:master.

bdunne added a commit that referenced this pull request Sep 5, 2013
Don't create FSTabEntry objects for lines with only whitespace.
@bdunne bdunne merged commit dfe82fa into ManageIQ:master Sep 5, 2013
@jrafanie jrafanie deleted the fix_empty_fstab_lines branch March 22, 2017 14:14
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