Code review comment for lp://staging/~themue/golxc/001-added-sudo-and-first-test
Revision history for this message
| Roger Peppe (rogpeppe) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
513534c
demo site
(Get the code!)
LGTM apart from the error messages. i think we don't want to discard the
important information printed to stderr.
https:/
File golxc.go (right):
https:/
golxc.go:9: // Frank Mueller <email address hidden>
On 2012/11/20 16:34:04, TheMue wrote:
> On 2012/11/20 15:20:11, rog wrote:
> > do we need individual attributions?
> Got it from packages like goamz. But personally don't need it.
i think goamz is different because it really is almost all gustavo's
work. YMMV, whatever.
https:/
golxc.go:370: log.Fatalf("error executing %q: %v",
strings.
On 2012/11/20 16:34:04, TheMue wrote:
> On 2012/11/20 15:20:11, rog wrote:
> > d
> > but i wonder whether the error message here should actually
incorporate the
> > error printed to stderr. nothing that calls run actually uses the
stderr
> > returned, and the error returned by cmd.Run is, in most cases,
almost
> > meaningless.
> The err here is an error of Command executing the command, not the
stderr of the
> executed command. I changed it to mode it more clear. So far we have
no usage
> for stderr, but it's also not everything implemented. If there stays
no need it
> will be removed.
my point is that if a command fails, the only message we'll get will be
"exit status 1" or whatever, and the useful error message that the
command has printed to stderr will be discarded.
i think we should put at least some of that information into the err
return.
https:/
File golxc.go (right):
https:/
golxc.go:35: // LogLevel represents a containers log level.
s/containers/
https:/
golxc.go:86: c.logLevel = logLevel
is it really worth having the setter method here?
could we just export the relevant fields?
https:/
golxc.go:273: func (c *Container) wait(states ...State) error {
given that nothing is actually making use of the functionality yet, we
could just have wait(state State) and not bother with combining states
at all. just a then this function is only 5 lines. just a thought.
https:/