disk module's functions now try to use non-existent args #1730

Closed
mchesnut opened this Issue Jul 30, 2012 · 6 comments

Projects

None yet

3 participants

@mchesnut

This appears to have been introduced in 0.10.2. From the commit log it looks like pull request #1687 was the culprit.

The only diff I see is that the function prototypes were changed to mention args and now the cmd string is built by erroneously assuming that args is always defined (although it has no default).

Now when I attempt to call disk.usage (or disk.inodeusage) using salt-call, I get an error:

"Error running 'disk.usage': cannot concatenate 'str' and 'NoneType' objects"

(This is because cmd and ' -' are strings, but args is NoneType)

When calling remotely (salt disk.usage) I just get a python traceback. This is on CentOS 6.2 although I suspect any Linux host (and possibly others) would have the same issue since the module just uses df under the hood.

The addition of this (now required) additional parameter is not reflected in the docs for the disk module. I had to poke around the code to figure out how it's expected to work.

So the issue is that either the change in functionality should be reverted, or the change should be implemented more robustly (i.e., work whether an argument is provided or not; include documentation for how it should work). Hoping for @thatch45 to provide guidance; I wouldn't mind doing the legwork either way once I know which direction we want to go.

@thatch45
Salt Stack member

@techhat can you please fix this bug? I am all for having the extra capabilities, but it should work without arguments and not throw a traceback

@techhat
Salt Stack member

Agreed. I will get this corrected.

@mchesnut

Thanks for the quick response guys.

I think the best way to address the desired functionality here would be to add a parameter called, e.g., "format" and just have it default to "k". Would that make sense?

I could also see adding other parameters down the line, e.g., "disk=/dev/sda1" to only return data on that device, so I'm trying to think of a convention that'd be general enough to cover everything. (And the option=value syntax seems to be pretty standardized in salt, but please correct me if I'm wrong!)

@techhat
Salt Stack member

I like the idea of being able to pass in a specific device, and will probably work on that. But in my eyes, the more important thing to do is fix the regression. I just submitted a pull request that will accept the args, but only try to apply them if they are actually passed in.

@thatch45 thatch45 closed this Jul 31, 2012
@mchesnut

Awesome, this looks to be working great for me. Thanks @techhat!

@thatch45
Salt Stack member

Thanks for the verification @mchesnut

@herlo herlo added a commit to herlo/salt that referenced this issue Aug 2, 2012
@herlo herlo repackage with patch to fix #1730 2b010ad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment