version: fix package and tests #195
Conversation
| "github.com/juju/utils/exec" | ||
| ) | ||
|
|
||
| func osVersion() (string, error) { |
There was a problem hiding this comment.
The idea was that we should keep the majority of the code in shared code, to make testing easier (obviously we'd need to mock out the command execution for this). Doesn't matter much at the moment, since there's no tests.
|
Please check with @wallyworld about SupportedSeries. If we shouldn't be calling it on the client side, then I think the simplestreams code may need to change. |
|
PTAL |
|
s/mustOsVersion/mustOSVersion/ |
|
PTAL |
| @@ -46,11 +60,11 @@ func kernelToMajor(getKernelVersion kernelVersionFunc) (int, error) { | |||
| return int(majorVersion), nil | |||
| } | |||
|
|
|||
| func macOSXSeriesFromKernelVersion(getKernelVersion kernelVersionFunc) string { | |||
| func macOSXSeriesFromKernelVersion(getKernelVersion func() (string, error)) (string, error) { | |||
There was a problem hiding this comment.
Why is MacOS stuff here? Shouldn't it be in osversion_darwin.go?
There was a problem hiding this comment.
It's here so that it can be tested on all platforms.
This is part of a larger problem that the version package is now serving two masters.
One use of version is for the client to try to figure out which tools it needs to upload, as well as simple stuff, like printing its own version with juju version. In this case the version package needs to compile and run on all platforms that the juju cli runs on.
The other use is the server, which is interested in answering questions like 'Which platform am I running on now?", "What is the version string of the tools I need to fetch"
These two use cases are complimentary, and sometimes overlap, but have pulled this package in two independent directions.
|
I think there's a trivial fix so that this can land on still support Ubuntu workloads on WIndows/MacOS clients - only error is ubuntu.csv cannot be found if on a Ubuntu client, otherwise just use the hard coded values. |
|
LGTM |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: dunno why |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
ಠ︵ಠ凸 [f* u] |
|
|
|
Build failed: magic constants |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
On Tue, Jul 1, 2014 at 7:58 PM, Juju bot notifications@github.com wrote:
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
This is futile. On Tue, Jul 1, 2014 at 10:00 PM, Juju bot notifications@github.com wrote:
|
version: fix package and tests Introduce `mustOsVersion`, which replaces `osVersion`, and panics if the version cannot be determined. `osVersion` now returns an error value if it cannot determine the series. Fix the tests so they actually _compile_ and _pass_ on OSX.
Manage multiple tutorial topics
Introduce
mustOsVersion, which replacesosVersion, and panics if the version cannot be determined.osVersionnow returns an error value if it cannot determine the series.Fix the tests so they actually compile and pass on OSX.