From 032e107ecb0ac180c701bde2972538d1975881d5 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 15:24:16 +0100 Subject: [PATCH 01/24] Introduce BaseActiontestCase.get_action_instance method for consistency with sensor tests. --- st2tests/st2tests/base.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index 96a17fba5b..f50dbb4a75 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -490,6 +490,18 @@ class BaseActionTestCase(TestCase): action_cls = None + def get_action_instance(self, config=None): + """ + Retrieve instance of the action class. + """ + kwargs = {} + + if config: + kwargs['config'] = config + + instance = self.action_cls(**kwargs) # pylint: disable=not-callable + return instance + class FakeResponse(object): From 9e16b3e7704bd24b5d19db5641b484291a28684f Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 15:28:23 +0100 Subject: [PATCH 02/24] Update existing tests to utilize get_action_instance method. --- .../tests/test_action_check_auto_deploy_repo.py | 12 +++++++----- contrib/packs/tests/test_action_expand_repo_name.py | 10 ++++++---- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/contrib/packs/tests/test_action_check_auto_deploy_repo.py b/contrib/packs/tests/test_action_check_auto_deploy_repo.py index 79a81e15c1..9b1b8752b8 100644 --- a/contrib/packs/tests/test_action_check_auto_deploy_repo.py +++ b/contrib/packs/tests/test_action_check_auto_deploy_repo.py @@ -32,30 +32,32 @@ """ class CheckAutoDeployRepoActionTestCase(BaseActionTestCase): + action_cls = CheckAutoDeployRepo + def test_run_config_blank(self): config = yaml.safe_load(MOCK_CONFIG_BLANK) - action = CheckAutoDeployRepo(config) + action = self.get_action_instance(config=config) self.assertRaises(Exception, action.run, branch="refs/heads/master", repo_name="st2contrib") def test_run_repositories_blank(self): config = yaml.safe_load(MOCK_CONFIG_BLANK_REPOSITORIES) - action = CheckAutoDeployRepo(config) + action = self.get_action_instance(config=config) self.assertRaises(Exception, action.run, branch="refs/heads/master", repo_name="st2contrib") def test_run_st2contrib_no_auto_deloy(self): config = yaml.safe_load(MOCK_CONFIG_FULL) - action = CheckAutoDeployRepo(config) + action = self.get_action_instance(config=config) self.assertRaises(Exception, action.run, branch="refs/heads/dev", repo_name="st2contrib") def test_run_st2contrib_auto_deloy(self): config = yaml.safe_load(MOCK_CONFIG_FULL) - action = CheckAutoDeployRepo(config) + action = self.get_action_instance(config=config) expected = {'deployment_branch': 'master', 'notify_channel': 'community'} @@ -72,7 +74,7 @@ def test_run_st2incubator_no_auto_deloy(self): def test_run_st2incubator_auto_deloy(self): config = yaml.safe_load(MOCK_CONFIG_FULL) - action = CheckAutoDeployRepo(config) + action = self.get_action_instance(config=config) expected = {'deployment_branch': 'master', 'notify_channel': 'community'} diff --git a/contrib/packs/tests/test_action_expand_repo_name.py b/contrib/packs/tests/test_action_expand_repo_name.py index 8e2a670499..c2d4088a79 100644 --- a/contrib/packs/tests/test_action_expand_repo_name.py +++ b/contrib/packs/tests/test_action_expand_repo_name.py @@ -30,23 +30,25 @@ """ class ExpandRepoNameTestCase(BaseActionTestCase): + action_cls = ExpandRepoName + def test_run_config_blank(self): config = yaml.safe_load(MOCK_CONFIG_BLANK) - action = ExpandRepoName(config) + action = self.get_action_instance(config=config) self.assertRaises(Exception, action.run, repo_name="st2contrib") def test_run_repositories_blank(self): config = yaml.safe_load(MOCK_CONFIG_BLANK_REPOSITORIES) - action = ExpandRepoName(config) + action = self.get_action_instance(config=config) self.assertRaises(Exception, action.run, repo_name="st2contrib") def test_run_st2contrib_expands(self): config = yaml.safe_load(MOCK_CONFIG_FULL) - action = ExpandRepoName(config) + action = self.get_action_instance(config=config) expected = {'repo_url': 'https://github.com/StackStorm/st2contrib.git', 'subtree': True} @@ -55,7 +57,7 @@ def test_run_st2contrib_expands(self): def test_run_st2incubator_expands(self): config = yaml.safe_load(MOCK_CONFIG_FULL) - action = ExpandRepoName(config) + action = self.get_action_instance(config=config) expected = {'repo_url': 'https://github.com/StackStorm/st2incubator.git', 'subtree': True} From c76b4fb78d1b875a47f65995e6d7e6901bb61ba9 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 15:29:20 +0100 Subject: [PATCH 03/24] Make sure we also lint Python files under contrib/packs/actions/. --- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index f383400476..0b39a25dfb 100644 --- a/Makefile +++ b/Makefile @@ -68,7 +68,8 @@ pylint: requirements .pylint . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models $$component/$$component || exit 1; \ done # Lint Python pack management actions - . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models contrib/packs/actions/pack_mgmt/ || exit 1; + . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models contrib/packs/actions/ || exit 1; + # Lint other packs . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models contrib/linux || exit 1; # Lint Python scripts @@ -84,7 +85,7 @@ flake8: requirements .flake8 @echo "==================== flake ====================" @echo . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 $(COMPONENTS) - . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 contrib/packs/actions/pack_mgmt/ + . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 contrib/packs/actions/ . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 contrib/linux . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 scripts/ . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 tools/ From ec18a7ee97df212d3664c336b549e1baef1abebc Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 15:33:48 +0100 Subject: [PATCH 04/24] Fix lint issues. --- .../packs/actions/check_auto_deploy_repo.py | 18 ++++++------------ contrib/packs/actions/expand_repo_name.py | 10 +--------- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/contrib/packs/actions/check_auto_deploy_repo.py b/contrib/packs/actions/check_auto_deploy_repo.py index 8dfb4a7103..45d2d6cd99 100755 --- a/contrib/packs/actions/check_auto_deploy_repo.py +++ b/contrib/packs/actions/check_auto_deploy_repo.py @@ -15,17 +15,9 @@ # See the License for the specific language governing permissions and # limitations under the License. -import sys -import requests -import json -import getopt -import argparse -import os -import yaml - -from getpass import getpass from st2actions.runners.pythonrunner import Action + class CheckAutoDeployRepo(Action): def run(self, branch, repo_name): """Returns the required data to complete an auto deployment of a pack in repo_name. @@ -42,12 +34,14 @@ def run(self, branch, repo_name): results = {} try: - results['deployment_branch'] = self.config["repositories"][repo_name]["auto_deployment"]["branch"] - results['notify_channel'] = self.config["repositories"][repo_name]["auto_deployment"]["notify_channel"] + repo_config = self.config["repositories"][repo_name] + results['deployment_branch'] = repo_config["auto_deployment"]["branch"] + results['notify_channel'] = repo_config["auto_deployment"]["notify_channel"] except KeyError: raise ValueError("No repositories or auto_deployment config for '%s'" % repo_name) else: if branch == "refs/heads/%s" % results['deployment_branch']: return results else: - raise ValueError("Branch %s for %s should not be auto deployed" % (branch, repo_name)) + raise ValueError("Branch %s for %s should not be auto deployed" % + (branch, repo_name)) diff --git a/contrib/packs/actions/expand_repo_name.py b/contrib/packs/actions/expand_repo_name.py index 845b80cfa3..5b9ca4b7b4 100755 --- a/contrib/packs/actions/expand_repo_name.py +++ b/contrib/packs/actions/expand_repo_name.py @@ -15,17 +15,9 @@ # See the License for the specific language governing permissions and # limitations under the License. -import sys -import requests -import json -import getopt -import argparse -import os -import yaml - -from getpass import getpass from st2actions.runners.pythonrunner import Action + class ExpandRepoName(Action): def run(self, repo_name): """Returns the data required to install packs from repo_name. From 631ec96eb21b601fe6739045dda1c8a14a8c3038 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 16:59:22 +0100 Subject: [PATCH 05/24] Update comment. --- st2actions/st2actions/runners/pythonrunner.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/st2actions/st2actions/runners/pythonrunner.py b/st2actions/st2actions/runners/pythonrunner.py index 4e8ab40e0a..0f7b7e3e48 100644 --- a/st2actions/st2actions/runners/pythonrunner.py +++ b/st2actions/st2actions/runners/pythonrunner.py @@ -69,7 +69,9 @@ def __init__(self, config=None): :type config: ``dict`` """ self.config = config or {} - # logger and datastore are assigned in PythonActionWrapper._get_action_instance + + # logger and datastore are late assigned post class instatiation in + # PythonActionWrapper._get_action_instance self.logger = None self.datastore = None From c7e16306b23dfd0f16fdabdedb0e79752a8f7303 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 17:16:00 +0100 Subject: [PATCH 06/24] Update Action code so it utilizes "action_service" argument and exposes datastore management methods via the action service. This way the whole interface is consistent with the existing sensor interface (sensor_service). --- .../runners/python_action_wrapper.py | 61 ++++++++++++------- st2actions/st2actions/runners/pythonrunner.py | 19 ++++-- st2actions/st2actions/runners/utils.py | 40 ++++++++++++ 3 files changed, 93 insertions(+), 27 deletions(-) create mode 100644 st2actions/st2actions/runners/utils.py diff --git a/st2actions/st2actions/runners/python_action_wrapper.py b/st2actions/st2actions/runners/python_action_wrapper.py index ce53949dc9..611afcde15 100644 --- a/st2actions/st2actions/runners/python_action_wrapper.py +++ b/st2actions/st2actions/runners/python_action_wrapper.py @@ -21,6 +21,7 @@ from st2common import log as logging from st2actions import config from st2actions.runners.pythonrunner import Action +from st2actions.runners.utils import get_logger_for_python_runner_action from st2common.util import loader as action_loader from st2common.util.config_parser import ContentPackConfigParser from st2common.constants.action import ACTION_OUTPUT_RESULT_DELIMITER @@ -34,6 +35,38 @@ LOG = logging.getLogger(__name__) +class ActionService(object): + """ + Instance of this class is passed to the action instance and exposes "public" + methods which can be called by the action. + """ + + def __init__(self, action_wrapper): + logger = get_logger_for_action(action_name=self._action_wrapper._class_name) + + self._action_wrapper = action_wrapper + self._datastore_service = DatastoreService(logger=logger, + pack_name=self._action_wrapper._pack, + class_name=self._action_wrapper._class_name, + api_username='action_service') + + ################################## + # Methods for datastore management + ################################## + + def list_values(self, local=True, prefix=None): + return self._datastore_service.list_values(local, prefix) + + def get_value(self, name, local=True): + return self._datastore_service.get_value(name, local) + + def set_value(self, name, value, ttl=None, local=True): + return self._datastore_service.set_value(name, value, ttl, local) + + def delete_value(self, name, local=True): + return self._datastore_service.delete_value(name, local) + + class PythonActionWrapper(object): def __init__(self, pack, file_path, parameters=None, parent_args=None): """ @@ -98,33 +131,15 @@ def _get_action_instance(self): LOG.info('No config found for action "%s"' % (self._file_path)) action_instance = action_cls(config={}) - # Setup action_instance proeprties + # Perform post instantiation action intiailization + # Note: This is needed since for backward compatibility reasons we can't simply update + # Action constructor to take in additional argument (action_service). + action_service = ActionService(action_wrapper=self) + action_instance.setup(action_service=action_service) action_instance.logger = self._set_up_logger(action_cls.__name__) - action_instance.datastore = DatastoreService(logger=action_instance.logger, - pack_name=self._pack, - class_name=action_cls.__name__, - api_username="action_service") return action_instance - def _set_up_logger(self, action_name): - """ - Set up a logger which logs all the messages with level DEBUG - and above to stderr. - """ - logger_name = 'actions.python.%s' % (action_name) - logger = logging.getLogger(logger_name) - - console = stdlib_logging.StreamHandler() - console.setLevel(stdlib_logging.DEBUG) - - formatter = stdlib_logging.Formatter('%(name)-12s: %(levelname)-8s %(message)s') - console.setFormatter(formatter) - logger.addHandler(console) - logger.setLevel(stdlib_logging.DEBUG) - - return logger - if __name__ == '__main__': parser = argparse.ArgumentParser(description='Python action runner process wrapper') diff --git a/st2actions/st2actions/runners/pythonrunner.py b/st2actions/st2actions/runners/pythonrunner.py index 0f7b7e3e48..df6d70c430 100644 --- a/st2actions/st2actions/runners/pythonrunner.py +++ b/st2actions/st2actions/runners/pythonrunner.py @@ -23,6 +23,7 @@ from eventlet.green import subprocess from st2actions.runners import ActionRunner +from st2actions.runners.utils import get_logger_for_python_runner_action from st2common.util.green.shell import run_command from st2common.constants.action import ACTION_OUTPUT_RESULT_DELIMITER from st2common.constants.action import LIVEACTION_STATUS_SUCCEEDED @@ -69,15 +70,25 @@ def __init__(self, config=None): :type config: ``dict`` """ self.config = config or {} + self.logger = get_logger_for_action(action_name=self.__class__.__name__) - # logger and datastore are late assigned post class instatiation in - # PythonActionWrapper._get_action_instance - self.logger = None + # action_service is late assigned post class instatiation in "setup()" self.datastore = None + # Flag which indicates if the class has been initialized and "setup()" has been called + self._initialized = False + + def setup(self, action_service): + """ + Method which performs additional class initialization. + """ + self.action_service = action_service + self._initialized = True + @abc.abstractmethod def run(self, **kwargs): - pass + if not self._initialized: + raise ValueError('Class hasn\'t been initialized. Make sure setup() is called.') class PythonRunner(ActionRunner): diff --git a/st2actions/st2actions/runners/utils.py b/st2actions/st2actions/runners/utils.py new file mode 100644 index 0000000000..06c6bc78b0 --- /dev/null +++ b/st2actions/st2actions/runners/utils.py @@ -0,0 +1,40 @@ +# Licensed to the StackStorm, Inc ('StackStorm') under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import logging as stdlib_logging + +from st2common import log as logging + +__all__ = [ + 'get_logger_for_python_runner_action' +] + + +def get_logger_for_python_runner_action(self, action_name): + """ + Set up a logger which logs all the messages with level DEBUG and above to stderr. + """ + logger_name = 'actions.python.%s' % (action_name) + logger = logging.getLogger(logger_name) + + console = stdlib_logging.StreamHandler() + console.setLevel(stdlib_logging.DEBUG) + + formatter = stdlib_logging.Formatter('%(name)-12s: %(levelname)-8s %(message)s') + console.setFormatter(formatter) + logger.addHandler(console) + logger.setLevel(stdlib_logging.DEBUG) + + return logger From b2a57589f6f70f3070bdea6a6859cde2b070fcbc Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 17:52:59 +0100 Subject: [PATCH 07/24] Make sure we pass MockActionService with corresponding MockDatastoreService to the Action class used by unit tests. --- st2tests/st2tests/base.py | 10 ++++++ st2tests/st2tests/mocks/action.py | 54 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 st2tests/st2tests/mocks/action.py diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index f50dbb4a75..b8bdd424fe 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -50,6 +50,8 @@ import st2tests.config from st2tests.mocks.sensor import MockSensorWrapper from st2tests.mocks.sensor import MockSensorService +from st2tests.mocks.action import MockActionWrapper +from st2tests.mocks.action import MockActionService __all__ = [ @@ -490,6 +492,13 @@ class BaseActionTestCase(TestCase): action_cls = None + def setUp(self): + super(BaseActionTestCase, self).setUp() + + class_name = self.action_cls.__name__ + action_wrapper = MockActionWrapper(pack='tests', class_name=class_name) + self.action_service = MockActionService(action_wrapper=action_wrapper) + def get_action_instance(self, config=None): """ Retrieve instance of the action class. @@ -500,6 +509,7 @@ def get_action_instance(self, config=None): kwargs['config'] = config instance = self.action_cls(**kwargs) # pylint: disable=not-callable + instance.setup(action_service=action_service) return instance diff --git a/st2tests/st2tests/mocks/action.py b/st2tests/st2tests/mocks/action.py new file mode 100644 index 0000000000..1fda11f547 --- /dev/null +++ b/st2tests/st2tests/mocks/action.py @@ -0,0 +1,54 @@ +# Licensed to the StackStorm, Inc ('StackStorm') under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +Mock classes for use in pack testing. +""" + +from logging import RootLogger + +from mock import Mock + +from st2actions.runners.python_action_wrapper import ActionService +from st2tests.mocks.datastore import MockDatastoreService + +__all__ = [ + 'MockActionWrapper', + 'MockActionService' +] + + +class MockActionWrapper(object): + def __init__(self, pack, class_name): + self._pack = pack + self._class_name = class_name + + +class MockActionService(ActionService): + """ + Mock ActionService for use in testing. + """ + + def __init__(self, action_wrapper): + self._action_wrapper = action_wrapper + + # Holds a mock logger instance + # We use a Mock class so use can assert logger was called with particular arguments + self._logger = Mock(spec=RootLogger) + + self._datastore_service = MockDatastoreService(logger=self._logger, + pack_name=self._action_wrapper._pack, + class_name=self._action_wrapper._class_name, + api_username='action_service') From 0ec9d1fafd8ee4a4f7dd8f8fd67f804beb15e550 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Thu, 18 Feb 2016 18:12:38 +0100 Subject: [PATCH 08/24] Add tests for MockActionService class. --- .../tests/unit/test_unit_testing_mocks.py | 78 +++++++++++-------- 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/st2common/tests/unit/test_unit_testing_mocks.py b/st2common/tests/unit/test_unit_testing_mocks.py index 250638dfda..90f46c132f 100644 --- a/st2common/tests/unit/test_unit_testing_mocks.py +++ b/st2common/tests/unit/test_unit_testing_mocks.py @@ -18,10 +18,13 @@ from st2tests.base import BaseSensorTestCase from st2tests.mocks.sensor import MockSensorWrapper from st2tests.mocks.sensor import MockSensorService +from st2tests.mocks.action import MockActionWrapper +from st2tests.mocks.action import MockActionService __all__ = [ 'BaseSensorTestCaseTestCase', - 'MockSensorServiceTestCase' + 'MockSensorServiceTestCase', + 'MockActionServiceTestCase' ] @@ -29,6 +32,38 @@ class MockSensorClass(object): pass +class BaseMockResourceServiceTestCase(object): + class TestCase(unittest2.TestCase): + def test_list_set_get_delete_values(self): + # list_values, set_value + result = self.mock_service.list_values() + self.assertSequenceEqual(result, []) + + self.mock_service.set_value(name='t1.local', value='test1', local=True) + self.mock_service.set_value(name='t1.global', value='test1', local=False) + + result = self.mock_service.list_values(local=True) + self.assertEqual(len(result), 1) + self.assertEqual(result[0].name, 'dummy.test:t1.local') + + result = self.mock_service.list_values(local=False) + self.assertEqual(result[0].name, 'dummy.test:t1.local') + self.assertEqual(result[1].name, 't1.global') + self.assertEqual(len(result), 2) + + # get_value + self.assertEqual(self.mock_service.get_value('inexistent'), None) + self.assertEqual(self.mock_service.get_value(name='t1.local', local=True), 'test1') + + # delete_value + self.assertEqual(len(self.mock_service.list_values(local=True)), 1) + self.assertEqual(self.mock_service.delete_value('inexistent'), False) + self.assertEqual(len(self.mock_service.list_values(local=True)), 1) + + self.assertEqual(self.mock_service.delete_value('t1.local'), True) + self.assertEqual(len(self.mock_service.list_values(local=True)), 0) + + class BaseSensorTestCaseTestCase(BaseSensorTestCase): sensor_cls = MockSensorClass @@ -51,12 +86,14 @@ def test_dispatch_and_assertTriggerDispacthed(self): payload={'a': 'c'}) -class MockSensorServiceTestCase(unittest2.TestCase): +class MockSensorServiceTestCase(BaseMockResourceServiceTestCase.TestCase): + def setUp(self): - self._mock_sensor_wrapper = MockSensorWrapper(pack='dummy', class_name='test') + mock_sensor_wrapper = MockSensorWrapper(pack='dummy', class_name='test') + self.mock_service = MockSensorService(sensor_wrapper=mock_sensor_wrapper) def test_get_logger(self): - sensor_service = MockSensorService(sensor_wrapper=self._mock_sensor_wrapper) + sensor_service = self.mock_service logger = sensor_service.get_logger('test') logger.info('test info') logger.debug('test debug') @@ -73,33 +110,8 @@ def test_get_logger(self): self.assertEqual(method_args, ('test debug',)) self.assertEqual(method_kwargs, {}) - def test_list_set_get_delete_values(self): - sensor_service = MockSensorService(sensor_wrapper=self._mock_sensor_wrapper) - - # list_values, set_value - result = sensor_service.list_values() - self.assertSequenceEqual(result, []) - - sensor_service.set_value(name='t1.local', value='test1', local=True) - sensor_service.set_value(name='t1.global', value='test1', local=False) - - result = sensor_service.list_values(local=True) - self.assertEqual(len(result), 1) - self.assertEqual(result[0].name, 'dummy.test:t1.local') - result = sensor_service.list_values(local=False) - self.assertEqual(result[0].name, 'dummy.test:t1.local') - self.assertEqual(result[1].name, 't1.global') - self.assertEqual(len(result), 2) - - # get_value - self.assertEqual(sensor_service.get_value('inexistent'), None) - self.assertEqual(sensor_service.get_value(name='t1.local', local=True), 'test1') - - # delete_value - self.assertEqual(len(sensor_service.list_values(local=True)), 1) - self.assertEqual(sensor_service.delete_value('inexistent'), False) - self.assertEqual(len(sensor_service.list_values(local=True)), 1) - - self.assertEqual(sensor_service.delete_value('t1.local'), True) - self.assertEqual(len(sensor_service.list_values(local=True)), 0) +class MockActionServiceTestCase(BaseMockResourceServiceTestCase.TestCase): + def setUp(self): + mock_action_wrapper = MockActionWrapper(pack='dummy', class_name='test') + self.mock_service = MockActionService(action_wrapper=mock_action_wrapper) From 503ea0873f5417d8e5c7b6ce91092a571cbfac99 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Fri, 19 Feb 2016 08:26:59 +0100 Subject: [PATCH 09/24] Update changelog. --- CHANGELOG.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c7da4d7de..b615fe878a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -8,7 +8,9 @@ in development * Allow user to pass a boolean value for the ``cacert`` st2client constructor argument. This way it now mimics the behavior of the ``verify`` argument of the ``requests.request`` method. (improvement) -* Add datastore access to Python actions. (new-feature) #2396 [Kale Blankenship] +* Add datastore access to Python runner actions via the ``action_service`` which is available + to all the Python runner actions after instantiation. (new-feature) #2396 #2511 + [Kale Blankenship] * Allow /v1/webhooks API endpoint request body to either be JSON or url encoded form data. Request body type is determined and parsed accordingly based on the value of ``Content-Type`` header. From 6b3c1859f06649a9391dfa76927793eea627389d Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Fri, 19 Feb 2016 08:59:13 +0100 Subject: [PATCH 10/24] Fix typos. --- st2actions/st2actions/runners/pythonrunner.py | 2 +- st2actions/st2actions/runners/utils.py | 2 +- st2tests/st2tests/base.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/st2actions/st2actions/runners/pythonrunner.py b/st2actions/st2actions/runners/pythonrunner.py index df6d70c430..91031088c6 100644 --- a/st2actions/st2actions/runners/pythonrunner.py +++ b/st2actions/st2actions/runners/pythonrunner.py @@ -70,7 +70,7 @@ def __init__(self, config=None): :type config: ``dict`` """ self.config = config or {} - self.logger = get_logger_for_action(action_name=self.__class__.__name__) + self.logger = get_logger_for_python_runner_action(action_name=self.__class__.__name__) # action_service is late assigned post class instatiation in "setup()" self.datastore = None diff --git a/st2actions/st2actions/runners/utils.py b/st2actions/st2actions/runners/utils.py index 06c6bc78b0..e9fe8ece4c 100644 --- a/st2actions/st2actions/runners/utils.py +++ b/st2actions/st2actions/runners/utils.py @@ -22,7 +22,7 @@ ] -def get_logger_for_python_runner_action(self, action_name): +def get_logger_for_python_runner_action(action_name): """ Set up a logger which logs all the messages with level DEBUG and above to stderr. """ diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index b8bdd424fe..b6e0832bc2 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -509,7 +509,7 @@ def get_action_instance(self, config=None): kwargs['config'] = config instance = self.action_cls(**kwargs) # pylint: disable=not-callable - instance.setup(action_service=action_service) + instance.setup(action_service=self.action_service) return instance From 0d420b2c1da25359d6e55e1111dcf21af4030bdb Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Fri, 19 Feb 2016 09:01:44 +0100 Subject: [PATCH 11/24] Update very out of date isprime example action. --- contrib/examples/actions/pythonactions/__init__.py | 0 contrib/examples/actions/pythonactions/isprime.py | 11 +++++------ 2 files changed, 5 insertions(+), 6 deletions(-) create mode 100644 contrib/examples/actions/pythonactions/__init__.py diff --git a/contrib/examples/actions/pythonactions/__init__.py b/contrib/examples/actions/pythonactions/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/contrib/examples/actions/pythonactions/isprime.py b/contrib/examples/actions/pythonactions/isprime.py index f38ff8abef..e57bf79456 100644 --- a/contrib/examples/actions/pythonactions/isprime.py +++ b/contrib/examples/actions/pythonactions/isprime.py @@ -1,12 +1,11 @@ import math +from st2actions.runners.pythonrunner import Action -class PrimeChecker(object): - - def __init__(self, config=None): - pass +class PrimeCheckerAction(Action): def run(self, value=0): + self.logger.debug('value=%s' % (value)) if math.floor(value) != value: raise ValueError('%s should be an integer.' % value) if value < 2: @@ -17,6 +16,6 @@ def run(self, value=0): return True if __name__ == '__main__': - checker = PrimeChecker() + checker = PrimeCheckerAction() for i in range(0, 10): - print '%s : %s' % (i, checker.run(**{'value': i})) + print '%s : %s' % (i, checker.run(value=1)) From 42b5369a1608417b6503d914b90d14945514bff8 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Fri, 19 Feb 2016 09:02:02 +0100 Subject: [PATCH 12/24] Add tests for example is prime action. --- contrib/examples/tests/test_action_isprime.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 contrib/examples/tests/test_action_isprime.py diff --git a/contrib/examples/tests/test_action_isprime.py b/contrib/examples/tests/test_action_isprime.py new file mode 100644 index 0000000000..533de72539 --- /dev/null +++ b/contrib/examples/tests/test_action_isprime.py @@ -0,0 +1,15 @@ +from st2tests.base import BaseActionTestCase + +from pythonactions.isprime import PrimeCheckerAction + + +class PrimeCheckerActionTestCase(BaseActionTestCase): + action_cls = PrimeCheckerAction + + def test_run(self): + action = self.get_action_instance() + result = action.run(value=1) + self.assertFalse(result) + + result = action.run(value=3) + self.assertTrue(result) From 60e2d219e16b48460162214ed0b3b555565df692 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Fri, 19 Feb 2016 09:02:54 +0100 Subject: [PATCH 13/24] Fix lint. --- st2actions/st2actions/runners/python_action_wrapper.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/st2actions/st2actions/runners/python_action_wrapper.py b/st2actions/st2actions/runners/python_action_wrapper.py index 611afcde15..80fe9e6380 100644 --- a/st2actions/st2actions/runners/python_action_wrapper.py +++ b/st2actions/st2actions/runners/python_action_wrapper.py @@ -16,7 +16,6 @@ import sys import json import argparse -import logging as stdlib_logging from st2common import log as logging from st2actions import config @@ -42,7 +41,7 @@ class ActionService(object): """ def __init__(self, action_wrapper): - logger = get_logger_for_action(action_name=self._action_wrapper._class_name) + logger = get_logger_for_python_runner_action(action_name=action_wrapper._class_name) self._action_wrapper = action_wrapper self._datastore_service = DatastoreService(logger=logger, @@ -136,7 +135,6 @@ def _get_action_instance(self): # Action constructor to take in additional argument (action_service). action_service = ActionService(action_wrapper=self) action_instance.setup(action_service=action_service) - action_instance.logger = self._set_up_logger(action_cls.__name__) return action_instance From e80249aba75316eeb393b6c5fd263083741ece44 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 13:54:10 +0100 Subject: [PATCH 14/24] Switch to a new approach where we try to pass action_service argument to the Action class constructor. If that doesn't work (e.g. old action which hasn't been updated yet), result to late assignment. --- .../runners/python_action_wrapper.py | 14 ++++----- st2actions/st2actions/runners/pythonrunner.py | 22 ++++--------- st2actions/st2actions/runners/utils.py | 31 ++++++++++++++++++- st2tests/st2tests/base.py | 3 +- 4 files changed, 45 insertions(+), 25 deletions(-) diff --git a/st2actions/st2actions/runners/python_action_wrapper.py b/st2actions/st2actions/runners/python_action_wrapper.py index 80fe9e6380..436102f8a1 100644 --- a/st2actions/st2actions/runners/python_action_wrapper.py +++ b/st2actions/st2actions/runners/python_action_wrapper.py @@ -21,6 +21,7 @@ from st2actions import config from st2actions.runners.pythonrunner import Action from st2actions.runners.utils import get_logger_for_python_runner_action +from st2actions.runners.utils import get_action_class_instance from st2common.util import loader as action_loader from st2common.util.config_parser import ContentPackConfigParser from st2common.constants.action import ACTION_OUTPUT_RESULT_DELIMITER @@ -121,21 +122,20 @@ def _get_action_instance(self): config_parser = ContentPackConfigParser(pack_name=self._pack) config = config_parser.get_action_config(action_file_path=self._file_path) + kwargs = {} if config: LOG.info('Using config "%s" for action "%s"' % (config.file_path, self._file_path)) - - action_instance = action_cls(config=config.config) + kwargs['config'] = config.config else: LOG.info('No config found for action "%s"' % (self._file_path)) - action_instance = action_cls(config={}) + kwargs['config'] = {} - # Perform post instantiation action intiailization - # Note: This is needed since for backward compatibility reasons we can't simply update - # Action constructor to take in additional argument (action_service). action_service = ActionService(action_wrapper=self) - action_instance.setup(action_service=action_service) + kwargs['action_service'] = action_service + action_instance = get_action_class_instance(action_cls=action_cls, + kwargs=kwargs) return action_instance diff --git a/st2actions/st2actions/runners/pythonrunner.py b/st2actions/st2actions/runners/pythonrunner.py index 91031088c6..328247c611 100644 --- a/st2actions/st2actions/runners/pythonrunner.py +++ b/st2actions/st2actions/runners/pythonrunner.py @@ -64,31 +64,21 @@ class Action(object): description = None - def __init__(self, config=None): + def __init__(self, config=None, action_service=None): """ :param config: Action config. :type config: ``dict`` - """ - self.config = config or {} - self.logger = get_logger_for_python_runner_action(action_name=self.__class__.__name__) - - # action_service is late assigned post class instatiation in "setup()" - self.datastore = None - # Flag which indicates if the class has been initialized and "setup()" has been called - self._initialized = False - - def setup(self, action_service): - """ - Method which performs additional class initialization. + :param action_service: ActionService object. + :type action_service: :class:`ActionService~ """ + self.config = config or {} self.action_service = action_service - self._initialized = True + self.logger = get_logger_for_python_runner_action(action_name=self.__class__.__name__) @abc.abstractmethod def run(self, **kwargs): - if not self._initialized: - raise ValueError('Class hasn\'t been initialized. Make sure setup() is called.') + pass class PythonRunner(ActionRunner): diff --git a/st2actions/st2actions/runners/utils.py b/st2actions/st2actions/runners/utils.py index e9fe8ece4c..788d24510d 100644 --- a/st2actions/st2actions/runners/utils.py +++ b/st2actions/st2actions/runners/utils.py @@ -13,14 +13,19 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy + import logging as stdlib_logging from st2common import log as logging __all__ = [ - 'get_logger_for_python_runner_action' + 'get_logger_for_python_runner_action', + 'get_action_class_instance' ] +LOG = logging.getLogger(__name__) + def get_logger_for_python_runner_action(action_name): """ @@ -38,3 +43,27 @@ def get_logger_for_python_runner_action(action_name): logger.setLevel(stdlib_logging.DEBUG) return logger + + +def get_action_class_instance(action_cls, kwargs): + """ + Instantiate and return Action class instance. + """ + # Note: This is done for backward compatibility reasons. We first try to pass + # "action_service" argument to the action class constructor, but if that doesn't work + # (e.g. old action which hasn't been updated yet), we resort to late assignment. + try: + action_instance = action_cls(**kwargs) + except TypeError as e: + if 'unexpected keyword argument \'action_service\'' not in str(e): + raise e + + LOG.debug('Action class constructor doesn\'t take "action_service" argument, ' + 'falling back to late assignment...') + + kwargs = copy.deepcopy(kwargs) + action_service = kwargs.pop('action_service', None) + action_instance = action_cls(**kwargs) + action_instance.action_service = action_service + + return action_instance diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index b6e0832bc2..88a9941df0 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -46,6 +46,7 @@ import st2common.models.db.liveaction as liveaction_model import st2common.models.db.actionalias as actionalias_model import st2common.models.db.policy as policy_model +from st2actions.runners.utils import get_action_class_instance import st2tests.config from st2tests.mocks.sensor import MockSensorWrapper @@ -507,9 +508,9 @@ def get_action_instance(self, config=None): if config: kwargs['config'] = config + kwargs['action_service'] = self.action_service instance = self.action_cls(**kwargs) # pylint: disable=not-callable - instance.setup(action_service=self.action_service) return instance From 0af4db193c11690dd2f3726fc777a3b66b99a8ee Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:09:38 +0100 Subject: [PATCH 15/24] Add tests for action class instantion. --- st2actions/tests/unit/test_pythonrunner.py | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/st2actions/tests/unit/test_pythonrunner.py b/st2actions/tests/unit/test_pythonrunner.py index 7dc9cae8f7..7ee53bedc6 100644 --- a/st2actions/tests/unit/test_pythonrunner.py +++ b/st2actions/tests/unit/test_pythonrunner.py @@ -18,7 +18,9 @@ import mock from st2actions.runners import pythonrunner +from st2actions.runners.pythonrunner import Action from st2actions.container import service +from st2actions.runners.utils import get_action_class_instance from st2common.constants.action import ACTION_OUTPUT_RESULT_DELIMITER from st2common.constants.action import LIVEACTION_STATUS_SUCCEEDED, LIVEACTION_STATUS_FAILED from st2common.constants.pack import SYSTEM_PACK_NAME @@ -184,6 +186,48 @@ def test_common_st2_env_vars_are_available_to_the_action(self, mock_popen): actual_env = call_kwargs['env'] self.assertCommonSt2EnvVarsAvailableInEnv(env=actual_env) + def test_action_class_instantiation_action_service_argument(self): + class Action1(Action): + # Constructor not overriden so no issue here + pass + + def run(self): + pass + + class Action2(Action): + # Constructor overriden, but takes action_service argument + def __init__(self, config, action_service=None): + super(Action2, self).__init__(config=config, + action_service=action_service) + + def run(self): + pass + + class Action3(Action): + # Constructor overriden, but doesn't take to action service + def __init__(self, config): + super(Action3, self).__init__(config=config) + + def run(self): + pass + + kwargs = { + 'config': 'bar', + 'action_service': 'action service' + } + + action = get_action_class_instance(action_cls=Action1, kwargs=kwargs) + self.assertEqual(action.config, kwargs['config']) + self.assertEqual(action.action_service, kwargs['action_service']) + + action = get_action_class_instance(action_cls=Action2, kwargs=kwargs) + self.assertEqual(action.config, kwargs['config']) + self.assertEqual(action.action_service, kwargs['action_service']) + + action = get_action_class_instance(action_cls=Action3, kwargs=kwargs) + self.assertEqual(action.config, kwargs['config']) + self.assertEqual(action.action_service, kwargs['action_service']) + def _get_mock_action_obj(self): """ Return mock action object. From 315ab9084cd0c4430eb123a0e3d2269ef801ba50 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:11:06 +0100 Subject: [PATCH 16/24] Update changelog. --- CHANGELOG.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b615fe878a..ed0d048534 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -11,6 +11,8 @@ in development * Add datastore access to Python runner actions via the ``action_service`` which is available to all the Python runner actions after instantiation. (new-feature) #2396 #2511 [Kale Blankenship] +* Update ``st2actions.runners.pythonrunner.Action`` class so the constructor also takes + ``action_service`` as the second argument. * Allow /v1/webhooks API endpoint request body to either be JSON or url encoded form data. Request body type is determined and parsed accordingly based on the value of ``Content-Type`` header. From 5a9aa59a3bd11613dc49c72512c759914248df9e Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:12:18 +0100 Subject: [PATCH 17/24] Update tests. --- st2tests/st2tests/base.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index 88a9941df0..82a2d3e195 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -510,7 +510,9 @@ def get_action_instance(self, config=None): kwargs['config'] = config kwargs['action_service'] = self.action_service - instance = self.action_cls(**kwargs) # pylint: disable=not-callable + # pylint: disable=not-callable + instance = get_action_class_instance(action_cls=self.action_cls, kwargs=kwargs) + instance = self.action_cls(**kwargs) return instance From 935ae4a7c937325799d80743fc474bcf63f6a444 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:16:23 +0100 Subject: [PATCH 18/24] Update affected core pack actions. --- contrib/chatops/actions/format_execution_result.py | 6 +++--- contrib/packs/actions/pack_mgmt/delete.py | 4 ++-- contrib/packs/actions/pack_mgmt/download.py | 4 ++-- contrib/packs/actions/pack_mgmt/setup_virtualenv.py | 5 +++-- contrib/packs/actions/pack_mgmt/unload.py | 4 ++-- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/contrib/chatops/actions/format_execution_result.py b/contrib/chatops/actions/format_execution_result.py index 55836bee23..8873d36b6a 100755 --- a/contrib/chatops/actions/format_execution_result.py +++ b/contrib/chatops/actions/format_execution_result.py @@ -7,8 +7,8 @@ class FormatResultAction(Action): - def __init__(self, config): - super(FormatResultAction, self).__init__(config) + def __init__(self, config=None, action_service=None): + super(FormatResultAction, self).__init__(config=config, action_service=action_service) api_url = os.environ.get('ST2_ACTION_API_URL', None) token = os.environ.get('ST2_ACTION_AUTH_TOKEN', None) self.client = Client(api_url=api_url, token=token) @@ -51,4 +51,4 @@ def _get_execution(self, execution_id): if not execution: return None excludes = ["trigger", "trigger_type", "trigger_instance", "liveaction"] - return execution.to_dict(exclude_attributes= excludes) + return execution.to_dict(exclude_attributes=excludes) diff --git a/contrib/packs/actions/pack_mgmt/delete.py b/contrib/packs/actions/pack_mgmt/delete.py index caada04d00..eebd9635a2 100644 --- a/contrib/packs/actions/pack_mgmt/delete.py +++ b/contrib/packs/actions/pack_mgmt/delete.py @@ -26,8 +26,8 @@ class UninstallPackAction(Action): - def __init__(self, config=None): - super(UninstallPackAction, self).__init__(config=config) + def __init__(self, config=None, action_service=None): + super(UninstallPackAction, self).__init__(config=config, action_service=action_service) self._base_virtualenvs_path = os.path.join(cfg.CONF.system.base_path, 'virtualenvs/') diff --git a/contrib/packs/actions/pack_mgmt/download.py b/contrib/packs/actions/pack_mgmt/download.py index a8a6f33312..e54a0241c9 100644 --- a/contrib/packs/actions/pack_mgmt/download.py +++ b/contrib/packs/actions/pack_mgmt/download.py @@ -61,8 +61,8 @@ class DownloadGitRepoAction(Action): - def __init__(self, config=None): - super(DownloadGitRepoAction, self).__init__(config=config) + def __init__(self, config=None, action_service=None): + super(DownloadGitRepoAction, self).__init__(config=config, action_service=action_service) self._subtree = None self._repo_url = None diff --git a/contrib/packs/actions/pack_mgmt/setup_virtualenv.py b/contrib/packs/actions/pack_mgmt/setup_virtualenv.py index d813dbf673..74b02e6d4a 100644 --- a/contrib/packs/actions/pack_mgmt/setup_virtualenv.py +++ b/contrib/packs/actions/pack_mgmt/setup_virtualenv.py @@ -43,8 +43,9 @@ class SetupVirtualEnvironmentAction(Action): current dependencies as well as an installation of new dependencies """ - def __init__(self, config=None): - super(SetupVirtualEnvironmentAction, self).__init__(config=config) + def __init__(self, config=None, action_service=None): + super(SetupVirtualEnvironmentAction, self).__init__(config=config, + action_service=action_service) self._base_virtualenvs_path = os.path.join(cfg.CONF.system.base_path, 'virtualenvs/') diff --git a/contrib/packs/actions/pack_mgmt/unload.py b/contrib/packs/actions/pack_mgmt/unload.py index 0263eaaf3e..e36c01bff6 100644 --- a/contrib/packs/actions/pack_mgmt/unload.py +++ b/contrib/packs/actions/pack_mgmt/unload.py @@ -31,8 +31,8 @@ class UnregisterPackAction(BaseAction): - def __init__(self, config=None): - super(UnregisterPackAction, self).__init__(config=config) + def __init__(self, config=None, action_service=None): + super(UnregisterPackAction, self).__init__(config=config, action_service=action_service) self.initialize() def initialize(self): From 15ab5073a04de972269eee2482bdcac6b807c1f4 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:27:17 +0100 Subject: [PATCH 19/24] Make sure we also lint python files in chatops pack. --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 0b39a25dfb..bbe3774ce8 100644 --- a/Makefile +++ b/Makefile @@ -69,9 +69,9 @@ pylint: requirements .pylint done # Lint Python pack management actions . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models contrib/packs/actions/ || exit 1; - # Lint other packs . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models contrib/linux || exit 1; + . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models contrib/chatops || exit 1; # Lint Python scripts . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models scripts/*.py || exit 1; . $(VIRTUALENV_DIR)/bin/activate; pylint -E --rcfile=./lint-configs/python/.pylintrc --load-plugins=pylint_plugins.api_models tools/*.py || exit 1; @@ -87,6 +87,7 @@ flake8: requirements .flake8 . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 $(COMPONENTS) . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 contrib/packs/actions/ . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 contrib/linux + . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 contrib/chatops/ . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 scripts/ . $(VIRTUALENV_DIR)/bin/activate; flake8 --config ./lint-configs/python/.flake8 tools/ From 44b36e6d8e1bf1d6cc86d3eeb2802ffedeebbbb2 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:35:36 +0100 Subject: [PATCH 20/24] Update __all__. --- st2actions/st2actions/runners/python_action_wrapper.py | 3 ++- st2reactor/st2reactor/container/sensor_wrapper.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/st2actions/st2actions/runners/python_action_wrapper.py b/st2actions/st2actions/runners/python_action_wrapper.py index 436102f8a1..168e2f9981 100644 --- a/st2actions/st2actions/runners/python_action_wrapper.py +++ b/st2actions/st2actions/runners/python_action_wrapper.py @@ -29,7 +29,8 @@ from st2common.services.datastore import DatastoreService __all__ = [ - 'PythonActionWrapper' + 'PythonActionWrapper', + 'ActionService' ] LOG = logging.getLogger(__name__) diff --git a/st2reactor/st2reactor/container/sensor_wrapper.py b/st2reactor/st2reactor/container/sensor_wrapper.py index 2bd873f33a..114da6b80c 100644 --- a/st2reactor/st2reactor/container/sensor_wrapper.py +++ b/st2reactor/st2reactor/container/sensor_wrapper.py @@ -35,7 +35,8 @@ from st2common.services.datastore import DatastoreService __all__ = [ - 'SensorWrapper' + 'SensorWrapper', + 'SensorService' ] eventlet.monkey_patch( From 454427ead5c7125abb78e9dd95d181fc7a7b4516 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:55:06 +0100 Subject: [PATCH 21/24] Pass actual objects instead of kwargs to the function. --- .../runners/python_action_wrapper.py | 10 ++++------ st2actions/st2actions/runners/utils.py | 18 ++++++++++++++---- st2tests/st2tests/base.py | 11 +++-------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/st2actions/st2actions/runners/python_action_wrapper.py b/st2actions/st2actions/runners/python_action_wrapper.py index 168e2f9981..86c37b6c14 100644 --- a/st2actions/st2actions/runners/python_action_wrapper.py +++ b/st2actions/st2actions/runners/python_action_wrapper.py @@ -123,20 +123,18 @@ def _get_action_instance(self): config_parser = ContentPackConfigParser(pack_name=self._pack) config = config_parser.get_action_config(action_file_path=self._file_path) - kwargs = {} if config: LOG.info('Using config "%s" for action "%s"' % (config.file_path, self._file_path)) - kwargs['config'] = config.config + config = config.config else: LOG.info('No config found for action "%s"' % (self._file_path)) - kwargs['config'] = {} + config = None action_service = ActionService(action_wrapper=self) - kwargs['action_service'] = action_service - action_instance = get_action_class_instance(action_cls=action_cls, - kwargs=kwargs) + config=config, + action_service=action_service) return action_instance diff --git a/st2actions/st2actions/runners/utils.py b/st2actions/st2actions/runners/utils.py index 788d24510d..e8f7550612 100644 --- a/st2actions/st2actions/runners/utils.py +++ b/st2actions/st2actions/runners/utils.py @@ -13,8 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import copy - import logging as stdlib_logging from st2common import log as logging @@ -45,10 +43,23 @@ def get_logger_for_python_runner_action(action_name): return logger -def get_action_class_instance(action_cls, kwargs): +def get_action_class_instance(action_cls, config=None, action_service=None): """ Instantiate and return Action class instance. + + :param action_cls: Action class to instantiate. + :type action_cls: ``class`` + + :param config: Config to pass to the action class. + :type config: ``dict`` + + :param action_service: ActionService instance to pass to the class. + :type action_service: :class:`ActionService` """ + kwargs = {} + kwargs['config'] = config + kwargs['action_service'] = action_service + # Note: This is done for backward compatibility reasons. We first try to pass # "action_service" argument to the action class constructor, but if that doesn't work # (e.g. old action which hasn't been updated yet), we resort to late assignment. @@ -61,7 +72,6 @@ def get_action_class_instance(action_cls, kwargs): LOG.debug('Action class constructor doesn\'t take "action_service" argument, ' 'falling back to late assignment...') - kwargs = copy.deepcopy(kwargs) action_service = kwargs.pop('action_service', None) action_instance = action_cls(**kwargs) action_instance.action_service = action_service diff --git a/st2tests/st2tests/base.py b/st2tests/st2tests/base.py index 82a2d3e195..992d190234 100644 --- a/st2tests/st2tests/base.py +++ b/st2tests/st2tests/base.py @@ -504,15 +504,10 @@ def get_action_instance(self, config=None): """ Retrieve instance of the action class. """ - kwargs = {} - - if config: - kwargs['config'] = config - kwargs['action_service'] = self.action_service - # pylint: disable=not-callable - instance = get_action_class_instance(action_cls=self.action_cls, kwargs=kwargs) - instance = self.action_cls(**kwargs) + instance = get_action_class_instance(action_cls=self.action_cls, + config=config, + action_service=self.action_service) return instance From 2078ba357b331f071e1bd36e71ed69ed68cceb1d Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 14:56:15 +0100 Subject: [PATCH 22/24] Add todo annotation. --- st2actions/st2actions/runners/utils.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/st2actions/st2actions/runners/utils.py b/st2actions/st2actions/runners/utils.py index e8f7550612..040f304e8b 100644 --- a/st2actions/st2actions/runners/utils.py +++ b/st2actions/st2actions/runners/utils.py @@ -61,8 +61,9 @@ def get_action_class_instance(action_cls, config=None, action_service=None): kwargs['action_service'] = action_service # Note: This is done for backward compatibility reasons. We first try to pass - # "action_service" argument to the action class constructor, but if that doesn't work - # (e.g. old action which hasn't been updated yet), we resort to late assignment. + # "action_service" argument to the action class constructor, but if that doesn't work (e.g. old + # action which hasn't been updated yet), we resort to late assignment post class instantiation. + # TODO: Remove in next major version once all the affected actions have been updated. try: action_instance = action_cls(**kwargs) except TypeError as e: From d1e9107b375c78bda5ec0b7cf1ce8f2e68fc968b Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 15:00:00 +0100 Subject: [PATCH 23/24] Include class name. --- st2actions/st2actions/runners/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2actions/st2actions/runners/utils.py b/st2actions/st2actions/runners/utils.py index 040f304e8b..b78be230ff 100644 --- a/st2actions/st2actions/runners/utils.py +++ b/st2actions/st2actions/runners/utils.py @@ -70,8 +70,8 @@ def get_action_class_instance(action_cls, config=None, action_service=None): if 'unexpected keyword argument \'action_service\'' not in str(e): raise e - LOG.debug('Action class constructor doesn\'t take "action_service" argument, ' - 'falling back to late assignment...') + LOG.debug('Action class (%s) constructor doesn\'t take "action_service" argument, ' + 'falling back to late assignment...' % (action_cls.__class__.__name__)) action_service = kwargs.pop('action_service', None) action_instance = action_cls(**kwargs) From 99452ea2e884376d87eee20cb26ac73a61342673 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 22 Feb 2016 15:01:26 +0100 Subject: [PATCH 24/24] Update affected tests. --- st2actions/tests/unit/test_pythonrunner.py | 33 +++++++++++----------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/st2actions/tests/unit/test_pythonrunner.py b/st2actions/tests/unit/test_pythonrunner.py index 7ee53bedc6..eb3e4ba31e 100644 --- a/st2actions/tests/unit/test_pythonrunner.py +++ b/st2actions/tests/unit/test_pythonrunner.py @@ -211,22 +211,23 @@ def __init__(self, config): def run(self): pass - kwargs = { - 'config': 'bar', - 'action_service': 'action service' - } - - action = get_action_class_instance(action_cls=Action1, kwargs=kwargs) - self.assertEqual(action.config, kwargs['config']) - self.assertEqual(action.action_service, kwargs['action_service']) - - action = get_action_class_instance(action_cls=Action2, kwargs=kwargs) - self.assertEqual(action.config, kwargs['config']) - self.assertEqual(action.action_service, kwargs['action_service']) - - action = get_action_class_instance(action_cls=Action3, kwargs=kwargs) - self.assertEqual(action.config, kwargs['config']) - self.assertEqual(action.action_service, kwargs['action_service']) + config = {'a': 1, 'b': 2} + action_service = 'ActionService!' + + action1 = get_action_class_instance(action_cls=Action1, config=config, + action_service=action_service) + self.assertEqual(action1.config, config) + self.assertEqual(action1.action_service, action_service) + + action2 = get_action_class_instance(action_cls=Action2, config=config, + action_service=action_service) + self.assertEqual(action2.config, config) + self.assertEqual(action2.action_service, action_service) + + action3 = get_action_class_instance(action_cls=Action3, config=config, + action_service=action_service) + self.assertEqual(action3.config, config) + self.assertEqual(action3.action_service, action_service) def _get_mock_action_obj(self): """