Repository: remind101/stacker (python)

Issue:
stacker.status.Status.__cmp__ implementation is wrong
Came across [this](https://github.com/remind101/stacker/blob/db2119270a6d903f5bba6e830b2818769957bc28/stacker/status.py#L7) while adding python3 support and it feels like there could be some subtle fallout. [Per the docs](https://docs.python.org/2/reference/datamodel.html#object.__cmp__) this is supposed to return zero iff the objects are equal, and since ```False == 0``` any Status is equal to any object without a code attribute. The only reason I stumbled across it is ```__cmp__``` isn't supported in py3 and one of the tests fails (forget which offhand), but the test compares a Status to True which currently always succeeds:

```
Python 2.7.8 (default, Oct 18 2017, 15:41:14)
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import stacker.status
>>> stacker.status.SUBMITTED == stacker.status.PENDING
False
>>> stacker.status.SUBMITTED == True
True
>>> stacker.status.SUBMITTED == False
True
>>> stacker.status.SUBMITTED == 'banana'
True
>>>
```

Everything I needed for py3 support was obvious and mechanical except this.

Pull request description:
Fixes 566
Fixes #566 

One of our tests had a logic error where we were using the returned
result of Step.run (which was always self.ok [which is always True])
and comparing this to a Status object.

The actual test should be comparing Step.status with SKIPPED.

	modified:   stacker/plan.py
	modified:   stacker/status.py
	modified:   stacker/tests/actions/test_destroy.py

Patch:
diff --git a/stacker/status.py b/stacker/status.py
index c399cb7..668cba3 100644
--- a/stacker/status.py
+++ b/stacker/status.py
@@ -1,13 +1,34 @@
+import operator
+
+
 class Status(object):
     def __init__(self, name, code, reason=None):
         self.name = name
         self.code = code
         self.reason = reason or getattr(self, "reason", None)
 
-    def __cmp__(self, other):
+    def _comparison(self, operator, other):
         if hasattr(other, "code"):
-            return cmp(self.code, other.code)
-        return False
+            return operator(self.code, other.code)
+        return NotImplemented
+
+    def __eq__(self, other):
+        return self._comparison(operator.eq, other)
+
+    def __ne__(self, other):
+        return self._comparison(operator.ne, other)
+
+    def __lt__(self, other):
+        return self._comparison(operator.lt, other)
+
+    def __gt__(self, other):
+        return self._comparison(operator.gt, other)
+
+    def __le__(self, other):
+        return self._comparison(operator.le, other)
+
+    def __ge__(self, other):
+        return self._comparison(operator.ge, other)
 
 
 class PendingStatus(Status):


Test patch:
diff --git a/stacker/tests/actions/test_destroy.py b/stacker/tests/actions/test_destroy.py
index 806a6b6..0593584 100644
--- a/stacker/tests/actions/test_destroy.py
+++ b/stacker/tests/actions/test_destroy.py
@@ -102,20 +102,24 @@ class TestDestroyAction(unittest.TestCase):
         # simulate stack doesn't exist and we haven't submitted anything for
         # deletion
         mock_provider.get_stack.side_effect = StackDoesNotExist("mock")
-        status = step.run()
-        self.assertEqual(status, SKIPPED)
+
+        step.run()
+        self.assertEqual(step.status, SKIPPED)
 
         # simulate stack getting successfully deleted
         mock_provider.get_stack.side_effect = get_stack
         mock_provider.is_stack_destroyed.return_value = False
         mock_provider.is_stack_in_progress.return_value = False
-        status = step._run_once()
-        self.assertEqual(status, SUBMITTED)
+
+        step._run_once()
+        self.assertEqual(step.status, SUBMITTED)
         mock_provider.is_stack_destroyed.return_value = False
         mock_provider.is_stack_in_progress.return_value = True
-        status = step._run_once()
-        self.assertEqual(status, SUBMITTED)
+
+        step._run_once()
+        self.assertEqual(step.status, SUBMITTED)
         mock_provider.is_stack_destroyed.return_value = True
         mock_provider.is_stack_in_progress.return_value = False
-        status = step._run_once()
-        self.assertEqual(status, COMPLETE)
+
+        step._run_once()
+        self.assertEqual(step.status, COMPLETE)
diff --git a/stacker/tests/test_plan.py b/stacker/tests/test_plan.py
index bdb5f1f..a49aaa8 100644
--- a/stacker/tests/test_plan.py
+++ b/stacker/tests/test_plan.py
@@ -22,6 +22,7 @@ from stacker.exceptions import (
     PlanFailed,
 )
 from stacker.status import (
+    SUBMITTED,
     COMPLETE,
     SKIPPED,
     FAILED,
@@ -44,13 +45,22 @@ class TestStep(unittest.TestCase):
     def test_status(self):
         self.assertFalse(self.step.submitted)
         self.assertFalse(self.step.completed)
+
         self.step.submit()
+        self.assertEqual(self.step.status, SUBMITTED)
         self.assertTrue(self.step.submitted)
         self.assertFalse(self.step.completed)
+
         self.step.complete()
+        self.assertEqual(self.step.status, COMPLETE)
+        self.assertNotEqual(self.step.status, SUBMITTED)
         self.assertTrue(self.step.submitted)
         self.assertTrue(self.step.completed)
 
+        self.assertNotEqual(self.step.status, True)
+        self.assertNotEqual(self.step.status, False)
+        self.assertNotEqual(self.step.status, 'banana')
+
 
 class TestPlan(unittest.TestCase):