Add ec2test package for fake EC2 server.

https://code.launchpad.net/~rogpeppe/goamz/ec2-ec2test/+merge/83196

(do not edit description out of merge proposal)

[niemeyer, 2011-12-01 14:51:07]

Very good stuff in this branch, thanks.

https://codereview.appspot.com/5433056/diff/2001/ec2.go
File ec2.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2.go#newcode213
ec2.go:213: State          InstanceState
The tag name seems to be "instanceState", in which case we'll need a field tag
here. That said, there are apparently no tests for this.

https://codereview.appspot.com/5433056/diff/2001/ec2.go#newcode374
ec2.go:374: XMLName   xml.Name
Looks like the left over is back.

https://codereview.appspot.com/5433056/diff/2001/ec2t_test.go
File ec2t_test.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2t_test.go#newcode23
ec2t_test.go:23: aws.Auth{},
The fact we can get away with no auth is an indication that the ec2test server
is doing a bad job at testing that. Let's solve that in a future CL.

https://codereview.appspot.com/5433056/diff/2001/ec2t_test.go#newcode28
ec2t_test.go:28: s.i.TestRunAndTerminate(c)
Interesting. It's cool that we can run selected tests against it.

https://codereview.appspot.com/5433056/diff/2001/ec2test/filter.go
File ec2test/filter.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2test/filter.go#newcode9
ec2test/filter.go:9: // filter holds an ec2 filter.
Would be good to have a link or a summary of what is an EC2 filter.

