Skip to content

Conversation

movitto
Copy link
Contributor

@movitto movitto commented Jul 17, 2013

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling bb9473d on movitto:fstab_write into 42d0486 on ManageIQ:master.

Copy link
Member

Choose a reason for hiding this comment

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

Suggest using block format of File.open. However, I also noticed there's a write method in LinuxAdmin::Common. Should we be using that one? Does the possible method name conflict cause a problem? FWIW, I'm actually not a fan of that method in Common.

Copy link
Member

Choose a reason for hiding this comment

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

@movitto I wonder if we need to check if /etc/fstab is writable.

Copy link
Member

Choose a reason for hiding this comment

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

@movitto Maybe we should make a big fat TODO and create an issue here that we may need to edit/append to the existing file in the future. I can imagine someone having customizations in fstab that may get lost depending on what write does.

@movitto
Copy link
Contributor Author

movitto commented Jul 17, 2013

I wonder if we need to check if /etc/fstab is writable.

I'd assume that only users w/ superuser privs would be able to call fstab.write (as well as alot of other linux_admin functionality). We can make this explicit at some point, but affects more than this method / should be handled separately.

Maybe we should make a big fat TODO and create an issue here that we may need to edit/append to the existing file in the future.

FStab is a singleton class and in the constructor the entries are read from the filesystem. From that point, any calls to 'write' will include those entries.

@jrafanie
Copy link
Member

My bad, I guess the idea is we'll add items to @entries and they'll automatically be written via the write! method.

@jrafanie
Copy link
Member

👍

jrafanie added a commit that referenced this pull request Jul 17, 2013
@jrafanie jrafanie merged commit 8efd5be into ManageIQ:master Jul 17, 2013
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