SimianArmy-pull-requests #44 FAILURE
Looks like there's a problem with this pull request

SimianArmy-pull-requests #47 FAILURE
Looks like there's a problem with this pull request

SimianArmy-pull-requests #48 FAILURE
Looks like there's a problem with this pull request

SimianArmy-pull-requests #49 FAILURE
Looks like there's a problem with this pull request

SimianArmy-pull-requests #51 SUCCESS
This pull request looks good

SimianArmy-pull-requests #52 SUCCESS
This pull request looks good

SimianArmy-pull-requests #53 FAILURE
Looks like there's a problem with this pull request

SimianArmy-pull-requests #54 FAILURE
Looks like there's a problem with this pull request

tests are failing:
https://netflixoss.ci.cloudbees.com/job/SimianArmy-pull-requests/54/console

SimianArmy-pull-requests #55 SUCCESS
This pull request looks good

SimianArmy-pull-requests #56 SUCCESS
This pull request looks good

SimianArmy-pull-requests #57 SUCCESS
This pull request looks good

Sorry for failures... while developing the actual plugins I was adding functionality into the base classes.

I've got two new plugins now (one active pull request #87 , one that needs a bit more testing), so I think this "refactor for pluggability" PR should be OK now.

Hi Justin,

Thanks for the contributions. The changes look interesting. Sorry it has taken so long for me to look at this. The pull requests are sort of a mess with the cumulative changes mixed in and basically too hard for me to find the meaningful content. I think it would make more sense to merge all the various chaos type changes into one large pull request.

Also I don't think I saw any unit tests provided? Perhaps I just missed them, but getting some tests added would really be appreciated.

Thanks
-Cory

No worries Cory! The Pull requests are more or less cumulative, as most of them build on the previous ones. I definitely agree they don't make any sense if read out of order! I think the order should be the numerical order in Github.

If I look at this PR #86 in this view: https://github.com/Netflix/SimianArmy/pull/86/files

It seems fairly straightforward (though I'm obviously biased!); this PR is setting up the infrastructure for allowing the chaos type to be pluggable, pulling the shutdown instance strategy into its own class.

There are a lot of commits that github is collapsing though; let me know if you'd like me to rebase (or do some other git magic!).

I'll look at adding unit tests. Do you have any suggestions for what would be a good unit tests to add here and on the others PRs? I've mostly been relying on the existing test suite along with manual testing (it's tricky to do a lot of this without real EC2 instances)...

Hey, it looks like pull request #93 is comprehensive of all the requests right? I think I will just review that more with more detail and add comments if I seen anything I think needs changing.

For the unit tests I was going to point you to our coverage reports, but the reports seem broken at the moment:
https://netflixoss.ci.cloudbees.com/job/SimianArmy-master/cobertura

I will try to get the coverage reports working again then that should highlight any chunks that need additional testing.

Thanks,
-Cory

Coverage reports would be great to work from.

#93 should be everything, so if you want to see the final picture then by all means jump in there! I tried to keep them as individual steps in case you wanted to go in small chunks, but (as you can see) I think it made the Git history a lot messier. If you go into my repo I think it's easier to compare the individual branches that back each pull request, if you want to do that.

Whatever review style works for you, works for me :-)