Simple bug in saving metric_statistic model #655

merged 1 commit into from Dec 14, 2011


None yet
2 participants

bmiller-hom commented Dec 13, 2011

Bug fix is removing this line ... looks like a WIP:

      put_opts.merge!('Timestamp' => dimensions) if timestamp

It sends a bad request to the API, the 'Timestamp' key is replaced with dimensions. The test for this is marked as 'pending', but it's easy to show it is broken in fog/master and that this one liner fixes it:

Symptom with fog master:

fog#> instanceid = "i-xxxxxxx"
fog#> params = { :timestamp =>, :minimum => 0, :maximum => 60, :sum => 100, :average => 3, :sample_count => 10, :namespace => "hom_custom", :metric_name => "PassengerQueue", :value => 10, :unit => 'None', 'Dimensions' => [{'Name' => 'InstanceId', 'Value' => instanceid}]}
fog#> => "us-west-1")

response => #<Excon::Response:0x000008046d28b8 @Body="<ErrorResponse xmlns="\">\n \n Sender\n MalformedInput\n timestamp must follow ISO8601\n \n d47bdedf-25c8-11e1-bbe6-419fabbfd753\n\n", @headers={"x-amzn-RequestId"=>"d47bdedf-25c8-11e1-bbe6-419fabbfd753", "Content-Type"=>"text/xml", "Content-Length"=>"281", "Date"=>"Tue, 13 Dec 2011 20:27:08 GMT"}, @status=400>


geemus commented Dec 13, 2011

@bmiller-hom looks good. If it fixes the pending test, could you also remove the pending marker? Thanks!


bmiller-hom commented Dec 14, 2011

Thing is, the test doesn't work anyway, it's marked pending because it uses a hard-coded instance id. Replace the hard coded instance id with a real one for your environment and remove pending, and it should pass with the patch. But that's not a permanent solution for other people to be able to run the test... perhaps you have a way to deal with that issue?


geemus commented Dec 14, 2011

@bmiller-hom got it, I guess I misunderstood. I thought from your comment that this bug was why the test was marked pending (not other outlying complications). The more general solution would probably be to spin up and subsequently shut down an instance using whoever is running the tests credentials, but I'm not going to force that on you. I'll take this as is and we can revisit that later. Thanks!

geemus added a commit that referenced this pull request Dec 14, 2011

Merge pull request #655 from hom/master
Simple bug in saving metric_statistic model

@geemus geemus merged commit 9a1f13f into fog:master Dec 14, 2011


geemus commented Dec 14, 2011

@bmiller-hom P.S. could you provide me with contact info somehow, so that I can send you the contributor welcome/thank you message?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment