Add representations of various system entities #6
Conversation
|
Changes made / pushed |
| FileUtils.mkdir @mountpoint unless File.directory? @mountpoint | ||
|
|
||
| run "/usr/bin/mount #{self.path} #{@mountpoint}" | ||
| end |
There was a problem hiding this comment.
It is not clear what the difference is between the path parameter and the path method. Should they be renamed for clarity?
|
@movitto The travis build failed, not sure why. @brandondunne @Fryguy @chessbyte Does it make sense to extract the paths to various binaries into another location, such as a module constants like Maybe it makes sense to do this after we have substantial use of more than 3 binaries and have a better sense of how to separate this. |
|
@movitto I have some style comments, and I'll put those inline, but I had a few general questions and comments first...
Although I have a lot of comments, I just wanted to say I think this is great work, and is a great start to adding a lot of functionality to LinuxAdmin! |
|
|
||
| def self.local | ||
| (Dir.glob('/dev/hd[a-z]') + | ||
| Dir.glob('/dev/sd[a-z]')).collect { |d| |
There was a problem hiding this comment.
@movitto style: multiline blocks should use do/end. @Fryguy is planning on forking one of the ruby style guides and working on what our group's style should be. We should always prefer readability over the style guide though, so keep in mind it's fine (or even great) to have an opinion that readability dictates the code should look different than the style guide.
There was a problem hiding this comment.
This can be a single Dir.glob call with Dir.glob('/dev/[hs]d[a-z]')
|
@movitto Oh, one more thing is that I noticed most of the commands prefix with /usr/bin. Is there a reason for this? In the other modules we just left the command bare, especially since we couldn't be sure where the command may be installed. |
| @mountpoint = path | ||
| @mountpoint = | ||
| "/mnt/#{disk.path.split(File::SEPARATOR).last}#{id}" if path.nil? | ||
| FileUtils.mkdir @mountpoint unless File.directory? @mountpoint |
There was a problem hiding this comment.
Wrap the arguments in parens () especially when you have an "unless" condition in the middle.
|
@movitto Looks like a great start. Other than our style nitpicks, the only major things are the shelling out in the specs and that the specs appear much smaller than the code, so I'm not sure we have all the code covered by specs. |
|
@movitto I don't understand the purpose of the .local methods. The purpose of LinuxAdmin is for interacting with a local system, so they seem kind of unnecessary. I would expect most, if not all, of that logic could go into the .initialize methods directly or delay-called in a method after initialization. |
|
Updated:
Other thoughts? |
|
@movitto It's looking good. A few thoughts on your comments. mocking/stubbing output: I would say it would make sense to mock/stub shelling out whenever possible and if it becomes too brittle, unreadable, etc., we find another solution... such as more abstraction to hide the nastiness or just shell out. Let's let the code/test tell us we're doing it right/wrong. If the full paths are encapsulated into modules as constants which you +1, it doesn't matter if they're fully qualified as we can change them in one place if FQ paths are not what we want later on. I'm fine with dealing with shell commands exiting with 0 as a true value in ruby. My concern is if we check the rc for 0 all over the place, it might make sense to have helper method that makes the code look more rubyish. Again, maybe we wait until it's needed instead of me assuming it would be a problem ;-) I don't like/dislike the local method. Did we want clases with only a singleton object? If so, singleton might be what we want and use the instance class method to get at the one and only one instance. https://practicingruby.com/articles/shared/jleygxejeopq |
| attr_accessor :path | ||
|
|
||
| def self.local | ||
| Dir.glob('/dev/[hs]d[a-z]').collect do |d| |
There was a problem hiding this comment.
Let's add "v" to [hsv]d[a-z] because RHEV VirtIO disks show up as vda, vdb, ...
|
With regard to mocking/stubbing, I firmly believe the code should not shell out in specs to allow for portability and for allowing the specs to run in different locations whether or not the underlying libraries and such are installed. For example, on TravisCI, there may not be rhn or subscription_manager, if it even runs on a RedHat system at all. For specs that just need to verify a particular command is executed we can use the .should_receive expectations. For specs that need to return data, the commands/files could be run on a real system with the output captured into files which are mocked in the test. This is not unlike VCR for http mocking, which has proven very useful. If you go through the existing specs we've used this technique in a few places. The constants question kind of falls out of this testing approach. At one point in the code, we had constants, and then found that for files, a method that returns a value allowed us to easily stub that method with a mocked file. For example, you can see how we stubbed this method with different values for different scenarios by providing different file paths in the spec here. For executable paths, a constant is probably good enough. Didn't know that about the full paths, and that link is really interesting! We should probably update the rest of the classes with respect to full paths as well. I think that question mark methods should do the The reason I raised the question about the .local methods is I was thinking about the Singleton pattern (as @jrafanie mentioned although that link seems to make it more confusing/scary than it needs to be). In particular the Singleton instance would be around a method like .local. In fact, the reason we initially had class methods for the other modules is because later we were thinking of Singleton-patterning them. |
|
|
||
| def unmount | ||
| run("/usr/bin/umount", | ||
| :params => { nil => [@mount_point] }) |
There was a problem hiding this comment.
I think these read better as a one-liner.
|
Updated:
Hope everything else looks good. |
|
OK, flushed out the specs / incorporated the remaining feedback. Opened an issue to move the existing modules over to the ood approach. Should be good to go |
|
See failing tests on https://travis-ci.org/ManageIQ/linux_admin/builds/8516079 |
| p = d.partitions | ||
| p[0].id.should == 1 | ||
| p[0].disk.should == d | ||
| p[0].size.should == '80.5GB' |
There was a problem hiding this comment.
I didn't notice this before when there weren't specs, but the callers shouldn't have to deal with a number in this format. We should parse this and return a number, preferably in bytes. Since active_support is already available to this gem, you could probably just get it to call 80.5.gigabytes to get the expected number.
|
The size parsing issue is resolved. The travis failures for the most were due to a difference in environments. I believe I have the fix but can't be certain until travis picks it up and tests this out (is there a more efficient way of doing this?) |
|
Still failing: https://travis-ci.org/ManageIQ/linux_admin/builds/8543448 |
|
@movitto Looks good although this pull request is from your master branch. It's not a huge deal but in the future, it would be helpful to use a somewhat relevant branch name as it becomes part of our history when we merge. Is this pull longer than the initial pull request? It seems larger than when I originally reviewed it? Were there classes added? |
|
K, made the changes: Noted about the branch. Will change it w/ new pull requests going forward. There have been a few revisions, but the architecture largely remains the same. Besides trivial organizational changes, the major differences between the latest revision and the original one are:
|
- add new modules to main linux_admin module - add method to lookup local command from dictionary
Add representations of various system entities
No description provided.