Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
HostSeries no longer panics #246
Conversation
| }) | ||
| + return series, err |
rogpeppe
Oct 14, 2016
Owner
This doesn't seem quite right. If HostSeries returns an error the first time, it'll return "", nil the second time AFAICS.
kat-co
Oct 14, 2016
Contributor
Whoops, good catch! I've taken your suggestion and just set series to UnknownSeries for now.
| + return series, err | ||
| +} | ||
| + | ||
| +// MustHostSeries calls HostSeries and panics if there is an error. |
rogpeppe
Oct 14, 2016
Owner
I wouldn't bother with this function unless you find yourself needing it a lot.
kat-co
Oct 14, 2016
Contributor
There are several places in Juju that currently rely on this behavior panicing. I don't plan on tackling that issue other than re-pointing those areas to this function.
| +// HostSeries returns the series of the machine the current process is | ||
| +// running on. | ||
| +// | ||
| +// This is not a concurrent-safe operation. |
| +// | ||
| +// This is not a concurrent-safe operation. | ||
| +func HostSeries() (string, error) { | ||
| + series, err := readSeries() |
rogpeppe
Oct 14, 2016
•
Edited 1 time
-
rogpeppe
Oct 14, 2016
Owner
-
Oct 14, 2016
or just:
return readSeries()
the trace doesn't add much IMHO when there's only one error path.
kat-co
Oct 14, 2016
Contributor
I prefer the errors.Trace so that it gives the path through the code when the error is output.
| - | ||
| - seriesOnce sync.Once | ||
| - series string // filled in by the first call to hostSeries | ||
| + logger = loggo.GetLogger("juju.juju.series") |
rogpeppe
Oct 14, 2016
Owner
Perhaps move this definition to the only file that uses it (supportedseries.go)
and perhaps add a TODO to remove the globals there?
| seriesOnce.Do(func() { | ||
| - var err error | ||
| series, err = readSeries() |
rogpeppe
Oct 14, 2016
•
Edited 1 time
-
rogpeppe
Oct 14, 2016
Owner
-
Oct 14, 2016
I'd suggest you declare err inside the function.
| "github.com/juju/utils/os" | ||
| ) | ||
| +var ( | ||
| + // TODO(katco): Remove this global |
| seriesOnce sync.Once | ||
| - series string // filled in by the first call to hostSeries | ||
| + // These are filled in by the first call to hostSeries | ||
| + series string |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils |
kat-co commentedOct 14, 2016
This is to help address lp:1633495.
HostSeries unecessarily panics. Any function that panics should be prepended with the keyword "Must". I have introduced a function which does just this in case callers want to continue to panic.
Also removed unecessary global variable referencing an unexported function.