Code review comment for lp:~rvb/gwacl/fix-destroydeploy

Gavin Panella (allenap) wrote :

Looks good.

[1]

-        if err != nil && !IsNotFoundError(err) {
-            return err
-        }
+        // Even when the operation itself works (i.e. the VM is stopped) we
+        // seem to alway get a 500 error back.
+        // We chose to ignore the error.
+        // If the operation errors for real and the VM is still running then
+        // the next operation (deleting the disks) will fail.
+        // if err != nil && !IsNotFoundError(err) {
+        //    return err
+        // }

I suggest only ignoring 500 errors. A new IsInternalServerError()
helper would be good here.

[2]

+/*
+// This test is currently disabled as we chose to ignore the errors returned
+// when shutting down the VM.  See the comment in DestroyDeployment's code
+// for more information.
 func (suite *suiteDestroyDeployment) TestFailsShuttingDownRole(c *C) {
     var responses []DispatcherResponse
     // Prepare.
@@ -377,6 +381,7 @@
     c.Check(err, ErrorMatches, "POST request failed [(]500: Internal Server Error[)]")
     c.Check(record, HasLen, 2)
 }
+*/

You can use c.ExpectFailure(reason string) here; see
http://godoc.org/launchpad.net/gocheck#C.ExpectFailure.

review: Approve

« Back to merge proposal