On Thursday, February 16, 2012 at 2:02 PM, [email protected] wrote: > ACK, couple thoughts: > > On 10/02/12 14:56, [email protected] (mailto:[email protected]) wrote: > > From: Michal Fojtik <[email protected] (mailto:[email protected])> > > > > > > Signed-off-by: Michal fojtik <[email protected] > > (mailto:[email protected])> > > --- > > server/lib/deltacloud/base_driver/exceptions.rb | 16 +++++++ > > server/lib/deltacloud/drivers/mock/mock_driver.rb | 32 +++++++++++++- > > .../lib/deltacloud/helpers/application_helper.rb | 2 +- > > server/lib/sinatra/lazy_auth.rb | 2 +- > > server/views/errors/501.html.haml | 43 ++++++++++++++++++++ > > server/views/errors/501.xml.haml | 12 +++++ > > server/views/errors/502.xml.haml | 13 ++++-- > > server/views/errors/504.html.haml | 43 ++++++++++++++++++++ > > server/views/errors/504.xml.haml | 12 +++++ > > > > we really need all of these? like 501.html.haml 504.html.haml are > identical, as are 501.xml.haml 502.xml.haml and 504.xml.haml. If we > really want seperate files for 'neatness' perhaps just include them in > each other? just thinking in terms of maintaining if we want to change > something its gonna be messy.
I agree. We should probably create one generic error template and then just render them in this files (or change the error reporting code completely for this). Will do it in next revision. > One more thought below: > > > diff --git a/server/lib/deltacloud/base_driver/exceptions.rb > > b/server/lib/deltacloud/base_driver/exceptions.rb > > index e44c89b..9effe29 100644 > > --- a/server/lib/deltacloud/base_driver/exceptions.rb > > > > def realms(credentials, opts=nil) > > - return REALMS if ( opts.nil? ) > > - results = REALMS > > + check_credentials( credentials ) > > + results = [] > > + safely do > > + # This hack is used to test if client capture exceptions correctly > > + # To raise an exception do GET /api/realms/50[0-2] > > + raise "DeltacloudErrorTest" if opts and opts[:id] == "500" > > + raise "NotImplementedTest" if opts and opts[:id] == "501" > > + raise "ProviderErrorTest" if opts and opts[:id] == "502" > > + raise "ProviderTimeoutTest" if opts and opts[:id] == "504" > > + return REALMS if ( opts.nil? ) > > + results = REALMS > > + end > > results = filter_on( results, :id, opts ) > > results > > > > you want to keep this stuff here? I saw that you are using them in > client/specs/error_spec but feels a bit too 'hacky' - maybe define some > special method just for this? Well, yes they are hacky :-) But on the other side this is the most easy way how to test/trigger exception handling in server/client. This could be also extra useful for conductor guys to test their exception handling code. I was thinking about special method but that would need to be integrated to Rabbit which can be a mess. I think we should keep this code there for now (to make our client tests happy) and properly document this as a feature (exception testing.) -- michal
