Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
add pkg version method (and unit tests) #53
Conversation
petevg
reviewed
Nov 4, 2016
I grumped about the acceptance and return of null values, but if you have a justification for them that I'm missing, I have no problem merging this code ...
| @@ -491,6 +491,31 @@ def get_hadoop_version(): | ||
| return parts[1] | ||
| +def get_package_version(pkg=None): |
petevg
Nov 4, 2016
What is the reason for accepting None here? Wouldn't it be simpler to just force the user to pass in a package?
kwmonroe
Nov 4, 2016
Member
Good point -- no one should call this method without a pkg name.
However, i do still want to make sure the value is truthy. If someone accidentally called this with an empty string, dpkg would happily return the version for every package on the system.
| + hookenv.log( | ||
| + 'Error getting package version: {}'.format(e.output), | ||
| + hookenv.ERROR) | ||
| + return ver_str |
petevg
Nov 4, 2016
I that I'd rather see us pass the exception up to the caller, rather than returning a null value, which might cause exceptions higher up.
If you have a good reason to need the None, feel free to ignore.
kwmonroe
Nov 4, 2016
Member
I opted for returning None in case something prevented dpkg from running (eg, something else has the dpkg lock). Since the main use is just for status output, I didn't want to put the charm in a failed state just because we weren't able to determine the pkg version.
I figured the caller would check for a valid return, and if it was None, they could decide whether to set something (like "unkown"), retry later, or do nothing.
petevg
Nov 4, 2016
@kwmonroe I think that it might make more sense to pass "unknown" or an empty string in the case where we can't get the package. That way, if someone naively joins the string with some other output, they won't necessarily get an error.
kwmonroe
Nov 4, 2016
Member
ack, i'll do empty string so callers can still do ver = get_package_version(foo) or 'unknown'
petevg
commented
Nov 4, 2016
|
This LGTM/+1 (have a slight preference for always returning a string, but I can see the argument for passing back None if we can't get the package version.) |
kwmonroe
added some commits
Nov 4, 2016
|
i've split the difference with ya @petevg. we don't accept |
petevg
commented
Nov 4, 2016
|
+1 :-) |
kwmonroe commentedNov 4, 2016
•
Edited 1 time
-
kwmonroe
Nov 4, 2016
Add helper to return a version string given a package name (currently, only dpkg is supported). This is useful for
hookenv.application_version_setwhen we only have the package manager to rely on.