From 386de1134f9b8e7efd0b2e306bc4278b1e13510b Mon Sep 17 00:00:00 2001 From: Alex Kennedy Date: Fri, 17 Jan 2025 19:35:49 -0800 Subject: [PATCH 1/5] fix: Allow boolean-like values for SKIP_DRY --- docs/conf.py | 12 ++------ kingpin/actors/base.py | 48 ++++++++++---------------------- kingpin/actors/test/test_base.py | 18 ------------ kingpin/bin/deploy.py | 9 +++++- kingpin/bin/test/test_deploy.py | 9 ++++++ kingpin/test/test_utils.py | 15 ++++++++++ kingpin/utils.py | 31 +++++++++++++++++---- 7 files changed, 74 insertions(+), 68 deletions(-) diff --git a/docs/conf.py b/docs/conf.py index be2475db..58577b33 100644 --- a/docs/conf.py +++ b/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()] diff --git a/kingpin/actors/base.py b/kingpin/actors/base.py index b635959a..3bca8708 100644 --- a/kingpin/actors/base.py +++ b/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,9 +250,9 @@ 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: + except RuntimeError as e: self.log.warning(e) if not (value is None or isinstance(value, expected_type)): @@ -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 [ diff --git a/kingpin/actors/test/test_base.py b/kingpin/actors/test/test_base.py index d41f7661..f6eb6eba 100644 --- a/kingpin/actors/test/test_base.py +++ b/kingpin/actors/test/test_base.py @@ -25,9 +25,6 @@ from kingpin.constants import REQUIRED, STATE -__author__ = "Matt Wise " - - 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 = { diff --git a/kingpin/bin/deploy.py b/kingpin/bin/deploy.py index 071a1e11..ef042806 100755 --- a/kingpin/bin/deploy.py +++ b/kingpin/bin/deploy.py @@ -174,8 +174,15 @@ def main(): sys.exit(0) + skip_dry = False + try: + skip_dry = utils.str2bool(os.environ.get("SKIP_DRY", "False"), strict=True) + except ValueError: + log.warning("SKIP_DRY is not a valid boolean-like. Defaulting to False.") + pass + # Begin doing real stuff! - if os.environ.get("SKIP_DRY", False): + if skip_dry: log.warning("") log.warning("*** You have disabled the dry run.") log.warning("*** Execution will begin with no expectation of success.") diff --git a/kingpin/bin/test/test_deploy.py b/kingpin/bin/test/test_deploy.py index 731d3779..73c1fa38 100644 --- a/kingpin/bin/test/test_deploy.py +++ b/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() diff --git a/kingpin/test/test_utils.py b/kingpin/test/test_utils.py index c422ec5a..0a617f34 100644 --- a/kingpin/test/test_utils.py +++ b/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): diff --git a/kingpin/utils.py b/kingpin/utils.py index 8e779fb3..13458d7c 100644 --- a/kingpin/utils.py +++ b/kingpin/utils.py @@ -32,8 +32,6 @@ from kingpin import exceptions -__author__ = "Matt Wise (matt@nextdoor.com)" - 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 From da0bc6c40a174930ab2c87e9982f07eb16118d72 Mon Sep 17 00:00:00 2001 From: Alex Kennedy Date: Fri, 17 Jan 2025 19:56:55 -0800 Subject: [PATCH 2/5] Fix SKIP_DRY reporting logic --- kingpin/bin/deploy.py | 49 +++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/kingpin/bin/deploy.py b/kingpin/bin/deploy.py index ef042806..7087b277 100755 --- a/kingpin/bin/deploy.py +++ b/kingpin/bin/deploy.py @@ -174,32 +174,35 @@ def main(): sys.exit(0) - skip_dry = False - try: - skip_dry = utils.str2bool(os.environ.get("SKIP_DRY", "False"), strict=True) - except ValueError: - log.warning("SKIP_DRY is not a valid boolean-like. Defaulting to False.") - pass - - # Begin doing real stuff! - if skip_dry: - 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.environ.get("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!") - log.info("Rehearsal OK! Performing!") + 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!") + + # Begin doing real stuff! try: runner = get_main_actor(dry=args.dry) From 885111f10ce437670e9c2a0872b16ea7a0b0a772 Mon Sep 17 00:00:00 2001 From: Alex Kennedy Date: Fri, 17 Jan 2025 20:03:31 -0800 Subject: [PATCH 3/5] fix exc type --- kingpin/actors/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kingpin/actors/base.py b/kingpin/actors/base.py index 3bca8708..a90122a0 100644 --- a/kingpin/actors/base.py +++ b/kingpin/actors/base.py @@ -252,7 +252,7 @@ def _validate_options(self): try: value = utils.str2bool(value, strict=True) self._options[opt] = value - except RuntimeError as e: + except ValueError as e: self.log.warning(e) if not (value is None or isinstance(value, expected_type)): From 03c4a0a9b6e054b870ec7035847a3524662db131 Mon Sep 17 00:00:00 2001 From: Alex Kennedy Date: Fri, 17 Jan 2025 20:24:00 -0800 Subject: [PATCH 4/5] chore: Use getenv --- kingpin/actors/base.py | 2 +- kingpin/bin/deploy.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/kingpin/actors/base.py b/kingpin/actors/base.py index a90122a0..a3aaf3c4 100644 --- a/kingpin/actors/base.py +++ b/kingpin/actors/base.py @@ -253,7 +253,7 @@ def _validate_options(self): value = utils.str2bool(value, strict=True) self._options[opt] = value except ValueError as e: - self.log.warning(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.' % ( diff --git a/kingpin/bin/deploy.py b/kingpin/bin/deploy.py index 7087b277..6010399e 100755 --- a/kingpin/bin/deploy.py +++ b/kingpin/bin/deploy.py @@ -178,7 +178,7 @@ def main(): # Lets maybe do a dry run first anyways... skip_dry = False try: - skip_dry = utils.str2bool(os.environ.get("SKIP_DRY", "False"), strict=True) + 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 From 593a1f0080e977536fb103f04770997817148e81 Mon Sep 17 00:00:00 2001 From: Alex Kennedy Date: Fri, 17 Jan 2025 20:52:26 -0800 Subject: [PATCH 5/5] chore: bump version number --- kingpin/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kingpin/version.py b/kingpin/version.py index 56ce1017..6f8e0217 100644 --- a/kingpin/version.py +++ b/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"