Merge lp:~dimitern/goose/swift-testing-service into lp:~gophers/goose/trunk
| Status: | Merged |
|---|---|
| Merged at revision: | 25 |
| Proposed branch: | lp:~dimitern/goose/swift-testing-service |
| Merge into: | lp:~gophers/goose/trunk |
| Diff against target: |
596 lines (+566/-0) 6 files modified
testservices/swiftservice/service.go (+98/-0) testservices/swiftservice/service_http.go (+151/-0) testservices/swiftservice/service_http_test.go (+205/-0) testservices/swiftservice/service_test.go (+86/-0) testservices/swiftservice/setup_test.go (+10/-0) testservices/swiftservice/swiftservice.go (+16/-0) |
| To merge this branch: | bzr merge lp:~dimitern/goose/swift-testing-service |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| The Go Language Gophers | 2012-11-27 | Pending | |
|
Review via email:
|
|||
Description of the Change
Swift test service double implemented.
Implements a minimal number of OS Swift API, needed for juju testing.
| Dimiter Naydenov (dimitern) wrote : | # |
| John A Meinel (jameinel) wrote : | # |
I think this is reasonable to land as a first draft of the SWIFT
services, and we can just iterate from here.
https:/
File testservices/
https:/
testservices/
already exists %q", name)
If at the HTTP request level it is not an error to AddContainer
something that already exists, I probably wouldn't make it an error at
this level.
Instead, we could just return "(created bool)".
Note that we'll want doc comments for all of these APIs eventually.
https:/
testservices/
strings.
I don't think we want Replace(-1) here. As if the base URL was something
like "foo/" that would replace all occurances. So "foo/foo/foo/" would
end up as just "".
Probably what we really want is just to look if the prefix matches and
then remove it. More like:
if r.URL.Path[
r.URL.Path[
And probably an error if it doesn't match?
https:/
testservices/
"/")
We can probably just unconditionally TrimRight here.
https:/
testservices/
I think that eventually we'll want to have knowledge of multiple users
and tokens, etc. But this is reasonable as a first step.
We'll probably want some generic code for checking Auth-Tokens (residing
in identityservice?)
https:/
testservices/
If err != nil then we say Accepted? Shouldn't that be "if err == nil" ?
Maybe I'm missing the api of AddContainer (it is acceptable to try to
PUT a container that already exists.)
We'll want to make sure to get test coverage of that.
This is the sort of thing where you sort of want TDD, because it is easy
to forget that you handled this case, and then the test case doesn't get
written.
https:/
testservices/
Ultimately, we probably want something like 'gorest' to do the work of
finding what methods need to be connected. But honestly, this doesn't
look too bad.
I would probably split it up more as:
if len(parts) == 1 { DoContainerAPI(
elif len(parts) == 2 {DoObjectAPI(
else { w.Write(ERROR) }
And then the container stuff of GET/DELETE/PUT are all nicely clustered,
and the Object stuff is also nicely clustered, and you don't repeat the
'len(parts)' checks everywhere.
| Martin Packman (gz) wrote : | # |
Seems like a good start, a few cleanup suggestions for the service.
https:/
File testservices/
https:/
testservices/
"/")
I would have container and object name explicitly here and check which
is nil, rather than leaving as parts.
https:/
testservices/
w.WriteHeader(
This throws away the err string and returns no body?
- 24. By Dimiter Naydenov on 2012-11-27
-
Changes after review + some reorganizing
| Dimiter Naydenov (dimitern) wrote : | # |
Please take a look.
https:/
File testservices/
https:/
testservices/
already exists %q", name)
On 2012/11/27 11:03:07, john.meinel wrote:
> If at the HTTP request level it is not an error to AddContainer
something that
> already exists, I probably wouldn't make it an error at this level.
> Instead, we could just return "(created bool)".
> Note that we'll want doc comments for all of these APIs eventually.
Sure I'll add doc comments. Not sure about the error though - it seems
to me nice and consistent this way. Care to elaborate why not?
https:/
testservices/
s.GetObject(
On 2012/11/27 11:13:33, dfc wrote:
> This can be expressed more compactly
> if _, err := ... ; err != nil {
> }
Sure, I was not sure, but I'll do it, 10x
https:/
testservices/
s.HasContainer(
On 2012/11/27 11:13:33, dfc wrote:
> same
> if err := s.AddContainer... ; err != nil {
> return err
> }
Done.
https:/
testservices/
s.GetObject(
On 2012/11/27 11:13:33, dfc wrote:
> same as above
Done.
https:/
testservices/
nil
On 2012/11/27 11:13:33, dfc wrote:
> not sure if you need to nil out the reference here, the delete below
will orphan
> it.
Again, was not sure delete is enough, I'll change it, 10x.
https:/
testservices/
s.GetObject(
On 2012/11/27 11:13:33, dfc wrote:
> same as above
Done.
https:/
testservices/
s.hostname, s.baseURL, container, object), nil
On 2012/11/27 11:13:33, dfc wrote:
> Pedantic people would suggest using path.Join() here, but I think this
is
> reasonable.
Some strings have slashes, some don't so that's why I concatenate them
like this.
https:/
testservices/
strings.
On 2012/11/27 11:03:07, john.meinel wrote:
> I don't think we want Replace(-1) here. As if the base URL was
something like
> "foo/" that would replace all occurances. So "foo/foo/foo/" would end
up as just
> "".
> Probably what we really want is just to look if the prefix matches and
then
> remove it. More like:
> if r.URL.Path[
- 25. By Dimiter Naydenov on 2012-11-27
-
Omission from previous commit
| Dimiter Naydenov (dimitern) wrote : | # |
Please take a look.
| John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 2012-11-27 23:01, Dimiter Naydenov wrote:
> Please take a look.
>
>
> https:/
>
>
File testservices/
>
> https:/
>
>
testservices/
> already exists %q", name) On 2012/11/27 11:03:07, john.meinel
> wrote:
>> If at the HTTP request level it is not an error to AddContainer
> something that
>> already exists, I probably wouldn't make it an error at this
>> level. Instead, we could just return "(created bool)".
>
>> Note that we'll want doc comments for all of these APIs
>> eventually.
>
> Sure I'll add doc comments. Not sure about the error though - it
> seems to me nice and consistent this way. Care to elaborate why
> not?
>
Because at the Openstack HTTP level, it is considered to not be an
error, but a no-op (accepted) case. It seems strange to have no-op be
an error, that we then ignore.
...
> https:/
>
>
testservices/
> s.hostname, s.baseURL, container, object), nil On 2012/11/27
> 11:13:33, dfc wrote:
>> Pedantic people would suggest using path.Join() here, but I think
>> this
> is
>> reasonable.
> Some strings have slashes, some don't so that's why I concatenate
> them like this.
path.Join() would handle if a section has slashes or not (I think). At
least, my guess is that Dave was suggesting using Join() to normalize
the slashes.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with undefined - http://
iEYEARECAAYFAlC
TCQAnj46rVXl6SX
=T6j+
-----END PGP SIGNATURE-----
| John A Meinel (jameinel) wrote : | # |
Overall LGTM.
I'd like to see the tests split up a little bit more, but that's
probably the only thing.
https:/
File testservices/
https:/
testservices/
Did the response really have this crazy spacing? That is surprising.
https:/
testservices/
Did we test that if you pass an invalid token you get the same response
as if you don't pass any token at all? We might, but something we'll
want to test.
https:/
File testservices/
https:/
testservices/
Ultimately, I'd like to see this split out into stuff like:
TestPUTExistsAc
TestPUTMissingC
TestRemoveExistsOK
TestRemoveMissi
etc.
It is ok to have it this way for now.
I realize there will be a small amount of extra setup and teardown in
the tests. However, changing one test will not affect the others, which
is a huge win.
https:/
testservices/
Similarly here.
https:/
File testservices/
https:/
testservices/
As mentioned by others, we'll want doc comments on the type and on the
individual functions.
| Dimiter Naydenov (dimitern) wrote : | # |
Submitting, with the changes suggested.
https:/
File testservices/
https:/
testservices/
On 2012/11/28 10:19:33, jameinel wrote:
> Did the response really have this crazy spacing? That is surprising.
Yes, it's exactly how I sniffed it from the real canonistack session.
https:/
testservices/
On 2012/11/28 10:19:33, jameinel wrote:
> Did we test that if you pass an invalid token you get the same
response as if
> you don't pass any token at all? We might, but something we'll want to
test.
Sure, I'm adding that test.
https:/
File testservices/
https:/
testservices/
On 2012/11/28 10:19:33, jameinel wrote:
> Ultimately, I'd like to see this split out into stuff like:
> TestPUTExistsAc
> TestPUTMissingC
> TestRemoveExistsOK
> TestRemoveMissi
> etc.
> It is ok to have it this way for now.
> I realize there will be a small amount of extra setup and teardown in
the tests.
> However, changing one test will not affect the others, which is a huge
win.
I was thinking of the same thing - it got kinda too much for just 2 test
cases.
https:/
testservices/
On 2012/11/28 10:19:33, jameinel wrote:
> Similarly here.
Done.
https:/
File testservices/
https:/
testservices/
On 2012/11/28 10:19:33, jameinel wrote:
> As mentioned by others, we'll want doc comments on the type and on the
> individual functions.
Done.
| Dimiter Naydenov (dimitern) wrote : | # |
*** Submitted:
Swift test service double implemented.
Implements a minimal number of OS Swift API, needed for juju testing.
| John A Meinel (jameinel) wrote : | # |
LGTM
I have some comments, but don't think it needs another review before
landing.
https:/
File testservices/
https:/
testservices/
*SwiftHTTPSuite) sendRequest(method, path string, body []byte)
(*http.Response, error) {
All callers of this follow up with:
c.Assert(err, IsNil)
c.Assert(
So I would propose to move that code into this function, and simplify
all of the callers.
https:/
testservices/
Where is 'token' being defined? I just realized I don't see its
definition, and it is both here and in the SetUpSuite.
If it is a global variable (which I'm not super excited about, but might
be necessary), then it *definitely* should not just be called "token".
The fact that we sometimes compare it to "" means that not only is it
global state, it is *mutable* global state, which is almost always
incorrect.
https:/
testservices/
The 'removeContainer' here looks like cleanup. So if have a
'ensureNoContainer' that removes it if it exists, but doesn't fail
(removeContainer might already be that), then we can do:
s.ensureNotCont
defer s.removeContain
s.sendRequest(c, "PUT", "test", nil, http.StatusCreated)
And that is the complete test. Though we might take it a step further,
and TearDownTest should be the thing that makes sure you don't leave any
state in the service.
Overall splitting these up looks good to me.
Reviewers: mp+136362_
Message:
Please take a look.
Description:
Swift test service double implemented.
Implements a minimal number of OS Swift API, needed for juju testing.
https:/
(do not edit description out of merge proposal)
Please review this at https:/
Affected files:
A [revision details]
A testservices/
A testservices/
A testservices/
A testservices/