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

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

I have an initial concern about the need for apiConfig - see comments in
code.

https://codereview.appspot.com/49960045/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/49960045/diff/1/cmd/jujud/machine.go#newcode310
cmd/jujud/machine.go:310: })
This is clumsy - port, cert, key all are derived from agentConfig, aka
a.Conf.config

So we should include dataDir with the result of APIServerDetails and
pass that in

https://codereview.appspot.com/49960045/diff/1/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/49960045/diff/1/provider/dummy/environs.go#newcode601
provider/dummy/environs.go:601: estate.apiServer, err =
apiserver.NewServer(st, "localhost:0", []byte(testing.ServerCert),
[]byte(testing.ServerKey), config)
As per my other comment, just pass in dataDir

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:
The above structs should be in params/internal.go right?

https://codereview.appspot.com/49960045/diff/1/state/api/client.go#newcode437
state/api/client.go:437:
Ditto

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

https://codereview.appspot.com/49960045/diff/1/state/apiserver/apiserver.go#newcode33
state/apiserver/apiserver.go:33: agentConfig agent.Config
Just dataDir would be better I think

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

https://codereview.appspot.com/49960045/diff/1/state/apiserver/client/run.go#newcode168
state/apiserver/client/run.go:168: outstanding.Wait()
Should there be some sort of timeout? Will this hang if an individual
command hangs?

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

« Back to merge proposal