fix(actors): Allow boolean-like values for SKIP_DRY (#538)

* fix: Allow boolean-like values for SKIP_DRY

* Fix SKIP_DRY reporting logic

* fix exc type

* chore: Use getenv

* chore: bump version number

docs/conf.py

@@ -19,7 +19,6 @@
 # add these directories to sys.path here. If the directory is relative to the
 # documentation root, use os.path.abspath to make it absolute, like shown here.
 sys.path.insert(0, os.path.abspath(".."))
-import kingpin
 from kingpin import version as kingpin_version
 
 # We need sphinx 1.2+ for some of our used features
@@ -69,7 +68,7 @@
 
 # General information about the project.
 project = u'Kingpin'
-copyright = u'2015, Nextdoor'
+copyright = u'2025, Nextdoor'
 author = u'Nextdoor Engineering'
 
 # The version info for the project you're documenting, acts as replacement for
@@ -112,7 +111,7 @@
 
 # The theme to use for HTML and HTML Help pages.  See the documentation for
 # a list of builtin themes.
-html_theme = 'alabaster'
+html_theme = "sphinx_rtd_theme" # default: 'alabaster'
 
 # Theme options are theme-specific and customize the look and feel of a theme
 # further.  For a list of options available for each theme, see the
@@ -271,7 +270,7 @@
 #  dir menu entry, description, category)
 texinfo_documents = [
   (master_doc, 'Kingpin', u'Kingpin Documentation',
-   author, 'Kingpin', 'One line description of project.',
+   author, 'Kingpin', 'Deployment Automation Engine',
    'Miscellaneous'),
 ]
 
@@ -294,8 +293,3 @@
     'boto': ('http://boto.cloudhackers.com/en/latest/', None),
     'boto3': ('http://boto3.readthedocs.io/en/latest/', None)
 }
-
-# Force the RTD theme for all builds
-import sphinx_rtd_theme
-html_theme = 'sphinx_rtd_theme'
-html_theme_path = [sphinx_rtd_theme.get_html_theme_path()]

kingpin/actors/base.py

