goose: all current code

goose: all current code

This CL is a way for anyone to comment on
the current state of the Goose world.

https://code.launchpad.net/~rogpeppe/goose/state-of-the-world/+merge/136144

(do not edit description out of merge proposal)

[jameinel, 2012-11-26 11:18:45]

Just adding a comment so that I get subscribed to any discussions on this MP.

[rog, 2012-11-26 12:51:37]

lots of comments but mostly superficial.

i haven't looked at the tests at all yet, and i have no knowledge of openstack
at all, so can't comment on the deeper aspects of the code.

https://codereview.appspot.com/6844087/diff/1/client/client.go
File client/client.go (right):

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode15
client/client.go:15: OS_API_TOKENS               = "/tokens"
CAPITAL_LETTER_CONSTANTS aren't conventional in Go. Moreover, it looks like
these aren't intended to be part of the public API.

how about:

const (
    apiTokens = "/tokens"
    apiFlavors = ... etc
)

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode24
client/client.go:24: GET    = "GET"
unexport. and... can we just use the string constants? i don't see that defining
constants here adds greatly to safety.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode32
client/client.go:32: type OpenStackClient struct {
s/OpenStackClient/Client/ ?

after all goose is all about openstack - i doubt we need to restate that.

also, needs a doc comment.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode33
client/client.go:33: client *goosehttp.GooseHTTPClient
goosehttp.Client would be a better name.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode39
client/client.go:39: ServiceURLs map[string]string
doc comment (what's the key and value in the map?)

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode45
client/client.go:45: func NewOpenStackClient(creds *identity.Credentials,
auth_method int) *OpenStackClient {
doc comment

also, s/auth_method/authMethod/

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode51
client/client.go:51: case identity.AUTH_LEGACY:
given that the identify package defines these constants and implements the
authentication schemes, how about

   client.auth, err = identity.AuthMethod(authMethod)

let the identity package implement this logic.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode59
client/client.go:59: func (c *OpenStackClient) Authenticate() (err error) {
doc comment.

if you don't use AddErrorContext, you won't need to name the variable.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode60
client/client.go:60: err = nil
d

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode62
client/client.go:62: return fmt.Errorf("Authentication method has not been
specified")
s/fmt.Errorf/errors.New/

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode66
client/client.go:66: gooseerrors.AddErrorContext(&err, "authentication failed")
return fmt.Errorf("authentication failed: %v", err)

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode70
client/client.go:70: c.TokenId = authDetails.TokenId
might it make sense to simply add the auth details as a field in the
OpenStackClient type?

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode77
client/client.go:77: func (c *OpenStackClient) IsAuthenticated() bool {
doc comment.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode81
client/client.go:81: type Link struct {
doc comment.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode88
client/client.go:88: // Contains a list of links.
the second sentence doesn't add much info. i can see it contains a list of links
from the type, but what is their significance?

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode95
client/client.go:95: func (c *OpenStackClient) ListFlavors() (flavors []Entity,
err error) {
doc comment.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode110
client/client.go:110: type FlavorDetail struct {
doc comment. and on the fields too. (what kind of thing does the Disk field
hold? the number of disks?).
alternatively, a URL to the relevant piece of documentation can be good.

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode116
client/client.go:116: Swap  interface{} // Can be an empty string (?!)
What type should we expect here?

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode119
client/client.go:119: func (c *OpenStackClient) ListFlavorsDetail() (flavors
[]FlavorDetail, err error) {
doc comment.
it's not clear to me how the FlavorDetails returned by this method mesh with the
Entities returned by ListFlavors. do the Name fields match?

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode127
client/client.go:127: gooseerrors.AddErrorContext(&err, "failed to get list of
flavors details")
fmt.Errorf

https://codereview.appspot.com/6844087/diff/1/client/client.go#newcode142
client/client.go:142: gooseerrors.AddErrorContext(&err, "failed to get list of
servers")
fmt.Errorf

https://codereview.appspot.com/6844087/diff/1/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/6844087/diff/1/errors/errors.go#newcode13
errors/errors.go:13: func AddErrorContext(err *error, format string, args
...interface{}) {
if we're going to have this function, can we name it the same as in juju-core -
ErrorContextf ?
might be worth naming the package "trivial" for the same reason, making code
more easily moveable between the two projects if necessary.

BTW the entire point of the signature of ErrorContextf is to make it possible to
defer it and have an error context automatically added. Using it instead of
fmt.Errorf misses the point.

https://codereview.appspot.com/6844087/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode16
http/client.go:16: type GooseHTTPClient struct {
s/GooseHTTPClient/Client/

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode20
http/client.go:20: type ErrorResponse struct {
looks like this doesn't need to exported.

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode27
http/client.go:27: return fmt.Sprintf("Failed: %d %s: %s", e.Code, e.Title,
e.Message)
goose request failed: 
?

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode30
http/client.go:30: type ErrorWrapper struct {
ditto

also, it might be nice to include some context on it was that failed (e.g. the
url) rather than just "Failed: ".

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode34
http/client.go:34: type RequestData struct {
doc comment

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode51
http/client.go:51: err = nil
d (it starts off as nil)

and lose the named error return. they're prone to errors in more involved
functions.

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode55
http/client.go:55: )
rather than having two piece of code that call http.NewRequest, why not
something like this?

var body string
if reqData.ReqValue != nil {
     body = ...
}
req, err := http.NewRequest(method, url, strings.NewReader(body))

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode59
http/client.go:59: gooseerrors.AddErrorContext(&err, "failed marshalling the
request body")
return fmt.Errorf("failed ...: %v", err)

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode68
http/client.go:68: gooseerrors.AddErrorContext(&err, "failed creating the
request")
fmt.Errorf

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode83
http/client.go:83: gooseerrors.AddErrorContext(&err, "failed unmarshaling the
response body: %s", respBody)
fmt.Errorf

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode90
http/client.go:90: // Sends the supplied byte array (if any) to the specified
URL.
BinaryRequest sends

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode134
http/client.go:134: if extraHeaders != nil {
d
(it's fine to range on a nil map)

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode144
http/client.go:144: gooseerrors.AddErrorContext(&err, "failed executing the
request")
this context is not very useful. some information about the request that failed
might make it so.

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode157
http/client.go:157: if !foundStatus && len(expectedStatus) > 0 {
if !foundStatus {

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode163
http/client.go:163: var wrappedErr ErrorWrapper
var wrappedErr struct {
    Error ErrorResponse `json:"error"`
}

and lose the WrappedError type ?

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode168
http/client.go:168: err = errors.New(
fmt.Errorf
but... since the remote side has gone to the trouble of providing a nice json
error response, and we're parsing it, it would only seem polite to make that
information available in the error we're returning.

that is, instead of having the polymorphic errInfo type, we could have two
branches here, one which returns the ErrorResponse; the other would return a
string form as a fallback.

https://codereview.appspot.com/6844087/diff/1/http/client.go#newcode178
http/client.go:178: respBody, err = ioutil.ReadAll(rawResp.Body)
i think it's probably better to return the response directly here rather than
slurping it into a []byte, so the caller can see what response was actually
returned, for example. YMMV though.

https://codereview.appspot.com/6844087/diff/1/identity/identity.go
File identity/identity.go (right):

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode8
identity/identity.go:8: AUTH_LEGACY = iota
authLegacy ?

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode12
identity/identity.go:12: type AuthDetails struct {
doc comment

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode19
identity/identity.go:19: type Credentials struct {
doc comment

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode27
identity/identity.go:27: type Authenticator interface {
doc comment - particularly crucial here.

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode34
identity/identity.go:34: value = os.Getenv(v)
if value := os.Getenv(v); value != "" {
    return value
}

then return "" at the end and lose the named return variable.

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode42
identity/identity.go:42: func CredentialsFromEnv() *Credentials {
doc comment.

are some of these credentials optional? ISTM that some reflection code in
environ/openstack could be avoided if this function returned an error.

https://codereview.appspot.com/6844087/diff/1/identity/legacy.go
File identity/legacy.go (right):

https://codereview.appspot.com/6844087/diff/1/identity/legacy.go#newcode15
identity/legacy.go:15: l.client = &http.Client{CheckRedirect: nil}
s/CheckRedirect: nil//

or just l.client = new(http.Client)

https://codereview.appspot.com/6844087/diff/1/identity/legacy.go#newcode24
identity/legacy.go:24: defer response.Body.Close()
this will crash if the Do request fails. move it after the error check.

https://codereview.appspot.com/6844087/diff/1/identity/legacy.go#newcode33
identity/legacy.go:33: details := &AuthDetails{}
var details AuthDetails ?

https://codereview.appspot.com/6844087/diff/1/identity/legacy.go#newcode36
identity/legacy.go:36: return nil, fmt.Errorf("Did not get valid Token from auth
request")
generally we keep error messages as lower case.

"token not found in auth request" ?

https://codereview.appspot.com/6844087/diff/1/identity/legacy.go#newcode40
identity/legacy.go:40: return nil, fmt.Errorf("Did not get valid management URL
from auth request")
"management URL not found in auth request" ?

https://codereview.appspot.com/6844087/diff/1/identity/userpass.go
File identity/userpass.go (right):

https://codereview.appspot.com/6844087/diff/1/identity/userpass.go#newcode9
identity/userpass.go:9: type PasswordCredentials struct {
do all the below types need to be exported?
it looks to me like they're part of the internal implementation of UserPass.

https://codereview.appspot.com/6844087/diff/1/identity/userpass.go#newcode69
identity/userpass.go:69: type UserPass struct {
doc comment.

https://codereview.appspot.com/6844087/diff/1/testing/envsuite/envsuite.go
File testing/envsuite/envsuite.go (right):

https://codereview.appspot.com/6844087/diff/1/testing/envsuite/envsuite.go#ne...
testing/envsuite/envsuite.go:3: // Provides an EnvSuite type which makes sure
this test suite gets an isolated
this should be a package level comment.

// The envsuite package provides ...
package envsuite


but to be honest, i'm not sure this or httpsuite justifies its own package -
they could both live together quite happily in the testing package itself.

https://codereview.appspot.com/6844087/diff/1/testservices/identityservice/us...
File testservices/identityservice/userpass.go (right):

https://codereview.appspot.com/6844087/diff/1/testservices/identityservice/us...
testservices/identityservice/userpass.go:12: type ErrorResponse struct {
do all the below types need to be exported?

it makes it much easier to work out what's important about a package if only
externally necessary parts are exported.

https://codereview.appspot.com/6844087/diff/1/testservices/identityservice/ut...
File testservices/identityservice/util.go (right):

https://codereview.appspot.com/6844087/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:16: raw_bytes := make([]byte, 16)
rawBytes
or just "raw", or "buf"?

the latter is probably most conventional.

https://codereview.appspot.com/6844087/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:17: n, err := rand.Read(raw_bytes)
io.ReadFull

https://codereview.appspot.com/6844087/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:18: if n != 16 || err != nil {
if err != nil {

https://codereview.appspot.com/6844087/diff/1/testservices/identityservice/ut...
testservices/identityservice/util.go:22: }
return fmt.Sprintf("%x", ...)

https://codereview.appspot.com/6844087/diff/1/testservices/main.go
File testservices/main.go (right):

https://codereview.appspot.com/6844087/diff/1/testservices/main.go#newcode14
testservices/main.go:14: }
func (uv userInfo) String() {
    return fmt.Sprint("%s:%s", user, secret)
}

not that it would actually get used unless you've got some defaults set up.

https://codereview.appspot.com/6844087/diff/1/testservices/main.go#newcode20
testservices/main.go:20: vals := strings.Split(s, ":")
the secret can't contain a colon?

[jameinel, 2012-11-26 16:30:58]

https://codereview.appspot.com/6844087/diff/1/identity/identity.go
File identity/identity.go (right):

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode8
identity/identity.go:8: AUTH_LEGACY = iota
On 2012/11/26 12:51:38, rog wrote:
> authLegacy ?

This one is actually part of the public api. Though AuthLegacy would be ok.

[rog, 2012-11-26 18:27:35]

https://codereview.appspot.com/6844087/diff/1/identity/identity.go
File identity/identity.go (right):

https://codereview.appspot.com/6844087/diff/1/identity/identity.go#newcode8
identity/identity.go:8: AUTH_LEGACY = iota
On 2012/11/26 16:30:58, john.meinel wrote:
> On 2012/11/26 12:51:38, rog wrote:
> > authLegacy ?
> 
> This one is actually part of the public api. Though AuthLegacy would be ok.

yeah, i saw that. although, given that the auth method is an argument to
NewOpenStackClient, perhaps it wouldn't be unreasonable to ask clients to pass
in an auth method value themselves, rather than a constant that implies one.
alternatively, you could make these constants of a type that has a Method()
Authenticator method.

requiring external clients to enumerate all the possible auth methods when we
can easily make the linkage here seems somewhat unnecessary.

i'm probably missing something crucial though.

[jameinel, 2012-11-27 04:26:34]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...


>> This one is actually part of the public api. Though AuthLegacy
>> would
> be ok.
> 
> yeah, i saw that. although, given that the auth method is an
> argument to NewOpenStackClient, perhaps it wouldn't be unreasonable
> to ask clients to pass in an auth method value themselves, rather
> than a constant that implies one. alternatively, you could make
> these constants of a type that has a Method() Authenticator
> method.
> 
> requiring external clients to enumerate all the possible auth
> methods when we can easily make the linkage here seems somewhat
> unnecessary.
> 
> i'm probably missing something crucial though.
> 

Ultimately the auth that we pick has to come from the
environments.yaml file. Because it depends heavily on what openstack
cloud we are connecting to. (HP's auth is different from Rackspace, is
different from Canonistack, etc.)

So ideally the outside-of-goose level would just be a string request
of an auth to use, of a specific set of enumerated constants.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iEYEARECAAYFAlC0QPcACgkQJdeBCYSNAANURwCgzxS64UxSnFL2spmLcUEbtKPI
7Q4AoLlK1FjFBaLcHgdxoaARbM193kSx
=COy9
-----END PGP SIGNATURE-----

[rog, 2012-11-27 14:44:36]

On 27 November 2012 04:26, John Arbash Meinel <john.meinel@canonical.com>
wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> ...
>
>
>>> This one is actually part of the public api. Though AuthLegacy
>>> would
>> be ok.
>>
>> yeah, i saw that. although, given that the auth method is an
>> argument to NewOpenStackClient, perhaps it wouldn't be unreasonable
>> to ask clients to pass in an auth method value themselves, rather
>> than a constant that implies one. alternatively, you could make
>> these constants of a type that has a Method() Authenticator
>> method.
>>
>> requiring external clients to enumerate all the possible auth
>> methods when we can easily make the linkage here seems somewhat
>> unnecessary.
>>
>> i'm probably missing something crucial though.
>>
>
> Ultimately the auth that we pick has to come from the
> environments.yaml file. Because it depends heavily on what openstack
> cloud we are connecting to. (HP's auth is different from Rackspace, is
> different from Canonistack, etc.)
>
> So ideally the outside-of-goose level would just be a string request
> of an auth to use, of a specific set of enumerated constants.

if this is the case, then something that maps from string to authenticator
type might seem more appropriate - the intermediate iota-based constants
still seem a little redundant.