Overall structure of filter below looks good.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go
File ec2test/server.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode25
ec2test/server.go:25: // Request holds the requested action as a url.Values
instance
Why is it an interface{} then?

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode45
ec2test/server.go:45: // It partially simulates the state for a single ec2
session.
The concept of an "ec2 session" is not well defined. The first sentence is
probably enough as a description.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode46
ec2test/server.go:46: type Server struct {
This type rings a bell. It looks good.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode73
ec2test/server.go:73: itype       string
itype feels a bit vague. instType?

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode91
ec2test/server.go:91: "AuthorizeSecurityGroupIngress": (*Server).notImplemented,
Nice.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode185
ec2test/server.go:185: func (srv *Server) NextFailure() string {
I suggest naming this as PopFailure, so we know when using it that it's going to
clean the failure state.

That said, this is a weird API to have public. Feels like it should really be
private, which means the function is not even necesary. Just embed this logic
into scheduledFailure.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode236
ec2test/server.go:236: log.Printf("error marshalling %#v: %v", x, err)
Yes, I think panicking here is the right thing to do. Any marshalling errors are
a logic mistake on our part.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode271
ec2test/server.go:271: func (srv *Server) runInstances(w http.ResponseWriter,
req *http.Request, reqId string) interface{} {
Nice logic here.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode346
ec2test/server.go:346: // TODO do we ever actually want to delete instances?
For now at least we don't. EC2 has a grace period where it actually shows them
terminated. We may want to enable external tweaking of that at some point in the
future with the goal of error checking, but for now what we've got here looks
great.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode355
ec2test/server.go:355: // TODO should probably just give an error
Possibly. We have to check what EC2 does for real.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode415
ec2test/server.go:415: type instanceState int
Why not "type instanceState string"?

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode467
ec2test/server.go:467: return &createSecurityGroupResponse{
I suspect the documentation of the Response field of Action is lying. It may be
a better idea to not name names there, so we don't have to maintain that list.

https://codereview.appspot.com/5433056/diff/2001/suite_test.go
File suite_test.go (left):

https://codereview.appspot.com/5433056/diff/2001/suite_test.go#oldcode46
suite_test.go:46: }
As we discussed previously, please undo this change.

[rog, 2011-12-01 15:41:52]

PTAL

https://codereview.appspot.com/5433056/diff/2001/ec2.go
File ec2.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2.go#newcode213
ec2.go:213: State          InstanceState
On 2011/12/01 14:51:07, niemeyer wrote:
> The tag name seems to be "instanceState", in which case we'll need a field tag
> here. That said, there are apparently no tests for this.

deleted by merging from trunk.

https://codereview.appspot.com/5433056/diff/2001/ec2.go#newcode374
ec2.go:374: XMLName   xml.Name
deleted by merging from trunk.

https://codereview.appspot.com/5433056/diff/2001/ec2t_test.go
File ec2t_test.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2t_test.go#newcode28
ec2t_test.go:28: s.i.TestRunAndTerminate(c)
On 2011/12/01 14:51:07, niemeyer wrote:
> Interesting. It's cool that we can run selected tests against it.

eventually the plan is to embed SuiteI and have additional ec2test-specific
tests.

https://codereview.appspot.com/5433056/diff/2001/ec2test/filter.go
File ec2test/filter.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2test/filter.go#newcode9
ec2test/filter.go:9: // filter holds an ec2 filter.
On 2011/12/01 14:51:07, niemeyer wrote:
> Would be good to have a link or a summary of what is an EC2 filter.
> 
> Overall structure of filter below looks good.

Done.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go
File ec2test/server.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode25
ec2test/server.go:25: // Request holds the requested action as a url.Values
instance
On 2011/12/01 14:51:07, niemeyer wrote:
> Why is it an interface{} then?

good point.
because originally i had individual types for each request, but gave that up as
unnecessary.
done.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode45
ec2test/server.go:45: // It partially simulates the state for a single ec2
session.
On 2011/12/01 14:51:07, niemeyer wrote:
> The concept of an "ec2 session" is not well defined. The first sentence is
> probably enough as a description.

Done.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode73
ec2test/server.go:73: itype       string
On 2011/12/01 14:51:07, niemeyer wrote:
> itype feels a bit vague. instType? 

well, if the language allowed me, i would have "type" which is even more vague
(but it's not, of course, because it's in the instance).

kind?

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode185
ec2test/server.go:185: func (srv *Server) NextFailure() string {
On 2011/12/01 14:51:07, niemeyer wrote:
> I suggest naming this as PopFailure, so we know when using it that it's going
to
> clean the failure state.
> 
> That said, this is a weird API to have public. Feels like it should really be
> private, which means the function is not even necesary. Just embed this logic
> into scheduledFailure.

the reason it's public is that a given juju operation may make no calls into the
ec2 API at all, in which case there's no operation to fail, so jujutest will
find that the operation succeeds unexpectedly.

my current solution to that is that the juju/ec2 testing code knows when no
operation will be performed, so calls NextFailure and fails directly if so.

we need to think more about the whole "inducing failure" thing.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode236
ec2test/server.go:236: log.Printf("error marshalling %#v: %v", x, err)
On 2011/12/01 14:51:07, niemeyer wrote:
> Yes, I think panicking here is the right thing to do. Any marshalling errors
are
> a logic mistake on our part.

Done.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode346
ec2test/server.go:346: // TODO do we ever actually want to delete instances?
On 2011/12/01 14:51:07, niemeyer wrote:
> For now at least we don't. EC2 has a grace period where it actually shows them
> terminated. We may want to enable external tweaking of that at some point in
the
> future with the goal of error checking, but for now what we've got here looks
> great.

perhaps i'll leave the comment (and the deletion code) in there just so it shows
what's necessary when we come to it.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode415
ec2test/server.go:415: type instanceState int
On 2011/12/01 14:51:07, niemeyer wrote:
> Why not "type instanceState string"?

because we need the numbers too, and mapping from int to string seems marginally
better than the other way around.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode467
ec2test/server.go:467: return &createSecurityGroupResponse{
On 2011/12/01 14:51:07, niemeyer wrote:
> I suspect the documentation of the Response field of Action is lying. It may
be
> a better idea to not name names there, so we don't have to maintain that list.

i'm not sure what you mean here

[niemeyer, 2011-12-01 17:06:58]

LGTM, assuming the following points are addressed:

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go
File ec2test/server.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode73
ec2test/server.go:73: itype       string
> kind?

We don't own the naming in this context, so changing it will be more confusing
than instType, or typeName, or similar.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode185
ec2test/server.go:185: func (srv *Server) NextFailure() string {
> we need to think more about the whole "inducing failure" thing.

Agreed, and we also already figured other problems with merely saying "next"
failure. There are several smells that something else is necessary. Given those,
can we please drop all of the current logic around planned failure until we
figure how we want to do this?

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode415
ec2test/server.go:415: type instanceState int
> because we need the numbers too, and mapping from int to 

Understood, thanks.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode467
ec2test/server.go:467: return &createSecurityGroupResponse{
> i'm not sure what you mean here

See the other comment.

https://codereview.appspot.com/5433056/diff/6001/ec2_test.go
File ec2_test.go (right):

https://codereview.appspot.com/5433056/diff/6001/ec2_test.go#newcode222
ec2_test.go:222: c.Assert(len(req.Form["Filter.1.Value.2"]), Equals, 0)
IsNil

https://codereview.appspot.com/5433056/diff/6001/ec2_test.go#newcode282
ec2_test.go:282: c.Assert(len(g1ipp.SourceIPs), Equals, 0)
IsNil

https://codereview.appspot.com/5433056/diff/6001/ec2test/server.go
File ec2test/server.go (right):

https://codereview.appspot.com/5433056/diff/6001/ec2test/server.go#newcode32
ec2test/server.go:32: //	*ec2.SimpleResp
Please drop the list of types above and use a comment similar to this:

// If the action succeeded, Response holds the value that
// was marshalled to build the XML response for the request.

[rog, 2011-12-02 12:10:42]

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go
File ec2test/server.go (right):

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode73
ec2test/server.go:73: itype       string
On 2011/12/01 17:06:58, niemeyer wrote:
> > kind?
> 
> We don't own the naming in this context, so changing it will be more confusing
> than instType, or typeName, or similar.

Done.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode185
ec2test/server.go:185: func (srv *Server) NextFailure() string {
On 2011/12/01 17:06:58, niemeyer wrote:
> > we need to think more about the whole "inducing failure" thing.
> 
> Agreed, and we also already figured other problems with merely saying "next"
> failure. There are several smells that something else is necessary. Given
those,
> can we please drop all of the current logic around planned failure until we
> figure how we want to do this?

Done.

https://codereview.appspot.com/5433056/diff/2001/ec2test/server.go#newcode467
ec2test/server.go:467: return &createSecurityGroupResponse{
On 2011/12/01 17:06:58, niemeyer wrote:
> > i'm not sure what you mean here
> 
> See the other comment.

oh yes. that's a bug actually.

https://codereview.appspot.com/5433056/diff/6001/ec2_test.go
File ec2_test.go (right):

https://codereview.appspot.com/5433056/diff/6001/ec2_test.go#newcode222
ec2_test.go:222: c.Assert(len(req.Form["Filter.1.Value.2"]), Equals, 0)
On 2011/12/01 17:06:58, niemeyer wrote:
> IsNil

Done.

https://codereview.appspot.com/5433056/diff/6001/ec2_test.go#newcode282
ec2_test.go:282: c.Assert(len(g1ipp.SourceIPs), Equals, 0)
On 2011/12/01 17:06:58, niemeyer wrote:
> IsNil

Done.

https://codereview.appspot.com/5433056/diff/6001/ec2test/server.go
File ec2test/server.go (right):

https://codereview.appspot.com/5433056/diff/6001/ec2test/server.go#newcode32
ec2test/server.go:32: //	*ec2.SimpleResp
On 2011/12/01 17:06:58, niemeyer wrote:
> Please drop the list of types above and use a comment similar to this:
> 
> // If the action succeeded, Response holds the value that
> // was marshalled to build the XML response for the request.

Done.

[niemeyer, 2011-12-02 12:29:57]

LGTM, thank you.