Skip to content

check functions for plugins (#1201933)#8

Merged
vpodzime merged 13 commits intostoraged-project:masterfrom
vpodzime:master-check_functions
Apr 14, 2015
Merged

check functions for plugins (#1201933)#8
vpodzime merged 13 commits intostoraged-project:masterfrom
vpodzime:master-check_functions

Conversation

@vpodzime
Copy link
Collaborator

@vpodzime vpodzime commented Apr 8, 2015

When loading plugins, we should check if the environment provides everything they need. This is the first step towards that goal.

This PR is expected to get more commits (with other plugins' check functions), but an early review of the basic design and implementation decisions would be nice.

@vpodzime
Copy link
Collaborator Author

vpodzime commented Apr 8, 2015

This is related to blivet's pull #60.

@mulkieran
Copy link
Contributor

I didn't delve into individual procedures, but overall it looks fine.

I think the name "check" should be made more specific...maybe check_availability or check_external or something.

And the error message in that method should probably be more specific to its function as well, something like
"Unable to verify availability of required external dependency 'lvm': %s". Someday it may seem like the correct idea to load the plugin even if the external dependency is not there at the time.

@vpodzime
Copy link
Collaborator Author

vpodzime commented Apr 9, 2015

I think the name "check" should be made more specific...maybe check_availability or check_external or something.

The name check is intentionally that generic. At some point, some plugin could check itself or whatever. I don't see any benefit in giving it a more specific name. I could move the code into a new function called check_external and call that from check, but I don't see any benefit in that neither.

And the error message in that method should probably be more specific to its function as well, something like
"Unable to verify availability of required external dependency 'lvm': %s".

The error message going from where? The bd_utils_check_util_version has no idea it's an external dependency, the check function just prints a warning and returns FALSE with no error passed to the caller. And that warning is like "Cannot load the LVM plugin: The 'lvm' utility is not available". I think that's enough specific.

Someday it may seem like the correct idea to load the plugin even if the external dependency is not there at the time.

Yes and I'm gonna bother with that someday. :)

vpodzime added 13 commits April 9, 2015 13:31
Will come handy for plugins' check() functions checking minimum required
versions of tools.
So that their error reporting is nicely usable from Python too.
The default should be a valid string that can be prepended to $PATH, so "." is
much better than "".
Will come handy for plugins' check() functions checking minimum required
versions of tools.
Loading plugins may involve running binaries and the logs from that may be
useful.
When loading plugins we want to check if everything they need is available in
the system. In case of the LVM plugin it's the availability of the 'lvm' tool in
version that is >= the mimimum version we know works.
If there's nothing on stdout, try using stderr. If the exit code was != 0, try
to use the output anyway.
@vpodzime vpodzime force-pushed the master-check_functions branch from 5a56ed3 to 64bcd7f Compare April 9, 2015 13:04
@vpodzime
Copy link
Collaborator Author

vpodzime commented Apr 9, 2015

What I'd love to be the final version of this PR is now pushed and ready for a review.

vpodzime added a commit that referenced this pull request Apr 14, 2015
check functions for plugins (#1201933)
@vpodzime vpodzime merged commit 8dec479 into storaged-project:master Apr 14, 2015
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.

2 participants