diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ba34d5c6a6..9d27649bb8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -31,6 +31,10 @@ Docs: http://docks.stackstorm.com/latest and ``set_poll_interval`` methods respectively. (new-feature) * Add support for a ``standalone`` mode to the st2auth service. In the standalone mode, authentication is handled inside the st2auth service using the defined backend. (new feature) +* Timer is not a sensor anymore. It is spun as part of rules_engine process (refactor) +* Fix a bug with action registration where action with invalid schema for + parameters get registered. (bug-fix) +* Fix a bug with 'default' param values inheritance in runner/actions. (bug-fix) v0.6.0 - December 8, 2014 ------------------------- diff --git a/contrib/core/actions/sendmail.json b/contrib/core/actions/sendmail.json index 9ddd6fa9ab..d590d5333c 100644 --- a/contrib/core/actions/sendmail.json +++ b/contrib/core/actions/sendmail.json @@ -23,17 +23,8 @@ "required": true, "position": 2 }, - "user": { - "immutable": true - }, - "dir": { - "immutable": true - }, "sudo": { "immutable": true - }, - "parallel": { - "immutable": true } } } diff --git a/contrib/core/actions/stormbot_say.json b/contrib/core/actions/stormbot_say.json index 1e865925de..584a53bafc 100644 --- a/contrib/core/actions/stormbot_say.json +++ b/contrib/core/actions/stormbot_say.json @@ -7,12 +7,6 @@ "sudo": { "immutable": true }, - "user": { - "immutable": true - }, - "dir": { - "immutable": true - }, "source": { "type": "string", "position": 0 diff --git a/contrib/examples/actions/ubuntu_pkg_info.json b/contrib/examples/actions/ubuntu_pkg_info.json index fedd4db18d..097b01043c 100644 --- a/contrib/examples/actions/ubuntu_pkg_info.json +++ b/contrib/examples/actions/ubuntu_pkg_info.json @@ -11,17 +11,8 @@ "required": true, "position": 0 }, - "user": { - "immutable": true - }, - "dir": { - "immutable": true - }, "sudo": { "immutable": true - }, - "parallel": { - "immutable": true } } } diff --git a/st2actions/st2actions/bootstrap/actionsregistrar.py b/st2actions/st2actions/bootstrap/actionsregistrar.py index eb31ef37f0..cddfa81d08 100644 --- a/st2actions/st2actions/bootstrap/actionsregistrar.py +++ b/st2actions/st2actions/bootstrap/actionsregistrar.py @@ -24,9 +24,10 @@ from st2common.content.loader import (ContentPackLoader, MetaLoader) from st2common.content.validators import RequirementsValidator from st2common.persistence.action import Action -from st2common.models.db.action import ActionDB +from st2common.models.api.action import ActionAPI from st2common.models.system.common import ResourceReference import st2common.util.action_db as action_utils +import st2common.validators.api.action as action_validator LOG = logging.getLogger(__name__) @@ -48,23 +49,26 @@ def _get_actions_from_pack(self, actions_dir): def _register_action(self, pack, action): content = self._meta_loader.load(action) + pack_field = content.get('pack', None) + if not pack_field: + content['pack'] = pack + pack_field = pack + if pack_field != pack: + raise Exception('Model is in pack "%s" but field "pack" is different: %s' % + (pack, pack_field)) + + action_api = ActionAPI(**content) + action_validator.validate_action(action_api) + model = ActionAPI.to_model(action_api) + action_ref = ResourceReference(pack=pack, name=str(content['name'])) - model = action_utils.get_action_by_ref(action_ref) - if not model: - model = ActionDB() - model.name = content['name'] - model.description = content['description'] - model.enabled = content['enabled'] - model.pack = pack - model.entry_point = content['entry_point'] - model.parameters = content.get('parameters', {}) - runner_type = str(content['runner_type']) - valid_runner_type, runner_type_db = self._has_valid_runner_type(runner_type) - if valid_runner_type: - model.runner_type = {'name': runner_type_db.name} + existing = action_utils.get_action_by_ref(action_ref) + if not existing: + LOG.info('Action %s not found. Creating new one with: %s', action_ref, content) else: - LOG.exception('Runner type %s doesn\'t exist.', runner_type) - raise + LOG.info('Action %s found. Will be updated from: %s to: %s', + action_ref, existing, model) + model.id = existing.id try: model = Action.add_or_update(model) @@ -73,12 +77,6 @@ def _register_action(self, pack, action): LOG.exception('Failed to write action to db %s.', model.name) raise - def _has_valid_runner_type(self, runner_type): - try: - return True, action_utils.get_runnertype_by_name(runner_type) - except: - return False, None - def _register_actions_from_pack(self, pack, actions): for action in actions: try: diff --git a/st2actions/st2actions/bootstrap/runnersregistrar.py b/st2actions/st2actions/bootstrap/runnersregistrar.py index 6a244c9479..291ec0fe5c 100644 --- a/st2actions/st2actions/bootstrap/runnersregistrar.py +++ b/st2actions/st2actions/bootstrap/runnersregistrar.py @@ -116,7 +116,8 @@ def register_runner_types(): }, 'sudo': { 'description': 'The remote command will be executed with sudo.', - 'type': 'boolean' + 'type': 'boolean', + 'default': False }, 'dir': { 'description': 'The working directory where the command will be ' @@ -158,7 +159,8 @@ def register_runner_types(): }, 'sudo': { 'description': 'The remote command will be executed with sudo.', - 'type': 'boolean' + 'type': 'boolean', + 'default': False }, 'dir': { 'description': 'The working directory where the command will be ' diff --git a/st2actions/st2actions/utils/param_utils.py b/st2actions/st2actions/utils/param_utils.py index bd604cec12..f7f14eae68 100644 --- a/st2actions/st2actions/utils/param_utils.py +++ b/st2actions/st2actions/utils/param_utils.py @@ -104,14 +104,18 @@ def _get_resolved_runner_params(runner_parameters, action_parameters, # No override if param is immutable if param_value.get('immutable', False): continue + # Check if param exists in action_parameters and if it has a default value then # pickup the override. - if param_name in action_parameters and 'default' in action_parameters[param_name]: + if param_name in action_parameters: action_param = action_parameters[param_name] - resolved_params[param_name] = action_param['default'] - # No further override if param is immutable + if action_param.get('default', False): + resolved_params[param_name] = action_param['default'] + + # No further override (from actionexecution) if param is immutable if action_param.get('immutable', False): continue + # Finally pick up override from actionexec_runner_parameters if param_name in actionexec_runner_parameters: resolved_params[param_name] = actionexec_runner_parameters[param_name] diff --git a/st2actions/tests/unit/test_actions_registrar.py b/st2actions/tests/unit/test_actions_registrar.py index 8d01e41b66..9d0fd179c2 100644 --- a/st2actions/tests/unit/test_actions_registrar.py +++ b/st2actions/tests/unit/test_actions_registrar.py @@ -17,14 +17,15 @@ import simplejson as json except ImportError: import json -import os - +import jsonschema import mock import st2actions.bootstrap.actionsregistrar as actions_registrar from st2common.persistence.action import Action +import st2common.validators.api.action as action_validator from st2common.models.db.action import RunnerTypeDB import st2tests.base as tests_base +import st2tests.fixturesloader as fixtures_loader MOCK_RUNNER_TYPE_DB = RunnerTypeDB() MOCK_RUNNER_TYPE_DB.name = 'run-local' @@ -32,18 +33,20 @@ class ActionsRegistrarTest(tests_base.DbTestCase): - @mock.patch.object(actions_registrar.ActionsRegistrar, '_has_valid_runner_type', - mock.MagicMock(return_value=(True, MOCK_RUNNER_TYPE_DB))) + @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True)) + @mock.patch.object(action_validator, '_get_runner_model', + mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB)) def test_register_all_actions(self): try: - packs_base_path = os.path.join(tests_base.get_fixtures_path()) + packs_base_path = fixtures_loader.get_fixtures_base_path() all_actions_in_db = Action.get_all() actions_registrar.register_actions(packs_base_path=packs_base_path) - all_actions_in_db = Action.get_all() - self.assertTrue(len(all_actions_in_db) > 0) except Exception as e: print(str(e)) self.fail('All actions must be registered without exceptions.') + else: + all_actions_in_db = Action.get_all() + self.assertTrue(len(all_actions_in_db) > 0) def test_register_actions_from_bad_pack(self): packs_base_path = tests_base.get_fixtures_path() @@ -53,12 +56,14 @@ def test_register_actions_from_bad_pack(self): except: pass - @mock.patch.object(actions_registrar.ActionsRegistrar, '_has_valid_runner_type', - mock.MagicMock(return_value=(True, MOCK_RUNNER_TYPE_DB))) + @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True)) + @mock.patch.object(action_validator, '_get_runner_model', + mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB)) def test_pack_name_missing(self): registrar = actions_registrar.ActionsRegistrar() - action_file = os.path.join(tests_base.get_fixtures_path(), - 'wolfpack/actions/action_3_pack_missing.json') + loader = fixtures_loader.FixturesLoader() + action_file = loader.get_fixture_file_path_abs( + 'generic', 'actions', 'action_3_pack_missing.json') registrar._register_action('dummy', action_file) action_name = None with open(action_file, 'r') as fd: @@ -68,3 +73,37 @@ def test_pack_name_missing(self): self.assertEqual(action_db.pack, 'dummy', 'Content pack must be ' + 'set to dummy') Action.delete(action_db) + + @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True)) + @mock.patch.object(action_validator, '_get_runner_model', + mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB)) + def test_invalid_params_schema(self): + registrar = actions_registrar.ActionsRegistrar() + loader = fixtures_loader.FixturesLoader() + action_file = loader.get_fixture_file_path_abs( + 'generic', 'actions', 'action-invalid-schema-params.yaml') + try: + registrar._register_action('dummy', action_file) + self.fail('Invalid action schema. Should have failed.') + except jsonschema.ValidationError: + pass + + @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True)) + @mock.patch.object(action_validator, '_get_runner_model', + mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB)) + def test_action_update(self): + registrar = actions_registrar.ActionsRegistrar() + loader = fixtures_loader.FixturesLoader() + action_file = loader.get_fixture_file_path_abs( + 'generic', 'actions', 'action1.json') + registrar._register_action('wolfpack', action_file) + # try registering again. this should not throw errors. + registrar._register_action('wolfpack', action_file) + action_name = None + with open(action_file, 'r') as fd: + content = json.load(fd) + action_name = str(content['name']) + action_db = Action.get_by_name(action_name) + self.assertEqual(action_db.pack, 'wolfpack', 'Content pack must be ' + + 'set to wolfpack') + Action.delete(action_db) diff --git a/st2actions/tests/unit/test_param_utils.py b/st2actions/tests/unit/test_param_utils.py index c6fd366d00..88c9512fe0 100644 --- a/st2actions/tests/unit/test_param_utils.py +++ b/st2actions/tests/unit/test_param_utils.py @@ -105,6 +105,8 @@ def test_get_resolved_params(self): self.assertEqual(runner_params.get('runnerint'), 555) # Assert that a runner param can be overriden by action param default. self.assertEqual(runner_params.get('runnerdummy'), 'actiondummy') + # Asser that runner param made immutable in action can use default value in runner. + self.assertEqual(runner_params.get('runnerfoo'), 'FOO') # Assert that an immutable param cannot be overriden by action param or execution param. self.assertEqual(runner_params.get('runnerimmutable'), 'runnerimmutable') @@ -328,6 +330,10 @@ def _setup_runner_models(cls): 'description': 'Foo int param.', 'type': 'number' }, + 'runnerfoo': { + 'description': 'Some foo param.', + 'default': 'FOO' + }, 'runnerdummy': { 'description': 'Dummy param.', 'type': 'string', @@ -358,6 +364,7 @@ def _setup_action_models(cls): 'actionstr': {'type': 'string', 'required': True}, 'actionint': {'type': 'number', 'default': 10}, 'runnerdummy': {'type': 'string', 'default': 'actiondummy', 'immutable': True}, + 'runnerfoo': {'type': 'string', 'immutable': True}, 'runnerimmutable': {'type': 'string', 'default': 'failed_override'}, 'actionimmutable': {'type': 'string', 'default': 'actionimmutable', 'immutable': True} } diff --git a/st2common/st2common/content/bootstrap.py b/st2common/st2common/content/bootstrap.py index a66b8b7fde..55e4f4967b 100644 --- a/st2common/st2common/content/bootstrap.py +++ b/st2common/st2common/content/bootstrap.py @@ -29,8 +29,8 @@ def register_opts(): content_opts = [ cfg.BoolOpt('all', default=False, help='Register sensors, actions and rules.'), - cfg.BoolOpt('sensors', default=True, help='Register sensors.'), - cfg.BoolOpt('actions', default=True, help='Register actions.'), + cfg.BoolOpt('sensors', default=False, help='Register sensors.'), + cfg.BoolOpt('actions', default=False, help='Register actions.'), cfg.BoolOpt('rules', default=False, help='Register rules.') ] try: diff --git a/st2common/st2common/validators/api/action.py b/st2common/st2common/validators/api/action.py index 23458b3039..96dc95cbc2 100644 --- a/st2common/st2common/validators/api/action.py +++ b/st2common/st2common/validators/api/action.py @@ -28,13 +28,7 @@ def validate_action(action_api): - runner_db = None - # Check if runner exists. - try: - runner_db = get_runnertype_by_name(action_api.runner_type) - except StackStormDBObjectNotFoundError: - msg = 'RunnerType %s is not found.' % action_api.runner_type - raise ValueValidationException(msg) + runner_db = _get_runner_model(action_api) # Check if pack is valid. if not _is_valid_pack(action_api.pack): @@ -47,6 +41,17 @@ def validate_action(action_api): _validate_parameters(action_api.parameters, runner_db.runner_parameters) +def _get_runner_model(action_api): + runner_db = None + # Check if runner exists. + try: + runner_db = get_runnertype_by_name(action_api.runner_type) + except StackStormDBObjectNotFoundError: + msg = 'RunnerType %s is not found.' % action_api.runner_type + raise ValueValidationException(msg) + return runner_db + + def _is_valid_pack(pack): base_path = cfg.CONF.content.packs_base_path return os.path.exists(os.path.join(base_path, pack, 'actions')) @@ -61,6 +66,10 @@ def _validate_parameters(action_params=None, runner_params=None): msg = 'Param %s is declared immutable in runner. ' % param + \ 'Cannot override in action.' raise ValueValidationException(msg) - if 'default' not in action_param_meta: - msg = 'Immutable param %s requires a default value.' % param - raise ValueValidationException(msg) + if 'default' not in action_param_meta and 'default' not in runner_param_meta: + msg = 'Immutable param %s requires a default value.' % param + raise ValueValidationException(msg) + else: + if 'default' not in action_param_meta: + msg = 'Immutable param %s requires a default value.' % param + raise ValueValidationException(msg) diff --git a/st2common/tests/unit/test_action_api_validator.py b/st2common/tests/unit/test_action_api_validator.py index 518e2a937b..e7a6c0acec 100644 --- a/st2common/tests/unit/test_action_api_validator.py +++ b/st2common/tests/unit/test_action_api_validator.py @@ -82,3 +82,17 @@ def test_validate_action_param_immutable(self): self.fail('Action validation should not have passed. %s' % json.dumps(action_api_dict)) except ValueValidationException as e: self.assertTrue('requires a default value.' in e.message) + + @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock( + return_value=True)) + def test_validate_action_param_immutable_no_default(self): + action_api_dict = fixture.ARTIFACTS['actions']['action-immutable-runner-param-no-default'] + action_api = ActionAPI(**action_api_dict) + + # Runner param sudo is decalred immutable in action but no defualt value + # supplied in action. We should pick up default value from runner. + try: + action_validator.validate_action(action_api) + except ValueValidationException as e: + print(e) + self.fail('Action validation should have passed. %s' % json.dumps(action_api_dict)) diff --git a/st2tests/st2tests/fixtures/generic/actions/action-invalid-schema-params.yaml b/st2tests/st2tests/fixtures/generic/actions/action-invalid-schema-params.yaml new file mode 100644 index 0000000000..2bbe3c7a9e --- /dev/null +++ b/st2tests/st2tests/fixtures/generic/actions/action-invalid-schema-params.yaml @@ -0,0 +1,32 @@ +--- + name: "st2_upgrade" + runner_type: "action-chain" + description: "Upgrades an existing st2 installation" + enabled: true + entry_point: "workflows/st2_upgrade.yaml" + parameters: + hostname: + type: "string" + description: "Host to upgrade st2 on" + required: true + action: + type: "string" + description: "Action to run after upgrade" + default: "core.local" + action_params: + type: "string" + description: "Parameters of action to be run" + default: "date" + repo: + type: "string" + # Note the quote after description. This is invalid. + description": "Git repository for this project" + build_server: + type: "string" + description: "build server" + branch: + type: "string" + description: "branch" + environment: + type: "string" + description: "environment" diff --git a/st2tests/st2tests/fixtures/wolfpack/actions/action_2_bad_json.json b/st2tests/st2tests/fixtures/generic/actions/action_2_bad_json.json similarity index 100% rename from st2tests/st2tests/fixtures/wolfpack/actions/action_2_bad_json.json rename to st2tests/st2tests/fixtures/generic/actions/action_2_bad_json.json diff --git a/st2tests/st2tests/fixtures/wolfpack/actions/action_3_pack_missing.json b/st2tests/st2tests/fixtures/generic/actions/action_3_pack_missing.json similarity index 100% rename from st2tests/st2tests/fixtures/wolfpack/actions/action_3_pack_missing.json rename to st2tests/st2tests/fixtures/generic/actions/action_3_pack_missing.json diff --git a/st2tests/st2tests/fixtures/history/actions.json b/st2tests/st2tests/fixtures/history/actions.json index 49f0e229f3..909ff354c0 100644 --- a/st2tests/st2tests/fixtures/history/actions.json +++ b/st2tests/st2tests/fixtures/history/actions.json @@ -43,5 +43,16 @@ "immutable": true } } + }, + "action-immutable-runner-param-no-default": { + "runner_type": "run-local", + "name": "action-immutable-param-no-default", + "enabled": true, + "pack": "core", + "parameters": { + "sudo": { + "immutable": true + } + } } } diff --git a/st2tests/st2tests/fixtures/history/runners.json b/st2tests/st2tests/fixtures/history/runners.json index 186045b72e..e29e71692b 100644 --- a/st2tests/st2tests/fixtures/history/runners.json +++ b/st2tests/st2tests/fixtures/history/runners.json @@ -11,6 +11,10 @@ }, "cmd": { "type": "string" + }, + "sudo": { + "type": "boolean", + "default": false } }, "runner_module": "st2actions.runners.fabricrunner" diff --git a/st2tests/st2tests/fixtures/wolfpack/actions/action_1.json b/st2tests/st2tests/fixtures/wolfpack/actions/action_1.json deleted file mode 100644 index b4053452bd..0000000000 --- a/st2tests/st2tests/fixtures/wolfpack/actions/action_1.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "name": "st2.dummy.action1", - "description": "test description", - "enabled": true, - "pack": "wolfpack", - "entry_point": "/tmp/test/action1.sh", - "runner_type": "run-local", - "parameters": { - "a": {"type": "string", "default": "A1"}, - "b": {"type": "string", "default": "B1"} - } -} diff --git a/tools/st2ctl b/tools/st2ctl index 4af533bc8a..77b1e8e983 100755 --- a/tools/st2ctl +++ b/tools/st2ctl @@ -120,7 +120,14 @@ function restart_component() { function register_content() { echo "Registering content..." - $PYTHON ${PYTHONPACK}/st2common/bin/registercontent.py --config-file ${STANCONF} ${1} + + if [ ! ${1} ]; then + REGISTER_FLAGS="--register-sensors --register-actions" + else + REGISTER_FLAGS="--register-${1}" + fi + + $PYTHON ${PYTHONPACK}/st2common/bin/registercontent.py --config-file ${STANCONF} ${REGISTER_FLAGS} } clean_db() {