From 536ba1843a9faf2ff139f06b917f67d3ee8edd3f Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Mon, 27 Nov 2017 13:45:05 -0800 Subject: [PATCH] Add a command line option to run.py for bot config files. Before this change, we were looking for config files in default locations in source control, which is not a good place to look for them. Now `run.py` and friends have a command line argument where users can specify the config files. Note that the change to server.py is only a partial fix to make it so that bots that don't use third party config files won't crash. That program needs an overhaul, anyway. --- zulip_bots/zulip_bots/lib.py | 58 ++++++++++++++++++----- zulip_bots/zulip_bots/run.py | 55 +++++++++++++++++---- zulip_bots/zulip_bots/test_lib.py | 2 +- zulip_bots/zulip_bots/test_run.py | 6 ++- zulip_botserver/zulip_botserver/server.py | 8 +++- 5 files changed, 104 insertions(+), 25 deletions(-) diff --git a/zulip_bots/zulip_bots/lib.py b/zulip_bots/zulip_bots/lib.py index 1a5da6e..27afe5e 100644 --- a/zulip_bots/zulip_bots/lib.py +++ b/zulip_bots/zulip_bots/lib.py @@ -19,6 +19,9 @@ from types import ModuleType from zulip import Client, ZulipError +class NoBotConfigException(Exception): + pass + def exit_gracefully(signum, frame): # type: (int, Optional[Any]) -> None sys.exit(0) @@ -83,8 +86,8 @@ class StateHandler(object): return key in self.state_ class ExternalBotHandler(object): - def __init__(self, client, root_dir, bot_details): - # type: (Client, str, Dict[str, Any]) -> None + def __init__(self, client, root_dir, bot_details, bot_config_file): + # type: (Client, str, Dict[str, Any], str) -> None # Only expose a subset of our Client's functionality try: user_profile = client.get_profile() @@ -108,6 +111,7 @@ class ExternalBotHandler(object): self._client = client self._root_dir = root_dir self.bot_details = bot_details + self.bot_config_file = bot_config_file self._storage = StateHandler(client) try: self.user_id = user_profile['user_id'] @@ -155,15 +159,45 @@ class ExternalBotHandler(object): def get_config_info(self, bot_name, optional=False): # type: (str, Optional[bool]) -> Dict[str, Any] - conf_file_path = os.path.realpath(os.path.join(self._root_dir, bot_name + '.conf')) - config = configparser.ConfigParser() - try: - with open(conf_file_path) as conf: - config.readfp(conf) # type: ignore # readfp->read_file in python 3, so not in stubs - except IOError: + + if self.bot_config_file is None: if optional: return dict() - raise + + # Well written bots should catch this exception + # and provide nice error messages with instructions + # on setting up the configuration specfic to this bot. + # And then `run.py` should also catch exceptions on how + # to specify the file in the command line. + raise NoBotConfigException(bot_name) + + if bot_name not in self.bot_config_file: + print(''' + WARNING! + + {} does not adhere to the + file naming convention, and it could be a + sign that you passed in the + wrong third-party configuration file. + + The suggested name is {}.conf + + We will proceed anyway. + '''.format(self.bot_config_file, bot_name)) + + # We expect the caller to pass in None if the user does + # not specify a bot_config_file. If they pass in a bogus + # filename, we'll let an IOError happen here. Callers + # like `run.py` will do the command line parsing and checking + # for the existence of the file. + config = configparser.ConfigParser() + with open(self.bot_config_file) as conf: + try: + config.readfp(conf) # type: ignore # readfp->read_file in python 3, so not in stubs + except configparser.Error as e: + display_config_file_errors(str(e), self.bot_config_file) + sys.exit(1) + return dict(config.items(bot_name)) def open(self, filepath): @@ -206,8 +240,8 @@ def display_config_file_errors(error_msg, config_file): print('\nERROR: {} seems to be broken:\n\n{}'.format(config_file, file_contents)) print('\nMore details here:\n\n{}\n'.format(error_msg)) -def run_message_handler_for_bot(lib_module, quiet, config_file, bot_name): - # type: (Any, bool, str, str) -> Any +def run_message_handler_for_bot(lib_module, quiet, config_file, bot_config_file, bot_name): + # type: (Any, bool, str, str, str) -> Any # # lib_module is of type Any, since it can contain any bot's # handler class. Eventually, we want bot's handler classes to @@ -231,7 +265,7 @@ def run_message_handler_for_bot(lib_module, quiet, config_file, bot_name): sys.exit(1) bot_dir = os.path.dirname(lib_module.__file__) - restricted_client = ExternalBotHandler(client, bot_dir, bot_details) + restricted_client = ExternalBotHandler(client, bot_dir, bot_details, bot_config_file) message_handler = lib_module.handler_class() if hasattr(message_handler, 'initialize'): diff --git a/zulip_bots/zulip_bots/run.py b/zulip_bots/zulip_bots/run.py index 3ae0248..21bc771 100755 --- a/zulip_bots/zulip_bots/run.py +++ b/zulip_bots/zulip_bots/run.py @@ -11,7 +11,11 @@ from importlib import import_module from os.path import basename, splitext from typing import Any, Optional, Text -from zulip_bots.lib import run_message_handler_for_bot +from zulip_bots.lib import ( + run_message_handler_for_bot, + NoBotConfigException, +) + from zulip_bots.provision import provision_bot current_dir = os.path.dirname(os.path.abspath(__file__)) @@ -63,6 +67,10 @@ def parse_args(): required=True, help='zulip configuration file (e.g. ~/Downloads/zuliprc)') + parser.add_argument('--bot-config-file', '-b', + action='store', + help='third party configuration file (e.g. ~/giphy.conf') + parser.add_argument('--force', action='store_true', help='try running the bot even if dependencies install fails') @@ -75,7 +83,7 @@ def parse_args(): return args -def exit_gracefully_if_config_file_does_not_exist(config_file): +def exit_gracefully_if_zulip_config_file_does_not_exist(config_file): # type: (str) -> None if not os.path.exists(config_file): print(''' @@ -87,6 +95,21 @@ def exit_gracefully_if_config_file_does_not_exist(config_file): ''' % (config_file,)) sys.exit(1) +def exit_gracefully_if_bot_config_file_does_not_exist(bot_config_file): + # type: (str) -> None + if bot_config_file is None: + # This is a common case, just so succeed quietly. (Some + # bots don't have third party configuration.) + return + + if not os.path.exists(bot_config_file): + print(''' + ERROR: %s does not exist. + + You probably just specified the wrong file location here. + ''' % (bot_config_file,)) + sys.exit(1) + def main(): # type: () -> None args = parse_args() @@ -103,14 +126,28 @@ def main(): if not args.quiet: logging.basicConfig(stream=sys.stdout, level=logging.INFO) - exit_gracefully_if_config_file_does_not_exist(args.config_file) + # It's a bit unfortunate that we have two config files, but the + # alternative would be way worse for people running multiple bots + # or testing against multiple Zulip servers. + exit_gracefully_if_zulip_config_file_does_not_exist(args.config_file) + exit_gracefully_if_bot_config_file_does_not_exist(args.bot_config_file) - run_message_handler_for_bot( - lib_module=lib_module, - config_file=args.config_file, - quiet=args.quiet, - bot_name=bot_name - ) + try: + run_message_handler_for_bot( + lib_module=lib_module, + config_file=args.config_file, + bot_config_file=args.bot_config_file, + quiet=args.quiet, + bot_name=bot_name + ) + except NoBotConfigException: + print(''' + ERROR: Your bot requires you to specify a third party + config file with the --bot-config-file option. + + Exiting now. + ''') + sys.exit(1) if __name__ == '__main__': main() diff --git a/zulip_bots/zulip_bots/test_lib.py b/zulip_bots/zulip_bots/test_lib.py index fc29ef6..9ce75c0 100755 --- a/zulip_bots/zulip_bots/test_lib.py +++ b/zulip_bots/zulip_bots/test_lib.py @@ -44,7 +44,7 @@ class BotTestCaseBase(TestCase): # Mocking ExternalBotHandler self.patcher = patch('zulip_bots.lib.ExternalBotHandler', autospec=True) self.MockClass = self.patcher.start() - self.mock_bot_handler = self.MockClass(None, None, None) + self.mock_bot_handler = self.MockClass(None, None, None, None) self.mock_client = MagicMock() self.mock_client.get_storage.return_value = {'result': 'success', 'state': {}} self.mock_client.update_storage.return_value = {'result': 'success'} diff --git a/zulip_bots/zulip_bots/test_run.py b/zulip_bots/zulip_bots/test_run.py index b27a1ef..7c90201 100644 --- a/zulip_bots/zulip_bots/test_run.py +++ b/zulip_bots/zulip_bots/test_run.py @@ -29,11 +29,12 @@ class TestDefaultArguments(TestCase): @patch('zulip_bots.run.run_message_handler_for_bot') def test_argument_parsing_with_bot_name(self, mock_run_message_handler_for_bot): # type: (mock.Mock) -> None - with patch('zulip_bots.run.exit_gracefully_if_config_file_does_not_exist'): + with patch('zulip_bots.run.exit_gracefully_if_zulip_config_file_does_not_exist'): zulip_bots.run.main() mock_run_message_handler_for_bot.assert_called_with(bot_name='giphy', config_file='/foo/bar/baz.conf', + bot_config_file=None, lib_module=mock.ANY, quiet=False) @@ -41,12 +42,13 @@ class TestDefaultArguments(TestCase): @patch('zulip_bots.run.run_message_handler_for_bot') def test_argument_parsing_with_bot_path(self, mock_run_message_handler_for_bot): # type: (mock.Mock) -> None - with patch('zulip_bots.run.exit_gracefully_if_config_file_does_not_exist'): + with patch('zulip_bots.run.exit_gracefully_if_zulip_config_file_does_not_exist'): zulip_bots.run.main() mock_run_message_handler_for_bot.assert_called_with( bot_name='giphy', config_file='/foo/bar/baz.conf', + bot_config_file=None, lib_module=mock.ANY, quiet=False) diff --git a/zulip_botserver/zulip_botserver/server.py b/zulip_botserver/zulip_botserver/server.py index f83e696..3889fce 100644 --- a/zulip_botserver/zulip_botserver/server.py +++ b/zulip_botserver/zulip_botserver/server.py @@ -54,7 +54,13 @@ def load_bot_handlers(): try: bot_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'bots', bot) - bot_handlers[bot] = ExternalBotHandler(client, bot_dir, bot_details={}) + # TODO: Figure out how to pass in third party config info. + bot_handlers[bot] = ExternalBotHandler( + client, + bot_dir, + bot_details={}, + bot_config_file=None + ) except SystemExit: return BadRequest("Cannot fetch user profile for bot {}, make sure you have set up the flaskbotrc " "file correctly.".format(bot))