You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ariatosca.apache.org by tliron <gi...@git.apache.org> on 2017/05/22 23:42:55 UTC

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

GitHub user tliron opened a pull request:

    https://github.com/apache/incubator-ariatosca/pull/138

    ARIA-149 Enhance operation configuration

    * Parse special "dependencies" configuration parameters as YAML and
      treat as Parameter models, allowing them full use of intrinsic
      functions, type coersions, and validations
    * Rename various functions that process "properties" to more generically
      process "parameters" (properties, inputs, attributes, arguments, etc.)
    * The "configuration" field in OperationTemplate and Operation models
      is now now a dict of Parameter models
    * Added "function" and "arguments" fields to Operation model to preserve
      user data (in "implementation" and "inputs") and to clearly demarcate
      orchestration data from user data
    * Some cleanup of parser code touched by this commit

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/apache/incubator-ariatosca ARIA-149-functions-in-operation-configuration

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-ariatosca/pull/138.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #138
    
----
commit 13888d24794458e818e01af75482feae6b6dded3
Author: Tal Liron <ta...@gmail.com>
Date:   2017-04-20T22:54:47Z

    ARIA-149 Enhance operation configuration
    
    * Parse special "dependencies" configuration parameters as YAML and
      treat as Parameter models, allowing them full use of intrinsic
      functions, type coersions, and validations
    * Rename various functions that process "properties" to more generically
      process "parameters" (properties, inputs, attributes, arguments, etc.)
    * The "configuration" field in OperationTemplate and Operation models
      is now now a dict of Parameter models
    * Added "function" and "arguments" fields to Operation model to preserve
      user data (in "implementation" and "inputs") and to clearly demarcate
      orchestration data from user data
    * Some cleanup of parser code touched by this commit

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118000333
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -309,7 +311,7 @@ policy_types:
           client connections cleanly and shut down services. 
         derived_from: aria.Workflow
         properties:
    -      implementation:
    +      function:
    --- End diff --
    
    i thought in the service-template even according to your view this should remain `implementation`, no?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119446252
  
    --- Diff: aria/modeling/exceptions.py ---
    @@ -57,3 +57,9 @@ class UndeclaredParametersException(ParameterException):
         """
         ARIA modeling exception: Undeclared parameters have been provided.
         """
    +
    +
    +class ForbiddenParameterNamesException(ParameterException):
    --- End diff --
    
    Changing to "Reserved".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119443917
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -16,107 +16,132 @@
     # TODO: this module will eventually be moved to a new "aria.instantiation" package
     
     from ...utils.type import full_type_name
    -from ...utils.collections import OrderedDict
    +from ...utils.formatting import safe_repr
     from ...parser import validation
     from ...parser.consumption import ConsumptionContext
    +from ...modeling.functions import Function
     
     
     def configure_operation(operation):
    -    configuration = OrderedDict(operation.configuration) if operation.configuration else {}
    -
    -    arguments = OrderedDict()
    -    arguments['script_path'] = operation.implementation
    -    arguments['process'] = _get_process(configuration.pop('process')) \
    -        if 'process' in configuration else dict()
    -
         host = None
         interface = operation.interface
         if interface.node is not None:
             host = interface.node.host
         elif interface.relationship is not None:
             if operation.relationship_edge is True:
                 host = interface.relationship.target_node.host
    -        else: # either False or None
    +        else: # either False or None (None meaning that edge was not specified)
                 host = interface.relationship.source_node.host
     
    +    _configure_common(operation)
         if host is None:
             _configure_local(operation)
         else:
    -        _configure_remote(operation, configuration, arguments)
    +        _configure_remote(operation)
    +
    +    # Any remaining un-handled configuration parameters will become extra arguments, available as
    +    # kwargs in either "run_script_locally" or "run_script_with_ssh"
    +    for key, value in operation.configuration.iteritems():
    +        if key not in ('process', 'ssh'):
    +            operation.arguments[key] = value.instantiate()
     
    -    # Any remaining unhandled configuration values will become extra arguments, available as kwargs
    -    # in either "run_script_locally" or "run_script_with_ssh"
    -    arguments.update(configuration)
     
    -    return arguments
    +def _configure_common(operation):
    +    """
    +    Local and remote operations.
    +    """
    +
    +    from ...modeling.models import Parameter
    +    operation.arguments['script_path'] = Parameter.wrap('script_path', operation.implementation,
    +                                                        'Relative path to the executable file.')
    +    operation.arguments['process'] = Parameter.wrap('process', _get_process(operation),
    +                                                    'Sub-process configuration.')
    +
     
     def _configure_local(operation):
         """
         Local operation.
         """
    +
         from . import operations
    -    operation.implementation = '{0}.{1}'.format(operations.__name__,
    -                                                operations.run_script_locally.__name__)
    +    operation.function = '{0}.{1}'.format(operations.__name__,
    +                                          operations.run_script_locally.__name__)
     
     
    -def _configure_remote(operation, configuration, arguments):
    +def _configure_remote(operation):
         """
         Remote SSH operation via Fabric.
         """
    +
    +    from ...modeling.models import Parameter
    +    from . import operations
    +
    +    ssh = _get_ssh(operation)
    +
    +    # Defaults
         # TODO: find a way to configure these generally in the service template
         default_user = ''
         default_password = ''
    -
    -    ssh = _get_ssh(configuration.pop('ssh')) if 'ssh' in configuration else {}
         if 'user' not in ssh:
             ssh['user'] = default_user
         if ('password' not in ssh) and ('key' not in ssh) and ('key_filename' not in ssh):
             ssh['password'] = default_password
     
    -    arguments['use_sudo'] = ssh.get('use_sudo', False)
    -    arguments['hide_output'] = ssh.get('hide_output', [])
    -    arguments['fabric_env'] = {}
    +    operation.arguments['use_sudo'] = Parameter.wrap('use_sudo', ssh.get('use_sudo', False),
    --- End diff --
    
    It's true that configuration parameters are wrapped in another place when it's not intended for the execution plugin, but my question is why bother wrapping them at this stage at all - as opposed to working with dicts at this stage and then have the instantiation code which calls this module do the wrapping. it just makes more sense IMO.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119445955
  
    --- Diff: aria/orchestrator/workflow_runner.py ---
    @@ -136,10 +137,11 @@ def _validate_no_active_executions(self, execution):
             active_executions = [e for e in self.service.executions if e.is_active()]
             if active_executions:
                 raise exceptions.ActiveExecutionsError(
    -                "Can't start execution; Service {0} has an active execution with id {1}"
    +                "Can't start execution; Service {0} has an active execution with ID {1}"
                     .format(self.service.name, active_executions[0].id))
     
    -    def _get_workflow_fn(self):
    +    @property
    +    def _workflow_fn(self):
    --- End diff --
    
    Changing back to function.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119442939
  
    --- Diff: aria/modeling/utils.py ---
    @@ -52,84 +53,95 @@ def service_template(self):
             return self.container.service_template
     
     
    -def create_parameters(parameters, declared_parameters):
    +def merge_parameter_values(parameter_values, declared_parameters, forbidden_names=None):
    --- End diff --
    
    i thought we talked about the concept of validating "forbidden names" and we agreed it should be done in the parser / instantiation phases instead. I don't see in what scenario we'd like to inform the user of such validation errors only at this stage.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118001512
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/parameters.py ---
    @@ -71,6 +72,7 @@ def get_assigned_and_defined_property_values(context, presentation, field_name='
         values = OrderedDict()
     
         the_type = presentation._get_type(context)
    +    field_name_plural = pluralize(field_name)
    --- End diff --
    
    how did the following lines worked before this? o_O


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-ariatosca/pull/138


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118007646
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -384,18 +387,37 @@ def create_operation_template_model(context, service_template, operation):
                     model.relationship_edge = True
     
             dependencies = implementation.dependencies
    +        configuration = OrderedDict()
             if dependencies:
                 for dependency in dependencies:
                     key, value = split_prefix(dependency)
                     if key is not None:
    -                    if model.configuration is None:
    -                        model.configuration = {}
    -                    set_nested(model.configuration, key.split('.'), value)
    +                    # Parse as YAML
    +                    try:
    +                        value = yaml.load(value)
    +                    except yaml.parser.MarkedYAMLError as e:
    +                        context.validation.report(
    +                            'YAML parser {0} in operation configuration: {1}'
    +                            .format(e.problem, value),
    +                            locator=implementation._locator,
    +                            level=Issue.FIELD)
    +                        continue
    --- End diff --
    
    shouldn't this be a re-raise? seems somewhat fatal...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron closed the pull request at:

    https://github.com/apache/incubator-ariatosca/pull/138


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118011107
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1728,34 +1732,41 @@ def operation_template(cls):
         def inputs(cls):
             return relationship.many_to_many(cls, 'parameter', prefix='inputs', dict_key='name')
     
    +    @declared_attr
    +    def configuration(cls):
    +        return relationship.many_to_many(cls, 'parameter', prefix='configuration', dict_key='name')
    +
    +    @declared_attr
    +    def arguments(cls):
    +        return relationship.many_to_many(cls, 'parameter', prefix='arguments', dict_key='name')
    +
         # endregion
     
         description = Column(Text)
         relationship_edge = Column(Boolean)
         implementation = Column(Text)
    -    configuration = Column(modeling_types.StrictDict(key_cls=basestring))
         dependencies = Column(modeling_types.StrictList(item_cls=basestring))
    +    function = Column(Text)
         executor = Column(Text)
         max_attempts = Column(Integer)
         retry_interval = Column(Integer)
     
         def configure(self):
    -        from . import models
    -        # Note: for workflows (operations attached directly to the service) "interface" will be None
    -        if (self.implementation is None) or (self.interface is None):
    +        if (self.implementation is None) and (self.function is None):
                 return
     
    -        if self.plugin is None:
    -            arguments = execution_plugin.instantiation.configure_operation(self)
    +        if (self.plugin is None) and (self.interface is not None):
    +            # Default to execution plugin ("interface" is None for workflow operations)
    +            execution_plugin.instantiation.configure_operation(self)
             else:
                 # In the future plugins may be able to add their own "configure_operation" hook that
    -            # can validate the configuration and otherwise return specially derived arguments
    -            arguments = self.configuration
    +            # can validate the configuration and otherwise create specially derived arguments. For
    +            # now, we just send all configuration parameters as arguments
    +            utils.instantiate_dict(self, self.arguments, self.configuration)
     
    -        # Note: the arguments will *override* operation inputs of the same name
    -        if arguments:
    -            for k, v in arguments.iteritems():
    -                self.inputs[k] = models.Parameter.wrap(k, v)
    --- End diff --
    
    why remove this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118375044
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/parameters.py ---
    @@ -71,6 +72,7 @@ def get_assigned_and_defined_property_values(context, presentation, field_name='
         values = OrderedDict()
     
         the_type = presentation._get_type(context)
    +    field_name_plural = pluralize(field_name)
    --- End diff --
    
    Previously we sent the plural as a function argument, but I optimized by removing it... just some code cleanup.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119449695
  
    --- Diff: aria/orchestrator/workflows/events_logging.py ---
    @@ -35,20 +35,20 @@ def _get_task_name(task):
     
     @events.start_task_signal.connect
     def _start_task_handler(task, **kwargs):
    -    # If the task has not implementation this is an empty task.
    -    if task.implementation:
    +    # If the task has no function this is an empty task.
    +    if task.function:
             suffix = 'started...'
             logger = task.context.logger.info
         else:
    -        suffix = 'has no implementation'
    +        suffix = 'has no function'
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118375175
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -384,18 +387,37 @@ def create_operation_template_model(context, service_template, operation):
                     model.relationship_edge = True
     
             dependencies = implementation.dependencies
    +        configuration = OrderedDict()
             if dependencies:
                 for dependency in dependencies:
                     key, value = split_prefix(dependency)
                     if key is not None:
    -                    if model.configuration is None:
    -                        model.configuration = {}
    -                    set_nested(model.configuration, key.split('.'), value)
    +                    # Parse as YAML
    +                    try:
    +                        value = yaml.load(value)
    +                    except yaml.parser.MarkedYAMLError as e:
    +                        context.validation.report(
    +                            'YAML parser {0} in operation configuration: {1}'
    +                            .format(e.problem, value),
    +                            locator=implementation._locator,
    +                            level=Issue.FIELD)
    +                        continue
    --- End diff --
    
    Validation issues accumulate and are displayed in the end. None are considered fatal enough to start the parsing, but of course a single error means that parsing failed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119446847
  
    --- Diff: tests/orchestrator/test_workflow_runner.py ---
    @@ -259,8 +258,9 @@ def _setup_mock_workflow_in_service(request, inputs=None):
         workflow = models.Operation(
             name=mock_workflow_name,
             service=service,
    -        implementation='workflow.mock_workflow',
    -        inputs=inputs or {})
    +        function='workflow.mock_workflow',
    +        inputs=inputs or {},
    +        arguments=inputs or {})
    --- End diff --
    
    No, not in the way this particular test works, since it creates an operation model directly and doesn't call `configure()`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119444625
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -309,7 +311,7 @@ policy_types:
           client connections cleanly and shut down services. 
         derived_from: aria.Workflow
         properties:
    -      implementation:
    +      function:
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119445999
  
    --- Diff: aria/orchestrator/workflows/events_logging.py ---
    @@ -35,20 +35,20 @@ def _get_task_name(task):
     
     @events.start_task_signal.connect
     def _start_task_handler(task, **kwargs):
    -    # If the task has not implementation this is an empty task.
    -    if task.implementation:
    +    # If the task has no function this is an empty task.
    +    if task.function:
             suffix = 'started...'
             logger = task.context.logger.info
         else:
    -        suffix = 'has no implementation'
    +        suffix = 'has no function'
    --- End diff --
    
    Changing back to "implementation".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119449922
  
    --- Diff: aria/modeling/utils.py ---
    @@ -51,74 +53,95 @@ def service_template(self):
             return self.container.service_template
     
     
    -def create_inputs(inputs, template_inputs):
    +def merge_parameter_values(parameter_values, declared_parameters, forbidden_names=None):
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118375237
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -16,107 +16,132 @@
     # TODO: this module will eventually be moved to a new "aria.instantiation" package
     
     from ...utils.type import full_type_name
    -from ...utils.collections import OrderedDict
    +from ...utils.formatting import safe_repr
     from ...parser import validation
     from ...parser.consumption import ConsumptionContext
    +from ...modeling.functions import Function
     
     
     def configure_operation(operation):
    -    configuration = OrderedDict(operation.configuration) if operation.configuration else {}
    -
    -    arguments = OrderedDict()
    -    arguments['script_path'] = operation.implementation
    -    arguments['process'] = _get_process(configuration.pop('process')) \
    -        if 'process' in configuration else dict()
    -
         host = None
         interface = operation.interface
         if interface.node is not None:
             host = interface.node.host
         elif interface.relationship is not None:
             if operation.relationship_edge is True:
                 host = interface.relationship.target_node.host
    -        else: # either False or None
    +        else: # either False or None (None meaning that edge was not specified)
                 host = interface.relationship.source_node.host
     
    +    _configure_common(operation)
         if host is None:
             _configure_local(operation)
         else:
    -        _configure_remote(operation, configuration, arguments)
    +        _configure_remote(operation)
    +
    +    # Any remaining un-handled configuration parameters will become extra arguments, available as
    +    # kwargs in either "run_script_locally" or "run_script_with_ssh"
    +    for key, value in operation.configuration.iteritems():
    +        if key not in ('process', 'ssh'):
    +            operation.arguments[key] = value.instantiate()
     
    -    # Any remaining unhandled configuration values will become extra arguments, available as kwargs
    -    # in either "run_script_locally" or "run_script_with_ssh"
    -    arguments.update(configuration)
     
    -    return arguments
    +def _configure_common(operation):
    +    """
    +    Local and remote operations.
    +    """
    +
    +    from ...modeling.models import Parameter
    +    operation.arguments['script_path'] = Parameter.wrap('script_path', operation.implementation,
    +                                                        'Relative path to the executable file.')
    +    operation.arguments['process'] = Parameter.wrap('process', _get_process(operation),
    +                                                    'Sub-process configuration.')
    +
     
     def _configure_local(operation):
         """
         Local operation.
         """
    +
         from . import operations
    -    operation.implementation = '{0}.{1}'.format(operations.__name__,
    -                                                operations.run_script_locally.__name__)
    +    operation.function = '{0}.{1}'.format(operations.__name__,
    +                                          operations.run_script_locally.__name__)
     
     
    -def _configure_remote(operation, configuration, arguments):
    +def _configure_remote(operation):
         """
         Remote SSH operation via Fabric.
         """
    +
    +    from ...modeling.models import Parameter
    +    from . import operations
    +
    +    ssh = _get_ssh(operation)
    +
    +    # Defaults
         # TODO: find a way to configure these generally in the service template
         default_user = ''
         default_password = ''
    -
    -    ssh = _get_ssh(configuration.pop('ssh')) if 'ssh' in configuration else {}
         if 'user' not in ssh:
             ssh['user'] = default_user
         if ('password' not in ssh) and ('key' not in ssh) and ('key_filename' not in ssh):
             ssh['password'] = default_password
     
    -    arguments['use_sudo'] = ssh.get('use_sudo', False)
    -    arguments['hide_output'] = ssh.get('hide_output', [])
    -    arguments['fabric_env'] = {}
    +    operation.arguments['use_sudo'] = Parameter.wrap('use_sudo', ssh.get('use_sudo', False),
    --- End diff --
    
    We went through the code together, this happens elsewhere.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119443130
  
    --- Diff: aria/modeling/exceptions.py ---
    @@ -57,3 +57,9 @@ class UndeclaredParametersException(ParameterException):
         """
         ARIA modeling exception: Undeclared parameters have been provided.
         """
    +
    +
    +class ForbiddenParameterNamesException(ParameterException):
    --- End diff --
    
    perhaps "Reserved" is more appropriate than "Forbidden"?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119447056
  
    --- Diff: aria/modeling/utils.py ---
    @@ -51,74 +53,95 @@ def service_template(self):
             return self.container.service_template
     
     
    -def create_inputs(inputs, template_inputs):
    +def merge_parameter_values(parameter_values, declared_parameters, forbidden_names=None):
    --- End diff --
    
    The original code was superfluous and complex, doing unpacking and re-packing of values, and actually had a bug in one area of it. I think this code is much cleaner!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119444005
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/parameters.py ---
    @@ -71,6 +72,7 @@ def get_assigned_and_defined_property_values(context, presentation, field_name='
         values = OrderedDict()
     
         the_type = presentation._get_type(context)
    +    field_name_plural = pluralize(field_name)
    --- End diff --
    
    :+1: 



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119449714
  
    --- Diff: aria/orchestrator/workflow_runner.py ---
    @@ -136,10 +137,11 @@ def _validate_no_active_executions(self, execution):
             active_executions = [e for e in self.service.executions if e.is_active()]
             if active_executions:
                 raise exceptions.ActiveExecutionsError(
    -                "Can't start execution; Service {0} has an active execution with id {1}"
    +                "Can't start execution; Service {0} has an active execution with ID {1}"
                     .format(self.service.name, active_executions[0].id))
     
    -    def _get_workflow_fn(self):
    +    @property
    +    def _workflow_fn(self):
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119445992
  
    --- Diff: aria/modeling/utils.py ---
    @@ -51,74 +53,95 @@ def service_template(self):
             return self.container.service_template
     
     
    -def create_inputs(inputs, template_inputs):
    +def merge_parameter_values(parameter_values, declared_parameters, forbidden_names=None):
    --- End diff --
    
    what was the purpose of the changes to this method? im not sure i understand the benefit and it seems less readable now to me


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119449568
  
    --- Diff: aria/modeling/exceptions.py ---
    @@ -57,3 +57,9 @@ class UndeclaredParametersException(ParameterException):
         """
         ARIA modeling exception: Undeclared parameters have been provided.
         """
    +
    +
    +class ForbiddenParameterNamesException(ParameterException):
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119427199
  
    --- Diff: aria/orchestrator/workflow_runner.py ---
    @@ -136,10 +137,11 @@ def _validate_no_active_executions(self, execution):
             active_executions = [e for e in self.service.executions if e.is_active()]
             if active_executions:
                 raise exceptions.ActiveExecutionsError(
    -                "Can't start execution; Service {0} has an active execution with id {1}"
    +                "Can't start execution; Service {0} has an active execution with ID {1}"
                     .format(self.service.name, active_executions[0].id))
     
    -    def _get_workflow_fn(self):
    +    @property
    +    def _workflow_fn(self):
    --- End diff --
    
    what's a `property` about this? it's a function with possible side effects, its private, and it isn't really meant to be used repeatedly, so i think it actually stands better as a function.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119449679
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/functions.py ---
    @@ -69,7 +69,7 @@ def __evaluate__(self, container_holder):
                 e, final = evaluate(e, final, container_holder)
                 if e is not None:
                     value.write(unicode(e))
    -        value = value.getvalue()
    +        value = value.getvalue() or u''
    --- End diff --
    
    :+1:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119449551
  
    --- Diff: tests/orchestrator/test_workflow_runner.py ---
    @@ -259,8 +258,9 @@ def _setup_mock_workflow_in_service(request, inputs=None):
         workflow = models.Operation(
             name=mock_workflow_name,
             service=service,
    -        implementation='workflow.mock_workflow',
    -        inputs=inputs or {})
    +        function='workflow.mock_workflow',
    +        inputs=inputs or {},
    +        arguments=inputs or {})
    --- End diff --
    
    right :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118374966
  
    --- Diff: aria/modeling/exceptions.py ---
    @@ -22,9 +22,9 @@ class ModelingException(AriaException):
         """
     
     
    -class InputsException(ModelingException):
    +class ParameterException(ModelingException):
    --- End diff --
    
    Discussed and we agreed that more general is better in this case.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r117999899
  
    --- Diff: aria/modeling/exceptions.py ---
    @@ -22,9 +22,9 @@ class ModelingException(AriaException):
         """
     
     
    -class InputsException(ModelingException):
    +class ParameterException(ModelingException):
    --- End diff --
    
    I'm not sure I understand this change - It's true that the model object is `Parameter` and not `Inputs` (although that too is supposedly under change by @AviaE ) - but the error itself is not about the model type and doesn't have anything much to do with the models themselves - it's about inputs, so why shouldn't the exception be named like so?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
GitHub user tliron reopened a pull request:

    https://github.com/apache/incubator-ariatosca/pull/138

    ARIA-149 Enhance operation configuration

    * Parse special "dependencies" configuration parameters as YAML and
      treat as Parameter models, allowing them full use of intrinsic
      functions, type coersions, and validations
    * Rename various functions that process "properties" to more generically
      process "parameters" (properties, inputs, attributes, arguments, etc.)
    * The "configuration" field in OperationTemplate and Operation models
      is now now a dict of Parameter models
    * Added "function" and "arguments" fields to Operation model to preserve
      user data (in "implementation" and "inputs") and to clearly demarcate
      orchestration data from user data
    * Some cleanup of parser code touched by this commit

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/apache/incubator-ariatosca ARIA-149-functions-in-operation-configuration

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-ariatosca/pull/138.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #138
    
----
commit f6ee65a9eaa8eb252c4431152327635a43dff425
Author: Tal Liron <ta...@gmail.com>
Date:   2017-04-20T22:54:47Z

    ARIA-149 Enhance operation configuration
    
    * Parse special "dependencies" configuration parameters as YAML and
      treat as Parameter models, allowing them full use of intrinsic
      functions, type coersions, and validations
    * Rename various functions that process "properties" to more generically
      process "parameters" (properties, inputs, attributes, arguments, etc.)
    * The "configuration" field in OperationTemplate and Operation models
      is now now a dict of Parameter models
    * Added "function" and "arguments" fields to Operation model to preserve
      user data (in "implementation" and "inputs") and to clearly demarcate
      orchestration data from user data
    * Some cleanup of parser code touched by this commit

commit 888c5cd6f86a12e096a8ce040aedd0f62c5eac0e
Author: Tal Liron <ta...@gmail.com>
Date:   2017-05-24T19:54:07Z

    Fixes
    
    * Rename implementation/inputs to function/arguments in Task API
    * Rename "create_parameters" to "merge_parameter_values" and improve
    * Change workflow "function" back to "implementation"

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119446194
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/functions.py ---
    @@ -69,7 +69,7 @@ def __evaluate__(self, container_holder):
                 e, final = evaluate(e, final, container_holder)
                 if e is not None:
                     value.write(unicode(e))
    -        value = value.getvalue()
    +        value = value.getvalue() or u''
    --- End diff --
    
    We default to Unicode everywhere possible in values -- this avoids thorny problems later down the road with values.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119428083
  
    --- Diff: aria/orchestrator/workflows/events_logging.py ---
    @@ -35,20 +35,20 @@ def _get_task_name(task):
     
     @events.start_task_signal.connect
     def _start_task_handler(task, **kwargs):
    -    # If the task has not implementation this is an empty task.
    -    if task.implementation:
    +    # If the task has no function this is an empty task.
    +    if task.function:
             suffix = 'started...'
             logger = task.context.logger.info
         else:
    -        suffix = 'has no implementation'
    +        suffix = 'has no function'
    --- End diff --
    
    actually, in this specific instance, it should probably remain `implementation` - function is an implementation (sorry for the confusing term in this context :D) detail, but in practice what the message means is that the user didn't input (there i go with the terms again.. :D) any implementation for the operation (AKA empty operation)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118293230
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1728,34 +1732,41 @@ def operation_template(cls):
         def inputs(cls):
             return relationship.many_to_many(cls, 'parameter', prefix='inputs', dict_key='name')
     
    +    @declared_attr
    +    def configuration(cls):
    +        return relationship.many_to_many(cls, 'parameter', prefix='configuration', dict_key='name')
    +
    +    @declared_attr
    +    def arguments(cls):
    +        return relationship.many_to_many(cls, 'parameter', prefix='arguments', dict_key='name')
    +
         # endregion
     
         description = Column(Text)
         relationship_edge = Column(Boolean)
         implementation = Column(Text)
    -    configuration = Column(modeling_types.StrictDict(key_cls=basestring))
         dependencies = Column(modeling_types.StrictList(item_cls=basestring))
    +    function = Column(Text)
         executor = Column(Text)
         max_attempts = Column(Integer)
         retry_interval = Column(Integer)
     
         def configure(self):
    -        from . import models
    -        # Note: for workflows (operations attached directly to the service) "interface" will be None
    -        if (self.implementation is None) or (self.interface is None):
    +        if (self.implementation is None) and (self.function is None):
                 return
     
    -        if self.plugin is None:
    -            arguments = execution_plugin.instantiation.configure_operation(self)
    +        if (self.plugin is None) and (self.interface is not None):
    +            # Default to execution plugin ("interface" is None for workflow operations)
    +            execution_plugin.instantiation.configure_operation(self)
             else:
                 # In the future plugins may be able to add their own "configure_operation" hook that
    -            # can validate the configuration and otherwise return specially derived arguments
    -            arguments = self.configuration
    +            # can validate the configuration and otherwise create specially derived arguments. For
    +            # now, we just send all configuration parameters as arguments
    +            utils.instantiate_dict(self, self.arguments, self.configuration)
     
    -        # Note: the arguments will *override* operation inputs of the same name
    -        if arguments:
    -            for k, v in arguments.iteritems():
    -                self.inputs[k] = models.Parameter.wrap(k, v)
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119445302
  
    --- Diff: tests/orchestrator/test_workflow_runner.py ---
    @@ -259,8 +258,9 @@ def _setup_mock_workflow_in_service(request, inputs=None):
         workflow = models.Operation(
             name=mock_workflow_name,
             service=service,
    -        implementation='workflow.mock_workflow',
    -        inputs=inputs or {})
    +        function='workflow.mock_workflow',
    +        inputs=inputs or {},
    +        arguments=inputs or {})
    --- End diff --
    
    why is this needed? wouldn't arguments get auto-populated with inputs anyway?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119449742
  
    --- Diff: aria/modeling/utils.py ---
    @@ -52,84 +53,95 @@ def service_template(self):
             return self.container.service_template
     
     
    -def create_parameters(parameters, declared_parameters):
    +def merge_parameter_values(parameter_values, declared_parameters, forbidden_names=None):
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119444608
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -384,18 +387,37 @@ def create_operation_template_model(context, service_template, operation):
                     model.relationship_edge = True
     
             dependencies = implementation.dependencies
    +        configuration = OrderedDict()
             if dependencies:
                 for dependency in dependencies:
                     key, value = split_prefix(dependency)
                     if key is not None:
    -                    if model.configuration is None:
    -                        model.configuration = {}
    -                    set_nested(model.configuration, key.split('.'), value)
    +                    # Parse as YAML
    +                    try:
    +                        value = yaml.load(value)
    +                    except yaml.parser.MarkedYAMLError as e:
    +                        context.validation.report(
    +                            'YAML parser {0} in operation configuration: {1}'
    +                            .format(e.problem, value),
    +                            locator=implementation._locator,
    +                            level=Issue.FIELD)
    +                        continue
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119428842
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/functions.py ---
    @@ -69,7 +69,7 @@ def __evaluate__(self, container_holder):
                 e, final = evaluate(e, final, container_holder)
                 if e is not None:
                     value.write(unicode(e))
    -        value = value.getvalue()
    +        value = value.getvalue() or u''
    --- End diff --
    
    why do the value we return here need to be unicode again?
    i understand the need to support unicode values which are set by the user, but why does it matter whether the value returned in case there never was a value is unicode or not?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118374974
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -309,7 +311,7 @@ policy_types:
           client connections cleanly and shut down services. 
         derived_from: aria.Workflow
         properties:
    -      implementation:
    +      function:
    --- End diff --
    
    Correct, changed back.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by tliron <gi...@git.apache.org>.
Github user tliron commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119445875
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -16,107 +16,132 @@
     # TODO: this module will eventually be moved to a new "aria.instantiation" package
     
     from ...utils.type import full_type_name
    -from ...utils.collections import OrderedDict
    +from ...utils.formatting import safe_repr
     from ...parser import validation
     from ...parser.consumption import ConsumptionContext
    +from ...modeling.functions import Function
     
     
     def configure_operation(operation):
    -    configuration = OrderedDict(operation.configuration) if operation.configuration else {}
    -
    -    arguments = OrderedDict()
    -    arguments['script_path'] = operation.implementation
    -    arguments['process'] = _get_process(configuration.pop('process')) \
    -        if 'process' in configuration else dict()
    -
         host = None
         interface = operation.interface
         if interface.node is not None:
             host = interface.node.host
         elif interface.relationship is not None:
             if operation.relationship_edge is True:
                 host = interface.relationship.target_node.host
    -        else: # either False or None
    +        else: # either False or None (None meaning that edge was not specified)
                 host = interface.relationship.source_node.host
     
    +    _configure_common(operation)
         if host is None:
             _configure_local(operation)
         else:
    -        _configure_remote(operation, configuration, arguments)
    +        _configure_remote(operation)
    +
    +    # Any remaining un-handled configuration parameters will become extra arguments, available as
    +    # kwargs in either "run_script_locally" or "run_script_with_ssh"
    +    for key, value in operation.configuration.iteritems():
    +        if key not in ('process', 'ssh'):
    +            operation.arguments[key] = value.instantiate()
     
    -    # Any remaining unhandled configuration values will become extra arguments, available as kwargs
    -    # in either "run_script_locally" or "run_script_with_ssh"
    -    arguments.update(configuration)
     
    -    return arguments
    +def _configure_common(operation):
    +    """
    +    Local and remote operations.
    +    """
    +
    +    from ...modeling.models import Parameter
    +    operation.arguments['script_path'] = Parameter.wrap('script_path', operation.implementation,
    +                                                        'Relative path to the executable file.')
    +    operation.arguments['process'] = Parameter.wrap('process', _get_process(operation),
    +                                                    'Sub-process configuration.')
    +
     
     def _configure_local(operation):
         """
         Local operation.
         """
    +
         from . import operations
    -    operation.implementation = '{0}.{1}'.format(operations.__name__,
    -                                                operations.run_script_locally.__name__)
    +    operation.function = '{0}.{1}'.format(operations.__name__,
    +                                          operations.run_script_locally.__name__)
     
     
    -def _configure_remote(operation, configuration, arguments):
    +def _configure_remote(operation):
         """
         Remote SSH operation via Fabric.
         """
    +
    +    from ...modeling.models import Parameter
    +    from . import operations
    +
    +    ssh = _get_ssh(operation)
    +
    +    # Defaults
         # TODO: find a way to configure these generally in the service template
         default_user = ''
         default_password = ''
    -
    -    ssh = _get_ssh(configuration.pop('ssh')) if 'ssh' in configuration else {}
         if 'user' not in ssh:
             ssh['user'] = default_user
         if ('password' not in ssh) and ('key' not in ssh) and ('key_filename' not in ssh):
             ssh['password'] = default_password
     
    -    arguments['use_sudo'] = ssh.get('use_sudo', False)
    -    arguments['hide_output'] = ssh.get('hide_output', [])
    -    arguments['fabric_env'] = {}
    +    operation.arguments['use_sudo'] = Parameter.wrap('use_sudo', ssh.get('use_sudo', False),
    --- End diff --
    
    If you notice I am also giving the parameters descriptions here. These are not mere dicts, but true parameters.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r119443983
  
    --- Diff: aria/modeling/exceptions.py ---
    @@ -22,9 +22,9 @@ class ModelingException(AriaException):
         """
     
     
    -class InputsException(ModelingException):
    +class ParameterException(ModelingException):
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-ariatosca pull request #138: ARIA-149 Enhance operation configurat...

Posted by ran-z <gi...@git.apache.org>.
Github user ran-z commented on a diff in the pull request:

    https://github.com/apache/incubator-ariatosca/pull/138#discussion_r118009534
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -16,107 +16,132 @@
     # TODO: this module will eventually be moved to a new "aria.instantiation" package
     
     from ...utils.type import full_type_name
    -from ...utils.collections import OrderedDict
    +from ...utils.formatting import safe_repr
     from ...parser import validation
     from ...parser.consumption import ConsumptionContext
    +from ...modeling.functions import Function
     
     
     def configure_operation(operation):
    -    configuration = OrderedDict(operation.configuration) if operation.configuration else {}
    -
    -    arguments = OrderedDict()
    -    arguments['script_path'] = operation.implementation
    -    arguments['process'] = _get_process(configuration.pop('process')) \
    -        if 'process' in configuration else dict()
    -
         host = None
         interface = operation.interface
         if interface.node is not None:
             host = interface.node.host
         elif interface.relationship is not None:
             if operation.relationship_edge is True:
                 host = interface.relationship.target_node.host
    -        else: # either False or None
    +        else: # either False or None (None meaning that edge was not specified)
                 host = interface.relationship.source_node.host
     
    +    _configure_common(operation)
         if host is None:
             _configure_local(operation)
         else:
    -        _configure_remote(operation, configuration, arguments)
    +        _configure_remote(operation)
    +
    +    # Any remaining un-handled configuration parameters will become extra arguments, available as
    +    # kwargs in either "run_script_locally" or "run_script_with_ssh"
    +    for key, value in operation.configuration.iteritems():
    +        if key not in ('process', 'ssh'):
    +            operation.arguments[key] = value.instantiate()
     
    -    # Any remaining unhandled configuration values will become extra arguments, available as kwargs
    -    # in either "run_script_locally" or "run_script_with_ssh"
    -    arguments.update(configuration)
     
    -    return arguments
    +def _configure_common(operation):
    +    """
    +    Local and remote operations.
    +    """
    +
    +    from ...modeling.models import Parameter
    +    operation.arguments['script_path'] = Parameter.wrap('script_path', operation.implementation,
    +                                                        'Relative path to the executable file.')
    +    operation.arguments['process'] = Parameter.wrap('process', _get_process(operation),
    +                                                    'Sub-process configuration.')
    +
     
     def _configure_local(operation):
         """
         Local operation.
         """
    +
         from . import operations
    -    operation.implementation = '{0}.{1}'.format(operations.__name__,
    -                                                operations.run_script_locally.__name__)
    +    operation.function = '{0}.{1}'.format(operations.__name__,
    +                                          operations.run_script_locally.__name__)
     
     
    -def _configure_remote(operation, configuration, arguments):
    +def _configure_remote(operation):
         """
         Remote SSH operation via Fabric.
         """
    +
    +    from ...modeling.models import Parameter
    +    from . import operations
    +
    +    ssh = _get_ssh(operation)
    +
    +    # Defaults
         # TODO: find a way to configure these generally in the service template
         default_user = ''
         default_password = ''
    -
    -    ssh = _get_ssh(configuration.pop('ssh')) if 'ssh' in configuration else {}
         if 'user' not in ssh:
             ssh['user'] = default_user
         if ('password' not in ssh) and ('key' not in ssh) and ('key_filename' not in ssh):
             ssh['password'] = default_password
     
    -    arguments['use_sudo'] = ssh.get('use_sudo', False)
    -    arguments['hide_output'] = ssh.get('hide_output', [])
    -    arguments['fabric_env'] = {}
    +    operation.arguments['use_sudo'] = Parameter.wrap('use_sudo', ssh.get('use_sudo', False),
    --- End diff --
    
    shouldn't the wrapping of configuration parameters simply take place where this module is called from?
    what about configuration parameters not meant for the execution plugin?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---