@@ -116,10 +116,10 @@ def __init__(
         Args:
             desc: (Str) description of the action being executed.
             options: (Dict) Key/Value pairs that have the options
-                     for this action. Values should be primitives.
+                for this action. Values should be primitives.
             dry: (Bool) or not this Actor will actually make changes.
             warn_on_failure: (Bool) Whether this actor ignores its return
-                             value and always succeeds (but warns).
+                value and always succeeds (but warns).
             condition: (Bool) Whether to run this actor.
             init_context: (Dict) Key/Value pairs used at instantiation
                 time to replace {KEY} strings in the actor definition.
@@ -250,10 +250,10 @@ def _validate_options(self):
             # failure get caught below.
             if expected_type is bool:
                 try:
-                    value = self.str2bool(value, strict=True)
+                    value = utils.str2bool(value, strict=True)
                     self._options[opt] = value
-                except exceptions.InvalidOptions as e:
-                    self.log.warning(e)
+                except ValueError as e:
+                    self.log.warning(exceptions.InvalidOptions(e.args))
 
             if not (value is None or isinstance(value, expected_type)):
                 message = 'Option "%s" has to be %s and is %s.' % (
@@ -350,29 +350,6 @@ def timeout(self, f, *args, **kwargs):
 
         raise gen.Return(ret)
 
-    def str2bool(self, v, strict=False):
-        """Returns a Boolean from a variety of inputs.
-
-        Args:
-            value: String/Bool
-            strict: Whether or not to _only_ convert the known words into booleans, or whether to allow "any" word to be considered True other than the known False words.
-
-        Returns:
-            A boolean
-        """
-        false = ("no", "false", "f", "0")
-        true = ("yes", "true", "t", "1")
-
-        string = str(v).lower()
-
-        if strict:
-            if string not in true and string not in false:
-                raise exceptions.InvalidOptions(
-                    "Expected [%s, %s] but got: %s" % (true, false, string)
-                )
-
-        return string not in false
-
     def _check_condition(self):
         """Check if specified condition allows this actor to run.
 
@@ -381,8 +358,11 @@ def _check_condition(self):
         the value of self._condition is a string "False" or string "0".
         """
 
-        check = self.str2bool(self._condition)
-        self.log.debug("Condition %s evaluates to %s" % (self._condition, check))
+        try:
+            check = utils.str2bool(self._condition)
+            self.log.debug("Condition %s evaluates to %s" % (self._condition, check))
+        except ValueError as e:
+            raise exceptions.InvalidOptions(e.args) from e
         return check
 
     def _fill_in_contexts(self, context={}, strict=True, remove_escape_sequence=True):
@@ -395,7 +375,7 @@ def _fill_in_contexts(self, context={}, strict=True, remove_escape_sequence=True
 
         Args:
             strict: bool whether or not to allow missing context keys to be
-                    skipped over.
+                skipped over.
 
         Raises:
             exceptions.InvalidOptions
@@ -460,10 +440,10 @@ def get_orgchart(self, parent=""):
         actors that are called.
 
         orgchart object:
-          id: unique string identifying this actor's instance.
-          class: kingpin class name
-          desc: actor description
-          parent_id: organizational relationship. Same as `id` above.
+            id: unique string identifying this actor's instance.
+            class: kingpin class name
+            desc: actor description
+            parent_id: organizational relationship. Same as `id` above.
         """
 
         return [

kingpin/actors/test/test_base.py

@@ -25,9 +25,6 @@
 from kingpin.constants import REQUIRED, STATE
 
 
-__author__ = "Matt Wise <[email protected]>"
-
-
 class FakeHTTPClientClass(object):
     """Fake HTTPClient object for testing"""
 
@@ -280,21 +277,6 @@ def test_execute(self):
         res = yield self.actor.execute()
         self.assertEqual(res, True)
 
-    def test_str2bool(self):
-        self.assertEqual(True, self.actor.str2bool("true"))
-        self.assertEqual(True, self.actor.str2bool("junk text"))
-        self.assertEqual(True, self.actor.str2bool("1"))
-        self.assertEqual(True, self.actor.str2bool(True))
-        self.assertEqual(False, self.actor.str2bool("false"))
-        self.assertEqual(False, self.actor.str2bool("0"))
-        self.assertEqual(False, self.actor.str2bool(False))
-
-    def test_str2bool_strict(self):
-        self.assertEqual(True, self.actor.str2bool("true"))
-        self.assertEqual(False, self.actor.str2bool(False))
-        with self.assertRaises(exceptions.InvalidOptions):
-            self.actor.str2bool("Junk", strict=True)
-
     @testing.gen_test
     def test_check_condition(self):
         conditions = {

kingpin/bin/deploy.py

@@ -174,25 +174,35 @@ def main():
 
         sys.exit(0)
 
-    # Begin doing real stuff!
-    if os.environ.get("SKIP_DRY", False):
-        log.warning("")
-        log.warning("*** You have disabled the dry run.")
-        log.warning("*** Execution will begin with no expectation of success.")
-        log.warning("")
-    elif not args.dry:
-        log.info("Rehearsing... Break a leg!")
-
+    if not args.dry:
+        # Lets maybe do a dry run first anyways...
+        skip_dry = False
         try:
-            dry_actor = get_main_actor(dry=True)
-            yield dry_actor.execute()
-        except actor_exceptions.ActorException as e:
-            log.critical("Dry run failed. Reason:")
-            log.critical(e)
-            sys.exit(2)
+            skip_dry = utils.str2bool(os.getenv("SKIP_DRY", "False"), strict=True)
+        except ValueError:
+            log.warning("SKIP_DRY is not a valid boolean-like. Defaulting to False.")
+            pass
+
+        if skip_dry:
+            # Okay, the user really does not want to do a dry run... fine
+            log.warning("")
+            log.warning("*** You have disabled the pre-check dry run.")
+            log.warning("*** Execution will begin with no expectation of success.")
+            log.warning("")
+        else:
+            log.info("Rehearsing... Break a leg!")
+
+            try:
+                dry_actor = get_main_actor(dry=True)
+                yield dry_actor.execute()
+            except actor_exceptions.ActorException as e:
+                log.critical("Dry run failed. Reason:")
+                log.critical(e)
+                sys.exit(2)
 
-        log.info("Rehearsal OK! Performing!")
+            log.info("Rehearsal OK! Performing!")
 
+    # Begin doing real stuff!
     try:
         runner = get_main_actor(dry=args.dry)
 

kingpin/bin/test/test_deploy.py

@@ -102,6 +102,15 @@ def test_main_with_skip_dry(self):
             self.kingpin_bin_deploy.main()
             mock_get_main_actor.assert_called()
 
+    @mock.patch("sys.argv", ["kingpin"])
+    @mock.patch.dict(os.environ, {"SKIP_DRY": "not-a-boolean-like-string"})
+    def test_main_with_skip_dry_invalid(self):
+        self._import_kingpin_bin_deploy()
+        with mock.patch("kingpin.bin.deploy.get_main_actor") as mock_get_main_actor:
+            mock_get_main_actor.return_value = Sleep(options={"sleep": 0.1}, dry=True)
+            self.kingpin_bin_deploy.main()
+            mock_get_main_actor.assert_called()
+
     @mock.patch("sys.argv", ["kingpin"])
     def test_main_with_bad_runner(self):
         self._import_kingpin_bin_deploy()

kingpin/test/test_utils.py

@@ -189,6 +189,21 @@ def raises_exc():
             self.assertEqual(1, logger().debug.call_count)
             logger().debug.assert_called_with(mock.ANY, exc_info=1)
 
+    def test_str2bool(self):
+        self.assertEqual(True, utils.str2bool("true"))
+        self.assertEqual(True, utils.str2bool("junk text"))
+        self.assertEqual(True, utils.str2bool("1"))
+        self.assertEqual(True, utils.str2bool(True))
+        self.assertEqual(False, utils.str2bool("false"))
+        self.assertEqual(False, utils.str2bool("0"))
+        self.assertEqual(False, utils.str2bool(False))
+
+    def test_str2bool_strict(self):
+        self.assertEqual(True, utils.str2bool("true"))
+        self.assertEqual(False, utils.str2bool(False))
+        with self.assertRaises(ValueError):
+            utils.str2bool("Junk", strict=True)
+
 
 class TestSetupRootLoggerUtils(unittest.TestCase):
     def setUp(self):

kingpin/utils.py

@@ -32,8 +32,6 @@
 from kingpin import exceptions
 
 
-__author__ = "Matt Wise ([email protected])"
-
 log = logging.getLogger(__name__)
 
 # Constants for some of the utilities below
@@ -277,10 +275,9 @@ def populate_with_tokens(
         left_wrapper: the character to use as the START of a token
         right_wrapper: the character to use as the END of a token
         strict: (bool) whether or not to make sure all tokens were replaced
-        escape_sequence: character string to use as the escape sequence for
-        left and right wrappers
-        remove_escape_sequence: (bool) whether or not to remove the escape
-        sequence if it found. For example \\%FOO\\% would turn into %FOO%.
+        escape_sequence: character string to use as the escape sequence for left and right wrappers
+        remove_escape_sequence: (bool) whether or not to remove the escape sequence if it found. For example \\%FOO\\% would turn into %FOO%.
+
     Example:
         export ME=biz
 
@@ -499,3 +496,25 @@ def diff_dicts(dict1, dict2):
     dict2 = [line.replace("u'", "'") for line in dict2]
 
     return "\n".join(difflib.unified_diff(dict1, dict2, n=2))
+
+
+def str2bool(v, strict=False) -> bool:
+    """Returns a Boolean from a variety of inputs.
+
+    Args:
+        value: String/Bool
+        strict: Whether or not to _only_ convert the known words into booleans, or whether to allow "any" word to be considered True other than the known False words.
+
+    Returns:
+        A boolean
+    """
+    false = ("no", "false", "f", "0")
+    true = ("yes", "true", "t", "1")
+
+    string = str(v).lower()
+
+    if strict:
+        if string not in true and string not in false:
+            raise ValueError(f"Expected [{true}, {false}] but got: {string}")
+
+    return string not in false

kingpin/version.py

@@ -1,4 +1,4 @@
 """Kingpin version number. You must bump this when creating a new release.
 """
 
-__version__ = "3.2.0"
+__version__ = "3.3.0"