Code review comment for lp:~thumper/juju-core/api-juju-run

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM with a couple of tweaks

https://codereview.appspot.com/49960045/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/49960045/diff/1/state/api/client.go#newcode417
state/api/client.go:417:
On 2014/01/10 04:24:39, thumper wrote:
> On 2014/01/10 03:19:03, wallyworld wrote:
> > The above structs should be in params/internal.go right?

> Is there a standard? This needs to be accessed by both the api caller
and the
> apiserver. If internal is the place to put them, I'll move it, but it
wasn't
> clear when writing that they should go there.

I'm not sure TBH. The main point of the comment was that we seem to put
params structs for most everything else in either params.go or
internal.go. Perhaps check with Dimiter or Roger before landing on the
best place to put them.

https://codereview.appspot.com/49960045/diff/20001/state/apiserver/client/run_test.go
File state/apiserver/client/run_test.go (right):

https://codereview.appspot.com/49960045/diff/20001/state/apiserver/client/run_test.go#newcode144
state/apiserver/client/run_test.go:144: func (s *runSuite) mockSSH(c
*gc.C, cmd string) {
Perhaps a brief comment to explain how this method fits into the big
scheme of things - I figured it out but I needed to exercise some brain
cells.

https://codereview.appspot.com/49960045/diff/20001/state/apiserver/client/run_test.go#newcode255
state/apiserver/client/run_test.go:255: client := s.APIState.Client()
Agreed, this is unfortunate :-(

https://codereview.appspot.com/49960045/

« Back to merge proposal