Code review comment for lp:~dimitern/juju-core/330-general-improvements
Revision history for this message
| Roger Peppe (rogpeppe) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
A few more suggestions. It's great that we've lost the testing
dependencies from the code, but there are a few pieces I'm concerned
about.
https:/
File environs/
https:/
environs/
I don't quite see why this and CheckProvisione
exported.
The only reason that I can see is that manual/testing is a separate
package. Given that it's only imported from environs/manual, I don't see
what value that adds. I'd prefer to avoid unnecessary twisty
dependencies.
https:/
File environs/open.go (right):
https:/
environs/
I think andrew has a point here - if ConfigForName returns a useful
error message, it will be lost.
If we're going to deprecate reading config from environments.yaml,
that deserves its own CL, as there is a substantial amount
of cleanup that can and should happen then.
https:/
File errors/errors.go (right):
https:/
errors/errors.go:6: import "fmt"
On 2014/03/11 10:06:33, jameinel wrote:
> I actually prefer the other form.
> Because as soon as we need 2 imports, we want to do it anyway, and
that avoids
> having the noise of what actually changed confused with the extra
lines.
> Certainly not worth going around again, just wanted to give my feeling
that
> multiline imports are still reasonable to use when you only have one.
I think we shouldn't care too much. If goimports inserts a single
import, it does so without the ( ), and that's a useful enough tool that
I think it's reasonable to allow its default output through.
https:/
File juju/api.go (right):
https:/
juju/api.go:104: if !configstore.
As I mentioned in earlier comments, I don't think this is right.
If we want to make it so that we can't open the API without a .jenv
file, that should be a deeper change in this file - there is a fair
amount of logic that can be simplified here.
https:/
File juju/apiconn_
https:/
juju/apiconn_
coretesting.
This is changed only because of the unnecessary Exists patching AFAICS.
https:/
File juju/conn_test.go (right):
https:/
juju/conn_
string) bool {
This seems wrong, as I've mentioned earlier.
It is not at all clear how patching this value will
affect any of the lines of code below - it's truly
action-
fact that the code calls Exists rather than using
the ReadInfo method to infer non-existence of an environment
info (which is a perfectly reasonable implementation).
https:/
File juju/osenv/home.go (right):
https:/
juju/osenv/
Why do we have this?
Any client must call InitJujuHome. If they do not,
it is right to panic.
https:/
File provider/
https:/
provider/
specified here
We should be consistent in the juju init output. Both ec2 and azure have
the descriptions as full sentences. Something like this would seem
better to me (we should really mention the actual environment variables
used too):
# sdc-user holds the SDC user. It can also be specified in
# the SDC_ACCOUNT environment variable.
#
# sdc-user: <username>
https:/
File provider/
https:/
provider/
used for the storage files and
# root-dir overrides the directory...
and below.
https:/
File state/apiserver
https:/
state/apiserver
"launchpad.
Why not just "storage" ?
https:/
File state/apiserver
https:/
state/apiserver
"launchpad.
s/utilsstorage/
https:/
File state/apiserver
https:/
state/apiserver
s.Conn.
please not the dreaded "the" convention!
Given that you've aliased both imports, you can
continue to use just "storage".
https:/
File utils/http.go (right):
https:/
utils/http.go:41: registerFilePro
Why are we doing this?
It seems like a potential security hole to me - there's nothing in the
function documentation to state that the transport also serves files
from /.
https:/
File utils/storage/
https:/
utils/storage/
(storage.Storage, error) {
I don't think this package is justified for this trivial utility
function.
Why not just put the function inside environs?
https:/