Add upstart package with Service and Conf types

Add upstart package with Service and Conf types

Service enables control of, and visibility into, the state of an upstart
service; Conf embeds a Service and additionally allows for installation
(both directly, as used by a machine agent creating a unit agent service,
and as a set of commands to run later, as used when constructing a
cloud-init file to run a machine and/or provisioning agent on a new
instance).

https://code.launchpad.net/~fwereade/juju/go-upstart/+merge/105242

(do not edit description out of merge proposal)

[fwereade, 2012-05-09 17:36:24]

Please take a look.

[niemeyer, 2012-05-17 20:09:14]

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode17
upstart/upstart.go:17: var startedRE = regexp.MustCompile("^.* start/running,
process (\\d+)\n$")
Really? There's no programmatic way to query it? :-(

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode30
upstart/upstart.go:30: func (s *Service) path() string {
s/path/confPath/?

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode52
upstart/upstart.go:52: // Installed returns true if the Service appears to be
installed.
// Installed returns true if the service configuration exists
// in the init directory.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode60
upstart/upstart.go:60: return s.pid() != -1
Why are we getting into the business of verifying pid numbers and checking for
whether the service is being restarted (with Stable)?  I'd expect that to be
handled by upstart itself, and we can just check status (not every service has a
pid, btw, although maybe that doesn't matter for the use case you have in mind).

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode96
upstart/upstart.go:96: // Remove removes the service.
// Remove deletes the service configuration from the init directory.

This is specially important to point out because it's not the typical way in
which a service is disabled with upstart.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode115
upstart/upstart.go:115: envT  = "env %s=%q\n"
This would be a lot more clear if defined as a template, in a single string. You
can pass Conf itself onto the template and let it render it.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode116
upstart/upstart.go:116: outT  = "/tmp/%s.output"
Writing on /tmp/ with well known filenames is a security issue (hint: any user
can create a symbolic link from /tmp/jujud.output into /etc/passwd).

[fwereade, 2012-05-21 10:04:18]

Thanks, nice review. A couple of questions...

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode17
upstart/upstart.go:17: var startedRE = regexp.MustCompile("^.* start/running,
process (\\d+)\n$")
On 2012/05/17 20:09:14, niemeyer wrote:
> Really? There's no programmatic way to query it? :-(

Not that I could discover... do you know of something obvious that I'm missing?

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode60
upstart/upstart.go:60: return s.pid() != -1
On 2012/05/17 20:09:14, niemeyer wrote:
> Why are we getting into the business of verifying pid numbers and checking for
> whether the service is being restarted (with Stable)?  I'd expect that to be
> handled by upstart itself, and we can just check status (not every service has
a
> pid, btw, although maybe that doesn't matter for the use case you have in
mind).

This is in a sense speculative; as I recall I added it to the python version
primarily to enable USE_SUDO testing of real services (upstart won't tell us
about services it's repeatedly restarting... until it finally gives up, that is,
but it takes a good long time before it gives up on a service). I guess we don't
need it now though :).

I don't anticipate us using PIDless services... do you think we might want to?

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode116
upstart/upstart.go:116: outT  = "/tmp/%s.output"
On 2012/05/17 20:09:14, niemeyer wrote:
> Writing on /tmp/ with well known filenames is a security issue (hint: any user
> can create a symbolic link from /tmp/jujud.output into /etc/passwd).

I'd be more than happy to make the top-level output optional, or even maybe drop
it entirely... tbh I never found it very useful when doing the original
stability work, but I'm sure someone was very keen on adding it in review.
Thoughts?

[rog, 2012-05-21 11:07:38]

superficial comments (i'm not familiar with upstart)

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode41
upstart/upstart.go:41: match := startedRE.FindStringSubmatch(string(out))
FindSubmatch(out), and then use string(match[1]) below?

save a tiny bit of garbage :-)

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode115
upstart/upstart.go:115: envT  = "env %s=%q\n"
On 2012/05/17 20:09:14, niemeyer wrote:
> This would be a lot more clear if defined as a template, in a single string.
You
> can pass Conf itself onto the template and let it render it.

emphatic +1

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode174
upstart/upstart.go:174: return err
perhaps annotate this error. an exit status error from Install because the stop
exec fails will be difficult to diagnose.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode190
upstart/upstart.go:190: fmt.Sprintf("cat >> %s << EOF\n%sEOF\n", c.path(),
conf),
s/EOF/'EOF'/
(first EOF only)
unless you *want* to expand metacharacters in the conf text.

[fwereade, 2012-05-28 14:33:10]

Please take a look.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode30
upstart/upstart.go:30: func (s *Service) path() string {
On 2012/05/17 20:09:14, niemeyer wrote:
> s/path/confPath/?

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode41
upstart/upstart.go:41: match := startedRE.FindStringSubmatch(string(out))
On 2012/05/21 11:07:38, rog wrote:
> FindSubmatch(out), and then use string(match[1]) below?
> 
> save a tiny bit of garbage :-)

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode52
upstart/upstart.go:52: // Installed returns true if the Service appears to be
installed.
On 2012/05/17 20:09:14, niemeyer wrote:
> // Installed returns true if the service configuration exists
> // in the init directory.

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode52
upstart/upstart.go:52: // Installed returns true if the Service appears to be
installed.
On 2012/05/17 20:09:14, niemeyer wrote:
> // Installed returns true if the service configuration exists
> // in the init directory.

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode60
upstart/upstart.go:60: return s.pid() != -1
On 2012/05/21 10:04:18, fwereade wrote:
> On 2012/05/17 20:09:14, niemeyer wrote:
> > Why are we getting into the business of verifying pid numbers and checking
for
> > whether the service is being restarted (with Stable)?  I'd expect that to be
> > handled by upstart itself, and we can just check status (not every service
has
> a
> > pid, btw, although maybe that doesn't matter for the use case you have in
> mind).
> 
> This is in a sense speculative; as I recall I added it to the python version
> primarily to enable USE_SUDO testing of real services (upstart won't tell us
> about services it's repeatedly restarting... until it finally gives up, that
is,
> but it takes a good long time before it gives up on a service). I guess we
don't
> need it now though :).
> 
> I don't anticipate us using PIDless services... do you think we might want to?

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode96
upstart/upstart.go:96: // Remove removes the service.
On 2012/05/17 20:09:14, niemeyer wrote:
> // Remove deletes the service configuration from the init directory.
> 
> This is specially important to point out because it's not the typical way in
> which a service is disabled with upstart.

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode115
upstart/upstart.go:115: envT  = "env %s=%q\n"
On 2012/05/21 11:07:38, rog wrote:
> On 2012/05/17 20:09:14, niemeyer wrote:
> > This would be a lot more clear if defined as a template, in a single string.
> You
> > can pass Conf itself onto the template and let it render it.
> 
> emphatic +1

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode116
upstart/upstart.go:116: outT  = "/tmp/%s.output"
On 2012/05/21 10:04:18, fwereade wrote:
> On 2012/05/17 20:09:14, niemeyer wrote:
> > Writing on /tmp/ with well known filenames is a security issue (hint: any
user
> > can create a symbolic link from /tmp/jujud.output into /etc/passwd).
> 
> I'd be more than happy to make the top-level output optional, or even maybe
drop
> it entirely... tbh I never found it very useful when doing the original
> stability work, but I'm sure someone was very keen on adding it in review.
> Thoughts?

Made it optional; we can use it if we want it.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode174
upstart/upstart.go:174: return err
On 2012/05/21 11:07:38, rog wrote:
> perhaps annotate this error. an exit status error from Install because the
stop
> exec fails will be difficult to diagnose.

Done.

https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode190
upstart/upstart.go:190: fmt.Sprintf("cat >> %s << EOF\n%sEOF\n", c.path(),
conf),
On 2012/05/21 11:07:38, rog wrote:
> s/EOF/'EOF'/
> (first EOF only)
> unless you *want* to expand metacharacters in the conf text.

Done.

[rog, 2012-05-28 15:43:14]

LGTM

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode82
upstart/upstart.go:82: {{range $k, $v := .Env}}{{printf "env %s=%q\n" $k
$v}}{{end}}
The %q here is dubious as discussed on irc.

also, i think the printf *might* be more clear expanded into an actual template:

{{range $k, $v := .Env}}env {{$k}}={{$v|printf "%q"}}
{{end}}

then if we do decide to fix the bug sometime, we can replace the printf by a
custom quote function.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode113
upstart/upstart.go:113: func (c *Conf) render() (string, error) {
if you return []byte here, you won't have to convert it again to pass to
ioutil.WriteFile.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117
upstart/upstart.go:117: buf := &bytes.Buffer{}
var buf bytes.Buffer
?

[dave_cheney.net, 2012-05-28 22:37:46]

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode41
upstart/upstart.go:41: cmd := exec.Command("status", s.Name)
what about "/sbin/status", so we don't rely on any PATH environment here. Feel
free to disregard this if it makes testing impossible.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode46
upstart/upstart.go:46: return startedRE.FindSubmatch(out) != nil
I was disappointed to learn that upstart doesn't use exit codes to indicate the
status.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode54
upstart/upstart.go:54: return exec.Command("start", s.Name).Run()
/sbin/start

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode62
upstart/upstart.go:62: return exec.Command("stop", s.Name).Run()
/sbin/stop

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117
upstart/upstart.go:117: buf := &bytes.Buffer{}
On 2012/05/28 15:43:15, rog wrote:
> var buf bytes.Buffer
> ?

var buf = new(bytes.Buffer)

Execute needs an io.Writer which requires a pointer receiver, not a value.
Alternatively

var buf bytes.Buffer; confT.Execute(&buf, c)

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode149
upstart/upstart.go:149: "start " + c.Name,
/sbin/start

[rog, 2012-05-29 07:51:01]

couple more trivial things.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode15
upstart/upstart.go:15: var startedRE = regexp.MustCompile("^.* start/running,
process (\\d+)\n$")
you can use backquotes here and save the \\.

(`\n` is interpreted correctly by regexp.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode46
upstart/upstart.go:46: return startedRE.FindSubmatch(out) != nil
return startedRE.Match(out)

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117
upstart/upstart.go:117: buf := &bytes.Buffer{}
On 2012/05/28 22:37:47, dfc wrote:
> On 2012/05/28 15:43:15, rog wrote:
> > var buf bytes.Buffer
> > ?
> 
> var buf = new(bytes.Buffer)
> 
> Execute needs an io.Writer which requires a pointer receiver, not a value.
> Alternatively
> 
> var buf bytes.Buffer; confT.Execute(&buf, c)

that's what i intended to suggest (even though i didn't mention the &buf bit)

i like the fact that the var decl doesn't have any punctuation, but i realise
which way to use is very much a matter of taste; my feelings aren't particularly
strong here.

[fwereade, 2012-05-29 09:58:31]

Please take a look.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode15
upstart/upstart.go:15: var startedRE = regexp.MustCompile("^.* start/running,
process (\\d+)\n$")
On 2012/05/29 07:51:01, rog wrote:
> you can use backquotes here and save the \\.
> 
> (`\n` is interpreted correctly by regexp.

Oh! Well, goodness me :). Thanks.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode41
upstart/upstart.go:41: cmd := exec.Command("status", s.Name)
On 2012/05/28 22:37:47, dfc wrote:
> what about "/sbin/status", so we don't rely on any PATH environment here. Feel
> free to disregard this if it makes testing impossible.

It's all about the testing :(. Better suggestions, anyone?

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode46
upstart/upstart.go:46: return startedRE.FindSubmatch(out) != nil
On 2012/05/29 07:51:01, rog wrote:
> return startedRE.Match(out)

D'oh. Done.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode82
upstart/upstart.go:82: {{range $k, $v := .Env}}{{printf "env %s=%q\n" $k
$v}}{{end}}
On 2012/05/28 15:43:15, rog wrote:
> The %q here is dubious as discussed on irc.

Added BUG comment.

> also, i think the printf *might* be more clear expanded into an actual
template:
> 
> {{range $k, $v := .Env}}env {{$k}}={{$v|printf "%q"}}
> {{end}}
> 
> then if we do decide to fix the bug sometime, we can replace the printf by a
> custom quote function.

Yeah, that looks nicer. Thanks.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117
upstart/upstart.go:117: buf := &bytes.Buffer{}
On 2012/05/29 07:51:01, rog wrote:
> On 2012/05/28 22:37:47, dfc wrote:
> > On 2012/05/28 15:43:15, rog wrote:
> > > var buf bytes.Buffer
> > > ?
> > 
> > var buf = new(bytes.Buffer)
> > 
> > Execute needs an io.Writer which requires a pointer receiver, not a value.
> > Alternatively
> > 
> > var buf bytes.Buffer; confT.Execute(&buf, c)
> 
> that's what i intended to suggest (even though i didn't mention the &buf bit)
> 
> i like the fact that the var decl doesn't have any punctuation, but i realise
> which way to use is very much a matter of taste; my feelings aren't
particularly
> strong here.

I went with the original suggestion for no exceptionally good reason.

[dave_cheney.net, 2012-05-29 10:01:50]

LGTM.

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode41
upstart/upstart.go:41: cmd := exec.Command("status", s.Name)
Apart from putting all the paths into a var block at the top of the file and
patching them in the test, nothing comes to mind without introducing a context
structure or something.


On 2012/05/29 09:58:31, fwereade wrote:
> On 2012/05/28 22:37:47, dfc wrote:
> > what about "/sbin/status", so we don't rely on any PATH environment here.
Feel
> > free to disregard this if it makes testing impossible.
> 
> It's all about the testing :(. Better suggestions, anyone?

[rog, 2012-05-29 16:49:52]

a few more thoughts after thinking about using this.

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode20
upstart/upstart.go:20: InitDir string // defaults to "/etc/init"
will the upstart daemon notice us if we use a directory other than /etc/init?

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode89
upstart/upstart.go:89: // Conf is responsible for defining and installing
upstart services.
i think the fields in this struct should be documented.

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode94
upstart/upstart.go:94: Cmd  string
just a thought after thinking about using this: should this be []string so that
it's obvious that it's the command and its arguments?

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode107
upstart/upstart.go:107: return errors.New("missing Desc")
should this really be an error?

[fwereade, 2012-05-30 11:22:11]

Please take a look.

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode20
upstart/upstart.go:20: InitDir string // defaults to "/etc/init"
On 2012/05/29 16:49:53, rog wrote:
> will the upstart daemon notice us if we use a directory other than /etc/init?

Depends what directory upstart is looking at ;). It's also useful for testing...

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode89
upstart/upstart.go:89: // Conf is responsible for defining and installing
upstart services.
On 2012/05/29 16:49:53, rog wrote:
> i think the fields in this struct should be documented.

Done.

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode94
upstart/upstart.go:94: Cmd  string
On 2012/05/29 16:49:53, rog wrote:
> just a thought after thinking about using this: should this be []string so
that
> it's obvious that it's the command and its arguments?

Hmm, my feeling is that it's obvious enough already, and I don't really want to
mess around with quoting args here... counterpoint?

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode107
upstart/upstart.go:107: return errors.New("missing Desc")
On 2012/05/29 16:49:53, rog wrote:
> should this really be an error?

Personally, I think we *should* provide descriptions for our jobs. If you
disagree there then, yeah, I guess this shouldn't be an error... but I don't
think it's bad to require this.

[rog, 2012-05-30 11:41:32]

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode94
upstart/upstart.go:94: Cmd  string
On 2012/05/30 11:22:11, fwereade wrote:
> On 2012/05/29 16:49:53, rog wrote:
> > just a thought after thinking about using this: should this be []string so
> that
> > it's obvious that it's the command and its arguments?
> 
> Hmm, my feeling is that it's obvious enough already, and I don't really want
to
> mess around with quoting args here... counterpoint?

counterpoint is that anyone that want to use this package to invoke a command
with arguments containing spaces or odd characters is going to need to know the
quoting convention anyway, so you might as well put it in here and encapsulate
the nih quoting rules, whatever they are.

perhaps Cmd string; Args []string
to match exec.Command?

https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode107
upstart/upstart.go:107: return errors.New("missing Desc")
On 2012/05/30 11:22:11, fwereade wrote:
> On 2012/05/29 16:49:53, rog wrote:
> > should this really be an error?
> 
> Personally, I think we *should* provide descriptions for our jobs. If you
> disagree there then, yeah, I guess this shouldn't be an error... but I don't
> think it's bad to require this.

fair enough.

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode92
upstart/upstart.go:92: // Desc is the upstart job's "description".
s/"description"/description/

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode94
upstart/upstart.go:94: // Env holds the environment variables that will be set
when the job runs.
s/job/command/ ?
(to make it clear that the command will have access to those vars, assuming it
does)

[niemeyer, 2012-05-30 20:55:17]

LGTM

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode32
upstart/upstart.go:32: // Installed returns true if the the service
configuration exists in the
s/the the/the/, but as you're fixing this, would you mind to use the more common
convention (it was my mistake to suggest otherwise):

// Installed returns whether the service ....

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode92
upstart/upstart.go:92: // Desc is the upstart job's "description".
On 2012/05/30 11:41:32, rog wrote:
> s/"description"/description/

+1, and s/job/service/ too.

Or, alternatively, expand the abbreviation and document the fact that Conf is
the upstart service configuration. The field name would be enough of a hint in
this context.

Works either way for me, though.

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode139
upstart/upstart.go:139: return fmt.Errorf("could not remove installed service:
%s", err)
"upstart: could not ..."

[fwereade, 2012-05-31 08:20:56]

*** Submitted:

Add upstart package with Service and Conf types

Service enables control of, and visibility into, the state of an upstart
service; Conf embeds a Service and additionally allows for installation
(both directly, as used by a machine agent creating a unit agent service,
and as a set of commands to run later, as used when constructing a
cloud-init file to run a machine and/or provisioning agent on a new
instance).

R=niemeyer, rog, dfc
CC=
https://codereview.appspot.com/6190061

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go
File upstart/upstart.go (right):

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode32
upstart/upstart.go:32: // Installed returns true if the the service
configuration exists in the
On 2012/05/30 20:55:18, niemeyer wrote:
> s/the the/the/, but as you're fixing this, would you mind to use the more
common
> convention (it was my mistake to suggest otherwise):
> 
> // Installed returns whether the service .... 

Done.

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode92
upstart/upstart.go:92: // Desc is the upstart job's "description".
On 2012/05/30 20:55:18, niemeyer wrote:
> On 2012/05/30 11:41:32, rog wrote:
> > s/"description"/description/
> 
> +1, and s/job/service/ too.
> 
> Or, alternatively, expand the abbreviation and document the fact that Conf is
> the upstart service configuration. The field name would be enough of a hint in
> this context.
> 
> Works either way for me, though.

Done.

https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode94
upstart/upstart.go:94: // Env holds the environment variables that will be set
when the job runs.
On 2012/05/30 11:41:32, rog wrote:
> s/job/command/ ?
> (to make it clear that the command will have access to those vars, assuming it
> does)

Done.