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

[GitHub] incubator-ariatosca pull request #95: ARIA-92 Automatic operation task confi...

GitHub user tliron opened a pull request:

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

    ARIA-92 Automatic operation task configuration

    Main changes:
    
    1. Removed `runs_on` field from task model and API
    2. Two new instantiations phase, one to find the host nodes, and the second to configure operation instances for the execution plugin
    3. Parser fills in `configuration` field in `OperationTemplate` using `dependencies` in TOSCA syntax
    4. Fixes to reqs-and-caps bugs

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

    $ git pull https://github.com/apache/incubator-ariatosca ARIA-92-plugin-in-implementation-string

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

    https://github.com/apache/incubator-ariatosca/pull/95.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 #95
    
----
commit 73c1d0297812a421eee2176fcff5e5ae73980ccb
Author: Tal Liron <ta...@gmail.com>
Date:   2017-03-24T21:33:11Z

    ARIA-92 Automatic operation task configuration

----


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111189712
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    --- End diff --
    
    why not use copy of some sort?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109946010
  
    --- Diff: aria/orchestrator/workflows/api/task.py ---
    @@ -65,86 +65,82 @@ def __init__(self,
                      actor,
                      actor_type,
                      interface_name,
    -                 operation_name,
    -                 runs_on=None,
    +                 operation_name, # remove configuration
    +                 inputs=None,
                      max_attempts=None,
                      retry_interval=None,
    -                 ignore_failure=None,
    -                 inputs=None):
    +                 ignore_failure=None):
             """
             Do not call this constructor directly. Instead, use :meth:`for_node` or
             :meth:`for_relationship`.
             """
     
    -        assert isinstance(actor, (models.Node, models.Relationship))
    -        assert actor_type in ('node', 'relationship')
             assert interface_name and operation_name
    -        assert runs_on in models.Task.RUNS_ON
             super(OperationTask, self).__init__()
     
             self.actor = actor
    +        self.actor_type = actor_type
    --- End diff --
    
    Because we decided together that the logic that was here should not be in operation creation, but in service instance creation.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111417664
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    +    arguments['hide_output'] = ssh.get('hide_output')
    +    arguments['fabric_env'] = {}
    +    if 'warn_only' in ssh:
    +        arguments['fabric_env']['warn_only'] = ssh['warn_only']
    +    arguments['fabric_env']['user'] = ssh.get('user')
    +    arguments['fabric_env']['password'] = ssh.get('password')
    +    arguments['fabric_env']['key'] = ssh.get('key')
    +    arguments['fabric_env']['key_filename'] = ssh.get('key_filename')
    +    if 'address' in ssh:
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111150551
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    --- End diff --
    
    im still a bit confused by this myself - caching the host is definitely a good idea, but it looks here like the internal method _find_host is called recursively repeatedly.
    I'm not sure this is exactly the same as what Maxim meant, but I think I'd expect each node's host finding method to run once at most, so that would mean either "host" is a self-initializing property, or a field initialized at its constructor, or you could use the "find_host" method, but basically what i'm missing here is the check for whether the value has been cached already or not.
    
    i'd expect it to be more like:
    ```
    def find_host(self):
        def _find_host(node):
        ....
            host = the_relationship.target_node.find_host()
        ....
    
    if not self.host:  # or indeed use some extra flag
        self.host = _find_host(node)    
    return self.host
    ```


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109944867
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1407,6 +1441,19 @@ def operation_template_fk(cls):
     
         # endregion
     
    +    def configure(self):
    --- End diff --
    
    OK, so where? We don't have these separate module yet and I did not think I should be creating it as part of this PR.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111151737
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1685,12 +1725,31 @@ def inputs(cls):
         # 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))
         executor = Column(Text)
         max_retries = Column(Integer)
         retry_interval = Column(Integer)
     
    +    def configure(self):
    +        from . import models
    +        if (self.implementation is None) or (self.interface is None):
    --- End diff --
    
    how can interface be None here?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111197005
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    --- End diff --
    
    Due to the various validations. The structure is also a little bit different. In the service template, the user doesn't have to write "fabric_env" (or really shouldn't care about our implementation detail at all), the just write "ssh.password".


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111184429
  
    --- Diff: aria/orchestrator/workflows/api/task.py ---
    @@ -66,98 +66,84 @@ def __init__(self,
                      actor_type,
                      interface_name,
                      operation_name,
    -                 runs_on=None,
    +                 inputs=None,
                      max_attempts=None,
                      retry_interval=None,
    -                 ignore_failure=None,
    -                 inputs=None):
    +                 ignore_failure=None):
             """
             Do not call this constructor directly. Instead, use :meth:`for_node` or
             :meth:`for_relationship`.
             """
     
    -        assert isinstance(actor, (models.Node, models.Relationship))
    -        assert actor_type in ('node', 'relationship')
             assert interface_name and operation_name
    -        assert runs_on in models.Task.RUNS_ON
             super(OperationTask, self).__init__()
     
    +        operation = None
    +        interface = actor.interfaces.get(interface_name)
    +        if interface is not None:
    +            operation = interface.operations.get(operation_name)
    +
    +        if operation is None:
    +            raise exceptions.OperationNotFoundException(
    +                'Could not find operation "{0}" on interface "{1}" for {2} "{3}"'
    +                .format(operation_name, interface_name, actor_type, actor.name))
    +
    +        if operation.implementation is None:
    --- End diff --
    
    Why should an empty operation raise an error (rather than do nothing)?
    if a node template has its type as root, and implements `create` with a proper implementation but does not use `configure`, would it not appear as an empty implementation 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109843083
  
    --- Diff: aria/modeling/orchestration.py ---
    @@ -200,15 +200,52 @@ class PluginBase(ModelMixin):
     
     class TaskBase(ModelMixin):
         """
    -    A Model which represents an task
    +    Represents the smallest unit of stateful execution in ARIA. The task state includes inputs,
    +    outputs, as well as an atomic status, ensuring that the task can only be running once at any
    +    given time.
    +
    +    Tasks may be "one shot" or may be configured to run repeatedly in the case of failure.
    +
    +    Tasks are often based on :class:`Operation`, and thus act on either a :class:`Node` or a
    +    :class:`Relationship`, however this is not required.
    +
    +    :ivar node: The node actor (optional)
    +    :vartype node: :class:`Node`
    +    :ivar relationship: The relationship actor (optional)
    +    :vartype relationship: :class:`Relationship`
    +    :ivar plugin: The implementing plugin (set to None for default execution plugin)
    +    :vartype plugin: :class:`Plugin`
    +    :ivar inputs: Parameters that can be used by this task
    +    :vartype inputs: {basestring: :class:`Parameter`}
    +    :ivar implementation: Python path to an ``@operation`` function
    +    :vartype implementation: basestring
    +    :ivar max_attempts: Maximum number of retries allowed in case of failure
    +    :vartype max_attempts: int
    +    :ivar retry_interval: Interval between retries (in seconds)
    +    :vartype retry_interval: int
    +    :ivar ignore_failure: Set to True to ignore failures
    +    :vartype ignore_failure: bool
    +    :ivar due_at: Timestamp to start the task
    +    :vartype due_at: datetime
    +    :ivar execution: Assigned execution
    +    :vartype execution: :class:`Execution`
    +    :ivar status: Current atomic status ('pending', 'retrying', 'sent', 'started', 'success',
    +                  'failed')
    +    :vartype status: basestring
    +    :ivar started_at: Timestamp for when task started
    +    :vartype started_at: datetime
    +    :ivar ended_at: Timestamp for when task ended
    +    :vartype ended_at: datetime
    +    :ivar retry_count: How many retries occurred
    +    :vartype retry_count: int
         """
     
         __tablename__ = 'task'
     
         __private_fields__ = ['node_fk',
                               'relationship_fk',
                               'plugin_fk',
    -                          'execution_fk',
    --- End diff --
    
    ?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111420329
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1632,9 +1673,9 @@ def interface_fk(cls):
             return relationship.foreign_key('interface', nullable=True)
     
         @declared_attr
    -    def plugin_specification_fk(cls):
    -        """For Operation one-to-one to PluginSpecification"""
    -        return relationship.foreign_key('plugin_specification', nullable=True)
    +    def plugin_fk(cls):
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109944668
  
    --- Diff: aria/modeling/orchestration.py ---
    @@ -258,29 +290,17 @@ def execution(cls):
         def inputs(cls):
             return relationship.many_to_many(cls, 'parameter', prefix='inputs', dict_key='name')
     
    -    status = Column(Enum(*STATES, name='status'), default=PENDING)
    -
    -    due_at = Column(DateTime, nullable=False, index=True, default=datetime.utcnow())
    -    started_at = Column(DateTime, default=None)
    -    ended_at = Column(DateTime, default=None)
    +    implementation = Column(String)
         max_attempts = Column(Integer, default=1)
    -    retry_count = Column(Integer, default=0)
         retry_interval = Column(Float, default=0)
         ignore_failure = Column(Boolean, default=False)
    +    due_at = Column(DateTime, nullable=False, index=True, default=datetime.utcnow())
    --- End diff --
    
    Does `due_at` ever change after creation? I guess you're implying that it does...


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111196744
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    +    arguments['hide_output'] = ssh.get('hide_output')
    +    arguments['fabric_env'] = {}
    +    if 'warn_only' in ssh:
    +        arguments['fabric_env']['warn_only'] = ssh['warn_only']
    +    arguments['fabric_env']['user'] = ssh.get('user')
    +    arguments['fabric_env']['password'] = ssh.get('password')
    +    arguments['fabric_env']['key'] = ssh.get('key')
    +    arguments['fabric_env']['key_filename'] = ssh.get('key_filename')
    +    if 'address' in ssh:
    --- End diff --
    
    Well, I read our code and figured these out. :) They were not well documented anywhere.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109884377
  
    --- Diff: aria/parser/consumption/modeling.py ---
    @@ -145,6 +145,24 @@ def consume(self):
             self.context.modeling.instance.validate_capabilities()
     
     
    +class FindHosts(Consumer):
    --- End diff --
    
    why is this implemented as another consumer?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111616080
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -2077,27 +2091,39 @@ def service_template_fk(cls):
             """For ServiceTemplate one-to-many to PluginSpecification"""
             return relationship.foreign_key('service_template', nullable=True)
     
    +    @declared_attr
    +    def plugin_fk(cls):
    +        """For PluginSpecification many-to-one to Plugin"""
    +        return relationship.foreign_key('plugin', nullable=True)
    +
         # endregion
     
    +    # region many_to_one relationships
    +
         @declared_attr
         def service_template(cls):
             return relationship.many_to_one(cls, 'service_template')
     
    +    @declared_attr
    +    def plugin(cls): # pylint: disable=method-hidden
    +        return relationship.many_to_one(cls, 'plugin', back_populates=relationship.NO_BACK_POP)
    +
    +    # endregion
    +
         @property
         def as_raw(self):
             return collections.OrderedDict((
                 ('name', self.name),
    -            ('version', self.version)))
    +            ('version', self.version),
    +            ('enabled', self.enabled)))
     
         def coerce_values(self, container, report_issues):
             pass
     
         def instantiate(self, container):
    -        from . import models
    --- End diff --
    
    wait, so instantiate is now empty? :confused: 


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111153941
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -98,8 +98,8 @@ topology_template:
                   #token: { get_property: [ HOST, flavor_name ] }
           interfaces:
             Maintenance:
    -          enable: juju > charm.maintenance_on
    -          disable: juju > charm.maintenance_off
    +          enable: juju \> charm.maintenance_on
    --- End diff --
    
    wait so what's this symbol about again? :)
    Why are we not using the same symbol as we do for `dependencies`?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111441695
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -522,17 +524,10 @@ def properties(cls):
         __mapper_args__ = {'version_id_col': version} # Enable SQLAlchemy automatic version counting
     
         @property
    -    def ip(self):
    -        # TODO: totally broken
    -        if not self.host_fk:
    -            return None
    -        host_node = self.host
    -        if 'ip' in host_node.runtime_properties:  # pylint: disable=no-member
    -            return host_node.runtime_properties['ip']  # pylint: disable=no-member
    -        host_node = host_node.node_template  # pylint: disable=no-member
    -        host_ip_property = host_node.properties.get('ip')
    -        if host_ip_property:
    -            return host_ip_property.value
    +    def host_address(self):
    +        if self.host:
    +            if self.host.runtime_properties:
    +                return self.host.runtime_properties.get('host_address')
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111190496
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -352,20 +359,44 @@ def create_interface_template_model(context, service_template, interface):
         return model if model.operation_templates else None
     
     
    -def create_operation_template_model(context, service_template, operation): # pylint: disable=unused-argument
    +def create_operation_template_model(context, service_template, operation):
         model = OperationTemplate(name=operation._name)
     
         if operation.description:
             model.description = operation.description.value
     
         implementation = operation.implementation
    -    if (implementation is not None) and operation.implementation.primary:
    -        model.plugin_specification, model.implementation = \
    -            parse_implementation_string(context, service_template, operation.implementation.primary)
    -
    +    if implementation is not None: 
    +        primary = implementation.primary
    +        plugin_name, model.implementation = split_prefix(primary)
    +        if plugin_name is not None:
    +            model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +            if model.plugin_specification is None:
    +                context.validation.report(
    +                    'unknown plugin "%s" specified in operation implementation: %s'
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111615833
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1864,13 +1865,21 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    -        plugin = self.plugin_specification.find_plugin() \
    -            if self.plugin_specification is not None else None
    +        if self.plugin_specification and self.plugin_specification.enabled:
    +            plugin = self.plugin_specification.plugin
    +            implementation = self.implementation if plugin is not None else None
    +            # "plugin" would be none if a match was not found. In that case, a validation error
    --- End diff --
    
    im not sure i understood this comment.
    i think you're trying to say that `plugin` can't be None here since the test for enabled has already been performed earlier?
    the first line of the comment makes it sound like `plugin` can be None when it cant


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111161999
  
    --- Diff: aria/parser/consumption/modeling.py ---
    @@ -145,6 +145,24 @@ def consume(self):
             self.context.modeling.instance.validate_capabilities()
     
     
    +class FindHosts(Consumer):
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111189263
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1685,12 +1725,31 @@ def inputs(cls):
         # 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))
         executor = Column(Text)
         max_retries = Column(Integer)
         retry_interval = Column(Integer)
     
    +    def configure(self):
    +        from . import models
    +        if (self.implementation is None) or (self.interface is None):
    +            return
    +
    +        if self.plugin is None:
    +            arguments = configure_operation(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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111161558
  
    --- Diff: aria/orchestrator/workflows/builtin/utils.py ---
    @@ -71,15 +71,13 @@ def relationship_tasks(
             operations.append(
                 OperationTask.for_relationship(relationship=relationship,
                                                interface_name=interface_name,
    -                                           operation_name=source_operation_name,
    -                                           runs_on='source')
    +                                           operation_name=source_operation_name)
             )
         if target_operation_name:
             operations.append(
                 OperationTask.for_relationship(relationship=relationship,
                                                interface_name=interface_name,
    -                                           operation_name=target_operation_name,
    -                                           runs_on='target')
    +                                           operation_name=target_operation_name,)
    --- End diff --
    
    tuple?


---
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 #95: ARIA-92 Automatic operation task confi...

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

    https://github.com/apache/incubator-ariatosca/pull/95#discussion_r109900234
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    --- End diff --
    
    why would i need to define a helper method, and not just call `the_relationship.target_node.find_host()` for example?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111197419
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    +    arguments['hide_output'] = ssh.get('hide_output')
    +    arguments['fabric_env'] = {}
    +    if 'warn_only' in ssh:
    +        arguments['fabric_env']['warn_only'] = ssh['warn_only']
    +    arguments['fabric_env']['user'] = ssh.get('user')
    +    arguments['fabric_env']['password'] = ssh.get('password')
    +    arguments['fabric_env']['key'] = ssh.get('key')
    +    arguments['fabric_env']['key_filename'] = ssh.get('key_filename')
    +    if 'address' in ssh:
    +        arguments['fabric_env']['host_string'] = ssh['address']
    +
    +    if arguments['fabric_env'].get('user') is None:
    +        context = ConsumptionContext.get_thread_local()
    +        context.validation.report('must configure "ssh.user" for "{0}"'
    +                                  .format(operation.implementation),
    +                                  level=validation.Issue.BETWEEN_TYPES)
    +    if (arguments['fabric_env'].get('password') is None) and \
    +        (arguments['fabric_env'].get('key') is None) and \
    +        (arguments['fabric_env'].get('key_filename') is None):
    +        context = ConsumptionContext.get_thread_local()
    +        context.validation.report('must configure "ssh.password", "ssh.key", or "ssh.key_filename" '
    +                                  'for "{0}"'
    +                                  .format(operation.implementation),
    +                                  level=validation.Issue.BETWEEN_TYPES)
    +
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_with_ssh.__name__)
    +
    +
    +def _get_process(value):
    +    if value is None:
    +        return None
    +    _validate_type(value, dict, 'process')
    +    for k, v in value.iteritems():
    +        if k == 'eval_python':
    +            value[k] = _str_to_bool(v, 'process.eval_python')
    +        elif k == 'cwd':
    +            _validate_type(v, basestring, 'process.cwd')
    +        elif k == 'command_prefix':
    +            _validate_type(v, basestring, 'process.command_prefix')
    +        elif k == 'args':
    +            value[k] = _dict_to_list(v, 'process.args')
    +        elif k == 'env':
    +            _validate_type(v, dict, 'process.env')
    +        else:
    +            context = ConsumptionContext.get_thread_local()
    +            context.validation.report('unsupported configuration: "process.{0}"'.format(k),
    +                                      level=validation.Issue.BETWEEN_TYPES)
    +    return value
    +
    +
    +def _get_ssh(value):
    +    if value is None:
    +        return {}
    +    _validate_type(value, dict, 'ssh')
    +    for k, v in value.iteritems():
    +        if k == 'use_sudo':
    --- End diff --
    
    I thought it would be more straightforward to do this than have a generic mechanism. Since *only* this plugin does this (for all the rest the configuration params get merged into inputs) it seemed unnecessary for me to create a generic mechanism.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111441903
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1685,12 +1725,31 @@ def inputs(cls):
         # 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))
         executor = Column(Text)
         max_retries = Column(Integer)
         retry_interval = Column(Integer)
     
    +    def configure(self):
    +        from . import models
    +        if (self.implementation is None) or (self.interface is 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111368126
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -352,20 +359,44 @@ def create_interface_template_model(context, service_template, interface):
         return model if model.operation_templates else None
     
     
    -def create_operation_template_model(context, service_template, operation): # pylint: disable=unused-argument
    +def create_operation_template_model(context, service_template, operation):
         model = OperationTemplate(name=operation._name)
     
         if operation.description:
             model.description = operation.description.value
     
         implementation = operation.implementation
    -    if (implementation is not None) and operation.implementation.primary:
    -        model.plugin_specification, model.implementation = \
    -            parse_implementation_string(context, service_template, operation.implementation.primary)
    -
    +    if implementation is not None: 
    +        primary = implementation.primary
    +        plugin_name, model.implementation = split_prefix(primary)
    +        if plugin_name is not None:
    +            model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +            if model.plugin_specification is None:
    +                context.validation.report(
    +                    'unknown plugin "%s" specified in operation implementation: %s'
    +                    % (plugin_name, primary),
    +                    locator=operation._get_child_locator('implementation', 'primary'),
    +                    level=Issue.BETWEEN_TYPES)
    +    
    +        relationship_edge = operation._get_extensions(context).get('relationship_edge')
    +        if relationship_edge is not None:
    --- End diff --
    
    Don't you think it'd be better to have it be set to `source` by default?
    I assume when it's `None` we should treat it as `source`, so I think it'd be better if the data model reflects this design decision, 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111147585
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -522,17 +524,10 @@ def properties(cls):
         __mapper_args__ = {'version_id_col': version} # Enable SQLAlchemy automatic version counting
     
         @property
    -    def ip(self):
    -        # TODO: totally broken
    -        if not self.host_fk:
    -            return None
    -        host_node = self.host
    -        if 'ip' in host_node.runtime_properties:  # pylint: disable=no-member
    -            return host_node.runtime_properties['ip']  # pylint: disable=no-member
    -        host_node = host_node.node_template  # pylint: disable=no-member
    -        host_ip_property = host_node.properties.get('ip')
    -        if host_ip_property:
    -            return host_ip_property.value
    +    def host_address(self):
    +        if self.host:
    +            if self.host.runtime_properties:
    +                return self.host.runtime_properties.get('host_address')
    --- End diff --
    
    while I understand your reasoning to using "host_address" instead of "ip" everywhere, the actual runtime property that gets set by plugins remains to be "ip". this can't change 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111417689
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,188 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +def configure_operation(operation):
    --- 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 #95: ARIA-92 Automatic operation task confi...

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

    https://github.com/apache/incubator-ariatosca/pull/95#discussion_r109912807
  
    --- Diff: tests/modeling/test_models.py ---
    @@ -583,48 +583,49 @@ def test_node_model_creation(self, node_template_storage, is_valid, name, runtim
                        node_template_storage.service.list()[0]
     
     
    -class TestNodeIP(object):
    +class TestNodeHostAddress(object):
    --- End diff --
    
    no new tests were written for the new feature. 


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111169192
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1848,11 +1856,30 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    +        from ..orchestrator import context
    +        plugin = None
    +        if self.plugin_specification is not None:
    --- End diff --
    
    I'm not sure I understand this section.
    Operations are instantiated at service creation time. No workflow context should be available at this time.
    If a storage model object is required, it should be passed into the instantiate module directly (the current interface is problematic as is, and I've already broken it on the CLI branch in order to pass inputs to service template's instantiate), not taken off some thread local context.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109884066
  
    --- Diff: tests/modeling/test_models.py ---
    @@ -583,48 +583,49 @@ def test_node_model_creation(self, node_template_storage, is_valid, name, runtim
                        node_template_storage.service.list()[0]
     
     
    -class TestNodeIP(object):
    +class TestNodeHostAddress(object):
    --- End diff --
    
    why change this everywhere?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111165982
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -677,21 +715,18 @@ def pattern(node_type, container): # pylint: disable=unused-argument
         return None
     
     
    -def parse_implementation_string(context, service_template, implementation):
    -    if not implementation:
    -        return None, ''
    +def split_prefix(string):
    +    split = IMPLEMENTATION_PREFIX_REGEX.split(string, 2)
    +    if len(split) < 2:
    +        return None, string
    +    return split[0].strip(), split[1].lstrip()
     
    -    index = implementation.find('>')
    -    if index == -1:
    -        return None, implementation
    -    plugin_name = implementation[:index].strip()
    -    
    -    if plugin_name == 'execution':
    -        plugin_specification = None
    -    else:
    -        plugin_specification = service_template.plugin_specifications.get(plugin_name)
    -        if plugin_specification is None:
    -            raise ValueError('unknown plugin: "{0}"'.format(plugin_name))
     
    -    implementation = implementation[index+1:].strip()
    -    return plugin_specification, implementation
    +def set_nested(the_dict, keys, value):
    --- End diff --
    
    could you add a few comments here, possibly an example of input --> output, just so it's clearer? thanks


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111189947
  
    --- Diff: aria/orchestrator/workflows/builtin/utils.py ---
    @@ -71,15 +71,13 @@ def relationship_tasks(
             operations.append(
                 OperationTask.for_relationship(relationship=relationship,
                                                interface_name=interface_name,
    -                                           operation_name=source_operation_name,
    -                                           runs_on='source')
    +                                           operation_name=source_operation_name)
             )
         if target_operation_name:
             operations.append(
                 OperationTask.for_relationship(relationship=relationship,
                                                interface_name=interface_name,
    -                                           operation_name=target_operation_name,
    -                                           runs_on='target')
    +                                           operation_name=target_operation_name,)
    --- End diff --
    
    :+1: (though it's totally legally in Python)


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109885117
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1407,6 +1441,19 @@ def operation_template_fk(cls):
     
         # endregion
     
    +    def configure(self):
    --- End diff --
    
    this definitely cant be here.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111195127
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1848,11 +1856,30 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    +        from ..orchestrator import context
    +        plugin = None
    +        if self.plugin_specification is not None:
    +            # TODO: we are planning a separate "instantiation" module where this will be called or
    +            # moved to. There, we will probably have a context with a storage manager. Until then,
    +            # this is the only potentially available context, which of course will only be available
    +            # if we're in a workflow.
    +            plugin = 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109885995
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -180,6 +181,16 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_hosts(self):
    --- End diff --
    
    more methods on the models which we'll want to remove in a short while :(
    i mean, this method isn't relevant for an instantiated service - it's only used for its (and its dependencies) creation. why would it sit as part of the instance?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111368446
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -98,8 +98,8 @@ topology_template:
                   #token: { get_property: [ HOST, flavor_name ] }
           interfaces:
             Maintenance:
    -          enable: juju > charm.maintenance_on
    -          disable: juju > charm.maintenance_off
    +          enable: juju \> charm.maintenance_on
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109946315
  
    --- Diff: aria/parser/consumption/modeling.py ---
    @@ -145,6 +145,24 @@ def consume(self):
             self.context.modeling.instance.validate_capabilities()
     
     
    +class FindHosts(Consumer):
    --- End diff --
    
    This is just how phases work right now in "the parser". We keep talking about all of this moving into a separate "instantiation" module, but this is what we have for now.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109950298
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    --- End diff --
    
    If we do that then we'll be calling `find_host` again on again on the same nodes that are targets of different relationships. So, to do that efficiently we would need some extra flag (`found_host`?) to signify that we don't have to do that again for that node. It made more sense to me to have it set once and only once per node.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111189433
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -98,8 +98,8 @@ topology_template:
                   #token: { get_property: [ HOST, flavor_name ] }
           interfaces:
             Maintenance:
    -          enable: juju > charm.maintenance_on
    -          disable: juju > charm.maintenance_off
    +          enable: juju \> charm.maintenance_on
    --- End diff --
    
    I've put the backslash to escape it and disable it for now. :/ Since the Juju plugin is not installed, the tests that use this would fail otherwise.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109952876
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1544,11 +1547,24 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    +        from ..orchestrator import context
    +        plugin = None
    +        if self.plugin_specification is not None:
    +            try:
    +                workflow_context = context.workflow.current.get()
    +                plugin = self.plugin_specification.find_plugin(workflow_context.model.plugin.list())
    +            except context.exceptions.ContextException:
    +                pass
    +                # TODO
    --- End diff --
    
    Yes, we need to talk about this. :) I don't know how else to find other models. Is there a current context for SQLAlchemy?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111165113
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -461,8 +492,15 @@ def create_workflow_operation_template_model(context, service_template, policy):
         properties = policy._get_property_values(context)
         for prop_name, prop in properties.iteritems():
             if prop_name == 'implementation':
    -            model.plugin_specification, model.implementation = \
    -                parse_implementation_string(context, service_template, prop.value)
    +            plugin_name, model.implementation = split_prefix(prop.value)
    +            if plugin_name is not None:
    --- End diff --
    
    duplicate code, why not refactor 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111189585
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -78,6 +78,11 @@ def get_inherited_capability_definitions(context, presentation, for_presentation
                     #capability_definitions[capability_name] = capability_definition
                 else:
                     capability_definition = our_capability_definition._clone(for_presentation)
    +                if isinstance(capability_definition._raw, basestring):
    +                    # # Make sure we have a dict
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111148838
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    +            if node.type.role == 'host':
    +                return node
    +            for the_relationship in node.outbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'host':
    +                    host = _find_host(the_relationship.target_node)
    +                    if host is not None:
    +                        return host
    +            for the_relationship in node.inbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111196525
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    --- End diff --
    
    Oops, leftover from devs. :+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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111190396
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -352,20 +359,44 @@ def create_interface_template_model(context, service_template, interface):
         return model if model.operation_templates else None
     
     
    -def create_operation_template_model(context, service_template, operation): # pylint: disable=unused-argument
    +def create_operation_template_model(context, service_template, operation):
         model = OperationTemplate(name=operation._name)
     
         if operation.description:
             model.description = operation.description.value
     
         implementation = operation.implementation
    -    if (implementation is not None) and operation.implementation.primary:
    -        model.plugin_specification, model.implementation = \
    -            parse_implementation_string(context, service_template, operation.implementation.primary)
    -
    +    if implementation is not None: 
    +        primary = implementation.primary
    +        plugin_name, model.implementation = split_prefix(primary)
    +        if plugin_name is not None:
    +            model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +            if model.plugin_specification is None:
    +                context.validation.report(
    +                    'unknown plugin "%s" specified in operation implementation: %s'
    +                    % (plugin_name, primary),
    +                    locator=operation._get_child_locator('implementation', 'primary'),
    +                    level=Issue.BETWEEN_TYPES)
    +    
    +        relationship_edge = operation._get_extensions(context).get('relationship_edge')
    +        if relationship_edge is not None:
    --- End diff --
    
    None is the default -- it means that nowhere did the user set the edge explicitly set one way or the other (the column is nullable).


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111195524
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1848,11 +1856,30 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    +        from ..orchestrator import context
    +        plugin = None
    +        if self.plugin_specification is not None:
    --- End diff --
    
    You are correct, I had a long conversation with Maxim about it. Eventually when we have an instantiation module, it is very likely that the module will have a context. See comment at line 1861.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111191148
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    +    arguments['hide_output'] = ssh.get('hide_output')
    +    arguments['fabric_env'] = {}
    +    if 'warn_only' in ssh:
    +        arguments['fabric_env']['warn_only'] = ssh['warn_only']
    +    arguments['fabric_env']['user'] = ssh.get('user')
    +    arguments['fabric_env']['password'] = ssh.get('password')
    +    arguments['fabric_env']['key'] = ssh.get('key')
    +    arguments['fabric_env']['key_filename'] = ssh.get('key_filename')
    +    if 'address' in ssh:
    +        arguments['fabric_env']['host_string'] = ssh['address']
    +
    +    if arguments['fabric_env'].get('user') is None:
    +        context = ConsumptionContext.get_thread_local()
    +        context.validation.report('must configure "ssh.user" for "{0}"'
    +                                  .format(operation.implementation),
    +                                  level=validation.Issue.BETWEEN_TYPES)
    +    if (arguments['fabric_env'].get('password') is None) and \
    +        (arguments['fabric_env'].get('key') is None) and \
    +        (arguments['fabric_env'].get('key_filename') is None):
    +        context = ConsumptionContext.get_thread_local()
    +        context.validation.report('must configure "ssh.password", "ssh.key", or "ssh.key_filename" '
    +                                  'for "{0}"'
    +                                  .format(operation.implementation),
    +                                  level=validation.Issue.BETWEEN_TYPES)
    +
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_with_ssh.__name__)
    +
    +
    +def _get_process(value):
    +    if value is None:
    +        return None
    +    _validate_type(value, dict, 'process')
    +    for k, v in value.iteritems():
    +        if k == 'eval_python':
    +            value[k] = _str_to_bool(v, 'process.eval_python')
    +        elif k == 'cwd':
    +            _validate_type(v, basestring, 'process.cwd')
    +        elif k == 'command_prefix':
    +            _validate_type(v, basestring, 'process.command_prefix')
    +        elif k == 'args':
    +            value[k] = _dict_to_list(v, 'process.args')
    +        elif k == 'env':
    +            _validate_type(v, dict, 'process.env')
    +        else:
    +            context = ConsumptionContext.get_thread_local()
    +            context.validation.report('unsupported configuration: "process.{0}"'.format(k),
    +                                      level=validation.Issue.BETWEEN_TYPES)
    +    return value
    +
    +
    +def _get_ssh(value):
    +    if value is None:
    +        return {}
    +    _validate_type(value, dict, 'ssh')
    +    for k, v in value.iteritems():
    +        if k == 'use_sudo':
    --- End diff --
    
    i think it'd probably have been neater to have a dict mapping field-name to type and then validating it rather than having to add here manually every new field that might get introduced


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109952085
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    +            if node.type.role == 'host':
    +                return node
    +            for the_relationship in node.outbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'host':
    +                    host = _find_host(the_relationship.target_node)
    +                    if host is not None:
    +                        return host
    +            for the_relationship in node.inbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    --- End diff --
    
    This is a bit hard to explain. We tend to think of the host relationship as "contained in", but TOSCA also allows for "hosts a" kind of relationship, for example for features. See [here](https://github.com/apache/incubator-ariatosca/blob/ARIA-92-plugin-in-implementation-string/tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml#L177): to find out the loadbalancer node's host, we need to look inbound that it is a feature of Nginx.
    
    So, bottom line is that this function has to be bi-recursive, both inbound and outbound. Yay. :)


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109940915
  
    --- Diff: aria/modeling/orchestration.py ---
    @@ -200,15 +200,52 @@ class PluginBase(ModelMixin):
     
     class TaskBase(ModelMixin):
         """
    -    A Model which represents an task
    +    Represents the smallest unit of stateful execution in ARIA. The task state includes inputs,
    +    outputs, as well as an atomic status, ensuring that the task can only be running once at any
    +    given time.
    +
    +    Tasks may be "one shot" or may be configured to run repeatedly in the case of failure.
    +
    +    Tasks are often based on :class:`Operation`, and thus act on either a :class:`Node` or a
    +    :class:`Relationship`, however this is not required.
    +
    +    :ivar node: The node actor (optional)
    +    :vartype node: :class:`Node`
    +    :ivar relationship: The relationship actor (optional)
    +    :vartype relationship: :class:`Relationship`
    +    :ivar plugin: The implementing plugin (set to None for default execution plugin)
    +    :vartype plugin: :class:`Plugin`
    +    :ivar inputs: Parameters that can be used by this task
    +    :vartype inputs: {basestring: :class:`Parameter`}
    +    :ivar implementation: Python path to an ``@operation`` function
    +    :vartype implementation: basestring
    +    :ivar max_attempts: Maximum number of retries allowed in case of failure
    +    :vartype max_attempts: int
    +    :ivar retry_interval: Interval between retries (in seconds)
    +    :vartype retry_interval: int
    +    :ivar ignore_failure: Set to True to ignore failures
    +    :vartype ignore_failure: bool
    +    :ivar due_at: Timestamp to start the task
    +    :vartype due_at: datetime
    +    :ivar execution: Assigned execution
    +    :vartype execution: :class:`Execution`
    +    :ivar status: Current atomic status ('pending', 'retrying', 'sent', 'started', 'success',
    +                  'failed')
    +    :vartype status: basestring
    +    :ivar started_at: Timestamp for when task started
    +    :vartype started_at: datetime
    +    :ivar ended_at: Timestamp for when task ended
    +    :vartype ended_at: datetime
    +    :ivar retry_count: How many retries occurred
    +    :vartype retry_count: int
         """
     
         __tablename__ = 'task'
     
         __private_fields__ = ['node_fk',
                               'relationship_fk',
                               'plugin_fk',
    -                          'execution_fk',
    --- End diff --
    
    The diff algorithm borked here, if you look at the file it makes sense.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111162387
  
    --- Diff: aria/modeling/orchestration.py ---
    @@ -200,15 +200,52 @@ class PluginBase(ModelMixin):
     
     class TaskBase(ModelMixin):
         """
    -    A Model which represents an task
    +    Represents the smallest unit of stateful execution in ARIA. The task state includes inputs,
    +    outputs, as well as an atomic status, ensuring that the task can only be running once at any
    +    given time.
    +
    +    Tasks may be "one shot" or may be configured to run repeatedly in the case of failure.
    +
    +    Tasks are often based on :class:`Operation`, and thus act on either a :class:`Node` or a
    +    :class:`Relationship`, however this is not required.
    +
    +    :ivar node: The node actor (optional)
    +    :vartype node: :class:`Node`
    +    :ivar relationship: The relationship actor (optional)
    +    :vartype relationship: :class:`Relationship`
    +    :ivar plugin: The implementing plugin (set to None for default execution plugin)
    +    :vartype plugin: :class:`Plugin`
    +    :ivar inputs: Parameters that can be used by this task
    +    :vartype inputs: {basestring: :class:`Parameter`}
    +    :ivar implementation: Python path to an ``@operation`` function
    +    :vartype implementation: basestring
    +    :ivar max_attempts: Maximum number of retries allowed in case of failure
    +    :vartype max_attempts: int
    +    :ivar retry_interval: Interval between retries (in seconds)
    +    :vartype retry_interval: int
    +    :ivar ignore_failure: Set to True to ignore failures
    +    :vartype ignore_failure: bool
    +    :ivar due_at: Timestamp to start the task
    +    :vartype due_at: datetime
    +    :ivar execution: Assigned execution
    +    :vartype execution: :class:`Execution`
    +    :ivar status: Current atomic status ('pending', 'retrying', 'sent', 'started', 'success',
    +                  'failed')
    +    :vartype status: basestring
    +    :ivar started_at: Timestamp for when task started
    +    :vartype started_at: datetime
    +    :ivar ended_at: Timestamp for when task ended
    +    :vartype ended_at: datetime
    +    :ivar retry_count: How many retries occurred
    +    :vartype retry_count: int
         """
     
         __tablename__ = 'task'
     
         __private_fields__ = ['node_fk',
                               'relationship_fk',
                               'plugin_fk',
    -                          'execution_fk',
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111443310
  
    --- Diff: aria/orchestrator/workflows/api/task.py ---
    @@ -66,98 +66,84 @@ def __init__(self,
                      actor_type,
                      interface_name,
                      operation_name,
    -                 runs_on=None,
    +                 inputs=None,
                      max_attempts=None,
                      retry_interval=None,
    -                 ignore_failure=None,
    -                 inputs=None):
    +                 ignore_failure=None):
             """
             Do not call this constructor directly. Instead, use :meth:`for_node` or
             :meth:`for_relationship`.
             """
     
    -        assert isinstance(actor, (models.Node, models.Relationship))
    -        assert actor_type in ('node', 'relationship')
             assert interface_name and operation_name
    -        assert runs_on in models.Task.RUNS_ON
             super(OperationTask, self).__init__()
     
    +        operation = None
    +        interface = actor.interfaces.get(interface_name)
    +        if interface is not None:
    +            operation = interface.operations.get(operation_name)
    +
    +        if operation is None:
    +            raise exceptions.OperationNotFoundException(
    +                'Could not find operation "{0}" on interface "{1}" for {2} "{3}"'
    +                .format(operation_name, interface_name, actor_type, actor.name))
    +
    +        if operation.implementation is None:
    --- End diff --
    
    We agreed to keep this behavior for now.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109843227
  
    --- Diff: aria/modeling/orchestration.py ---
    @@ -258,29 +290,17 @@ def execution(cls):
         def inputs(cls):
             return relationship.many_to_many(cls, 'parameter', prefix='inputs', dict_key='name')
     
    -    status = Column(Enum(*STATES, name='status'), default=PENDING)
    -
    -    due_at = Column(DateTime, nullable=False, index=True, default=datetime.utcnow())
    -    started_at = Column(DateTime, default=None)
    -    ended_at = Column(DateTime, default=None)
    +    implementation = Column(String)
         max_attempts = Column(Integer, default=1)
    -    retry_count = Column(Integer, default=0)
         retry_interval = Column(Float, default=0)
         ignore_failure = Column(Boolean, default=False)
    +    due_at = Column(DateTime, nullable=False, index=True, default=datetime.utcnow())
    --- End diff --
    
    if you already do the separation to "state" fields and other, this should probably be under state.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111412168
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111188352
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1685,12 +1725,31 @@ def inputs(cls):
         # 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))
         executor = Column(Text)
         max_retries = Column(Integer)
         retry_interval = Column(Integer)
     
    +    def configure(self):
    +        from . import models
    +        if (self.implementation is None) or (self.interface is None):
    --- End diff --
    
    It will be None if the operation is attached to `Service` as a workflow :) I know you suggested a different model entirely for workflows ... that would be a separate JIRA perhaps.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111194890
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -677,21 +715,18 @@ def pattern(node_type, container): # pylint: disable=unused-argument
         return None
     
     
    -def parse_implementation_string(context, service_template, implementation):
    -    if not implementation:
    -        return None, ''
    +def split_prefix(string):
    +    split = IMPLEMENTATION_PREFIX_REGEX.split(string, 2)
    +    if len(split) < 2:
    +        return None, string
    +    return split[0].strip(), split[1].lstrip()
     
    -    index = implementation.find('>')
    -    if index == -1:
    -        return None, implementation
    -    plugin_name = implementation[:index].strip()
    -    
    -    if plugin_name == 'execution':
    -        plugin_specification = None
    -    else:
    -        plugin_specification = service_template.plugin_specifications.get(plugin_name)
    -        if plugin_specification is None:
    -            raise ValueError('unknown plugin: "{0}"'.format(plugin_name))
     
    -    implementation = implementation[index+1:].strip()
    -    return plugin_specification, implementation
    +def set_nested(the_dict, keys, value):
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111415430
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -677,21 +698,47 @@ def pattern(node_type, container): # pylint: disable=unused-argument
         return None
     
     
    -def parse_implementation_string(context, service_template, implementation):
    -    if not implementation:
    -        return None, ''
    +def split_prefix(string):
    +    """
    +    Splits the prefix on the first unescaped ">".
    +    """
     
    -    index = implementation.find('>')
    -    if index == -1:
    -        return None, implementation
    -    plugin_name = implementation[:index].strip()
    -    
    -    if plugin_name == 'execution':
    -        plugin_specification = None
    -    else:
    -        plugin_specification = service_template.plugin_specifications.get(plugin_name)
    -        if plugin_specification is None:
    -            raise ValueError('unknown plugin: "{0}"'.format(plugin_name))
    +    split = IMPLEMENTATION_PREFIX_REGEX.split(string, 2)
    +    if len(split) < 2:
    +        return None, string
    +    return split[0].strip(), split[1].lstrip()
    +
    +
    +def set_nested(the_dict, keys, value):
    +    """
    +    If the ``keys`` list has just one item, puts the value in the the dict. If there are more items,
    +    puts the value in a sub-dict, creating sub-dicts as necessary for each key.
     
    -    implementation = implementation[index+1:].strip()
    -    return plugin_specification, implementation
    +    For example, if ``the_dict`` is an empty dict, keys is ``['first', 'second', 'third']`` and
    +    value is ``'value'``, then the_dict will be: ``{'first':{'second':{'third':'value'}}}``.
    +
    +    :param the_dict: Dict to change
    +    :type the_dict: {}
    +    :param keys: Keys
    +    :type keys: [basestring]
    +    :param value: Value
    +    """
    +    key = keys.pop(0)
    +    if len(keys) == 0:
    +        the_dict[key] = value
    +    else:
    +        if key not in the_dict:
    +            the_dict[key] = StrictDict(key_class=basestring)
    +        set_nested(the_dict[key], keys, value)
    +
    +
    +def parse_implementation_string(context, service_template, presentation, model, implementation):
    +    plugin_name, model.implementation = split_prefix(implementation)
    +    if plugin_name is not None:
    +        model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +        if model.plugin_specification is None:
    +            context.validation.report(
    +                'no policy for plugin "%s" specified in operation implementation: %s'
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111188155
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1632,9 +1673,9 @@ def interface_fk(cls):
             return relationship.foreign_key('interface', nullable=True)
     
         @declared_attr
    -    def plugin_specification_fk(cls):
    -        """For Operation one-to-one to PluginSpecification"""
    -        return relationship.foreign_key('plugin_specification', nullable=True)
    +    def plugin_fk(cls):
    --- End diff --
    
    Correct.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111190512
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    +    arguments['hide_output'] = ssh.get('hide_output')
    +    arguments['fabric_env'] = {}
    +    if 'warn_only' in ssh:
    +        arguments['fabric_env']['warn_only'] = ssh['warn_only']
    +    arguments['fabric_env']['user'] = ssh.get('user')
    +    arguments['fabric_env']['password'] = ssh.get('password')
    +    arguments['fabric_env']['key'] = ssh.get('key')
    +    arguments['fabric_env']['key_filename'] = ssh.get('key_filename')
    +    if 'address' in ssh:
    +        arguments['fabric_env']['host_string'] = ssh['address']
    +
    +    if arguments['fabric_env'].get('user') is None:
    +        context = ConsumptionContext.get_thread_local()
    +        context.validation.report('must configure "ssh.user" for "{0}"'
    +                                  .format(operation.implementation),
    +                                  level=validation.Issue.BETWEEN_TYPES)
    +    if (arguments['fabric_env'].get('password') is None) and \
    +        (arguments['fabric_env'].get('key') is None) and \
    +        (arguments['fabric_env'].get('key_filename') is None):
    +        context = ConsumptionContext.get_thread_local()
    +        context.validation.report('must configure "ssh.password", "ssh.key", or "ssh.key_filename" '
    +                                  'for "{0}"'
    +                                  .format(operation.implementation),
    +                                  level=validation.Issue.BETWEEN_TYPES)
    +
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_with_ssh.__name__)
    +
    +
    +def _get_process(value):
    +    if value is None:
    +        return None
    +    _validate_type(value, dict, 'process')
    +    for k, v in value.iteritems():
    +        if k == 'eval_python':
    +            value[k] = _str_to_bool(v, 'process.eval_python')
    +        elif k == 'cwd':
    +            _validate_type(v, basestring, 'process.cwd')
    +        elif k == 'command_prefix':
    +            _validate_type(v, basestring, 'process.command_prefix')
    +        elif k == 'args':
    +            value[k] = _dict_to_list(v, 'process.args')
    +        elif k == 'env':
    +            _validate_type(v, dict, 'process.env')
    +        else:
    +            context = ConsumptionContext.get_thread_local()
    +            context.validation.report('unsupported configuration: "process.{0}"'.format(k),
    +                                      level=validation.Issue.BETWEEN_TYPES)
    +    return value
    +
    +
    +def _get_ssh(value):
    +    if value is None:
    +        return {}
    +    _validate_type(value, dict, 'ssh')
    +    for k, v in value.iteritems():
    +        if k == 'use_sudo':
    +            value[k] = _str_to_bool(v, 'ssh.use_sudo')
    +        elif k == 'hide_output':
    +            value[k] = _dict_to_list(v, 'ssh.hide_output')
    +        elif k == 'warn_only':
    +            value[k] = _str_to_bool(v, 'ssh.warn_only')
    +        elif k == 'user':
    +            _validate_type(v, basestring, 'ssh.user')
    +        elif k == 'password':
    +            _validate_type(v, basestring, 'ssh.password')
    +        elif k == 'key':
    +            _validate_type(v, basestring, 'ssh.key')
    +        elif k == 'key_filename':
    +            _validate_type(v, basestring, 'ssh.key_filename')
    +        elif k == 'address':
    +            _validate_type(v, basestring, 'ssh.address')
    +        else:
    +            context = ConsumptionContext.get_thread_local()
    +            context.validation.report('unsupported configuration: "ssh.{0}"'.format(k),
    +                                      level=validation.Issue.BETWEEN_TYPES)
    +    return value
    +
    +
    +def _validate_type(value, the_type, name):
    +    if not isinstance(value, the_type):
    +        context = ConsumptionContext.get_thread_local()
    +        context.validation.report('"{0}" configuration is not a {1}'
    +                                  .format(name, full_type_name(the_type)),
    +                                  level=validation.Issue.BETWEEN_TYPES)
    +
    +def _str_to_bool(value, name):
    --- End diff --
    
    this and the next method need to sit elsewhere. on CLI branch there's a `type` module under `utils`, and the `dict_to_list`  should go to where there's already similar methods.
    
    the validation thats called within these methods can be called before or after them


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109945270
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,188 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +def configure_operation(operation):
    --- End diff --
    
    OK, where do you suggest moving them?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111189660
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    +    default_password = 'admin'
    +    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')
    +    arguments['hide_output'] = ssh.get('hide_output')
    +    arguments['fabric_env'] = {}
    +    if 'warn_only' in ssh:
    +        arguments['fabric_env']['warn_only'] = ssh['warn_only']
    +    arguments['fabric_env']['user'] = ssh.get('user')
    +    arguments['fabric_env']['password'] = ssh.get('password')
    +    arguments['fabric_env']['key'] = ssh.get('key')
    +    arguments['fabric_env']['key_filename'] = ssh.get('key_filename')
    +    if 'address' in ssh:
    --- End diff --
    
    i assume you came up with this one?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111189473
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,187 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +# TODO: this module will eventually be moved to a new "aria.instantiation" package
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +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 None
    +
    +    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
    +            host = interface.relationship.source_node.host
    +
    +    if host is None:
    +        _configure_local(operation)
    +    else:
    +        _configure_remote(operation, configuration, arguments)
    +
    +    # 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_local(operation):
    +    """
    +    Local operation.
    +    """
    +    from . import operations
    +    operation.implementation = '{0}.{1}'.format(operations.__name__,
    +                                                operations.run_script_locally.__name__)
    +
    +
    +def _configure_remote(operation, configuration, arguments):
    +    """
    +    Remote SSH operation via Fabric.
    +    """
    +    default_user = 'admin'
    --- End diff --
    
    why these defaults? wouldnt it be better to fail?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111159247
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -78,6 +78,11 @@ def get_inherited_capability_definitions(context, presentation, for_presentation
                     #capability_definitions[capability_name] = capability_definition
                 else:
                     capability_definition = our_capability_definition._clone(for_presentation)
    +                if isinstance(capability_definition._raw, basestring):
    +                    # # Make sure we have a dict
    --- End diff --
    
    double comment


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109989092
  
    --- Diff: aria/modeling/orchestration.py ---
    @@ -258,29 +290,17 @@ def execution(cls):
         def inputs(cls):
             return relationship.many_to_many(cls, 'parameter', prefix='inputs', dict_key='name')
     
    -    status = Column(Enum(*STATES, name='status'), default=PENDING)
    -
    -    due_at = Column(DateTime, nullable=False, index=True, default=datetime.utcnow())
    -    started_at = Column(DateTime, default=None)
    -    ended_at = Column(DateTime, default=None)
    +    implementation = Column(String)
         max_attempts = Column(Integer, default=1)
    -    retry_count = Column(Integer, default=0)
         retry_interval = Column(Float, default=0)
         ignore_failure = Column(Boolean, default=False)
    +    due_at = Column(DateTime, nullable=False, index=True, default=datetime.utcnow())
    --- End diff --
    
    yup :)


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109946485
  
    --- Diff: extensions/aria_extension_tosca/profiles/tosca-simple-1.0/interfaces.yaml ---
    @@ -63,24 +63,48 @@ interface_types:
         pre_configure_source:
           description: >-
             Operation to pre-configure the source endpoint.
    +      _extensions:
    +        default_dependencies:
    +          - edge > source
    --- End diff --
    
    I thought you said that if there is no cost to allowing the user to override it, we could.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109883559
  
    --- Diff: extensions/aria_extension_tosca/profiles/tosca-simple-1.0/interfaces.yaml ---
    @@ -63,24 +63,48 @@ interface_types:
         pre_configure_source:
           description: >-
             Operation to pre-configure the source endpoint.
    +      _extensions:
    +        default_dependencies:
    +          - edge > source
    --- End diff --
    
    as we mentioned, these should not be a part of the dependencies. it should sit 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111368722
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1685,12 +1725,31 @@ def inputs(cls):
         # 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))
         executor = Column(Text)
         max_retries = Column(Integer)
         retry_interval = Column(Integer)
     
    +    def configure(self):
    +        from . import models
    +        if (self.implementation is None) or (self.interface is None):
    --- End diff --
    
    oh, right. Please add a tiny comment next to it explaining that this case is only relevant for "Workflow operations" because I'm afraid I'll wonder about it the next time I see it as well :sweat_smile:  thanks!


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111367441
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1848,11 +1856,30 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    +        from ..orchestrator import context
    +        plugin = None
    +        if self.plugin_specification is not None:
    --- End diff --
    
    I understand and I have seen the comment, but I'm saying the changes I've mentioned need to happen now, not after we have the instantiation module.
    More so, I don't understand how this is expected to work anyway, as no execution context is expect to be present at the time this method is called.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109940731
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -180,6 +181,16 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_hosts(self):
    --- End diff --
    
    That's not true: this phase will need be run every time there is a change in the topology. This is part of the reason I don't think `instantiation` is the right name for this module we talk about, since it would actually be used in many cases after instantiation.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111192691
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -461,8 +492,15 @@ def create_workflow_operation_template_model(context, service_template, policy):
         properties = policy._get_property_values(context)
         for prop_name, prop in properties.iteritems():
             if prop_name == 'implementation':
    -            model.plugin_specification, model.implementation = \
    -                parse_implementation_string(context, service_template, prop.value)
    +            plugin_name, model.implementation = split_prefix(prop.value)
    +            if plugin_name is not 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111166545
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -180,6 +181,16 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_hosts(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 #95: ARIA-92 Automatic operation task confi...

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

    https://github.com/apache/incubator-ariatosca/pull/95#discussion_r109900718
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1544,11 +1547,24 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    +        from ..orchestrator import context
    +        plugin = None
    +        if self.plugin_specification is not None:
    +            try:
    +                workflow_context = context.workflow.current.get()
    +                plugin = self.plugin_specification.find_plugin(workflow_context.model.plugin.list())
    +            except context.exceptions.ContextException:
    +                pass
    +                # TODO
    --- End diff --
    
    ?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111164451
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -352,20 +359,44 @@ def create_interface_template_model(context, service_template, interface):
         return model if model.operation_templates else None
     
     
    -def create_operation_template_model(context, service_template, operation): # pylint: disable=unused-argument
    +def create_operation_template_model(context, service_template, operation):
         model = OperationTemplate(name=operation._name)
     
         if operation.description:
             model.description = operation.description.value
     
         implementation = operation.implementation
    -    if (implementation is not None) and operation.implementation.primary:
    -        model.plugin_specification, model.implementation = \
    -            parse_implementation_string(context, service_template, operation.implementation.primary)
    -
    +    if implementation is not None: 
    +        primary = implementation.primary
    +        plugin_name, model.implementation = split_prefix(primary)
    +        if plugin_name is not None:
    +            model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +            if model.plugin_specification is None:
    +                context.validation.report(
    +                    'unknown plugin "%s" specified in operation implementation: %s'
    +                    % (plugin_name, primary),
    +                    locator=operation._get_child_locator('implementation', 'primary'),
    +                    level=Issue.BETWEEN_TYPES)
    +    
    +        relationship_edge = operation._get_extensions(context).get('relationship_edge')
    +        if relationship_edge is not None:
    --- End diff --
    
    what if it is None? where do we set the default? I think either we don't or I've missed it :)


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109886404
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    +            if node.type.role == 'host':
    +                return node
    +            for the_relationship in node.outbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'host':
    +                    host = _find_host(the_relationship.target_node)
    +                    if host is not None:
    +                        return host
    +            for the_relationship in node.inbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'feature':
    +                    host = _find_host(the_relationship.source_node)
    +                    if host is not None:
    +                        return host
    +            return None
    +
    +        self.host = _find_host(self)
    --- End diff --
    
    why is this dynamic? are we expecting this to change for an existing node?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111166567
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    +            if node.type.role == 'host':
    +                return node
    +            for the_relationship in node.outbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'host':
    +                    host = _find_host(the_relationship.target_node)
    +                    if host is not None:
    +                        return host
    +            for the_relationship in node.inbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'feature':
    +                    host = _find_host(the_relationship.source_node)
    +                    if host is not None:
    +                        return host
    +            return None
    +
    +        self.host = _find_host(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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109947108
  
    --- Diff: tests/modeling/test_models.py ---
    @@ -583,48 +583,49 @@ def test_node_model_creation(self, node_template_storage, is_valid, name, runtim
                        node_template_storage.service.list()[0]
     
     
    -class TestNodeIP(object):
    +class TestNodeHostAddress(object):
    --- End diff --
    
    That whole part of the code (finding the "ip") had to be refactored because it was essentially broken. As I was changing it anyway it made sense to change the name. It's really not hard to change 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111164850
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -352,20 +359,44 @@ def create_interface_template_model(context, service_template, interface):
         return model if model.operation_templates else None
     
     
    -def create_operation_template_model(context, service_template, operation): # pylint: disable=unused-argument
    +def create_operation_template_model(context, service_template, operation):
         model = OperationTemplate(name=operation._name)
     
         if operation.description:
             model.description = operation.description.value
     
         implementation = operation.implementation
    -    if (implementation is not None) and operation.implementation.primary:
    -        model.plugin_specification, model.implementation = \
    -            parse_implementation_string(context, service_template, operation.implementation.primary)
    -
    +    if implementation is not None: 
    +        primary = implementation.primary
    +        plugin_name, model.implementation = split_prefix(primary)
    +        if plugin_name is not None:
    +            model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +            if model.plugin_specification is None:
    +                context.validation.report(
    +                    'unknown plugin "%s" specified in operation implementation: %s'
    --- End diff --
    
    is this message clear enough about the fact that the plugin is not properly declared as a policy in the service template (rather than the question of whether it exists or not in the plugin repository)?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111151453
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1632,9 +1673,9 @@ def interface_fk(cls):
             return relationship.foreign_key('interface', nullable=True)
     
         @declared_attr
    -    def plugin_specification_fk(cls):
    -        """For Operation one-to-one to PluginSpecification"""
    -        return relationship.foreign_key('plugin_specification', nullable=True)
    +    def plugin_fk(cls):
    --- End diff --
    
    does it make sense for operation no longer to have an fk for plugin specification at all?
    is that going to remain for operation template?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109952388
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    +            if node.type.role == 'host':
    +                return node
    +            for the_relationship in node.outbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'host':
    +                    host = _find_host(the_relationship.target_node)
    +                    if host is not None:
    +                        return host
    +            for the_relationship in node.inbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'feature':
    +                    host = _find_host(the_relationship.source_node)
    +                    if host is not None:
    +                        return host
    +            return None
    +
    +        self.host = _find_host(self)
    --- End diff --
    
    I don't know what you mean by "dynamic" here. These functions must be called during the initial instantiation phase, and then will need to be called again whenever there is a change in the topology.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109883628
  
    --- Diff: extensions/aria_extension_tosca/profiles/tosca-simple-1.0/interfaces.yaml ---
    @@ -63,24 +63,48 @@ interface_types:
         pre_configure_source:
           description: >-
             Operation to pre-configure the source endpoint.
    +      _extensions:
    +        default_dependencies:
    +          - edge > source
    --- End diff --
    
    furthermore, for the time being, it's not "default", but rather the only place where one can determine where the operation shall run.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111443166
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -352,20 +359,44 @@ def create_interface_template_model(context, service_template, interface):
         return model if model.operation_templates else None
     
     
    -def create_operation_template_model(context, service_template, operation): # pylint: disable=unused-argument
    +def create_operation_template_model(context, service_template, operation):
         model = OperationTemplate(name=operation._name)
     
         if operation.description:
             model.description = operation.description.value
     
         implementation = operation.implementation
    -    if (implementation is not None) and operation.implementation.primary:
    -        model.plugin_specification, model.implementation = \
    -            parse_implementation_string(context, service_template, operation.implementation.primary)
    -
    +    if implementation is not None: 
    +        primary = implementation.primary
    +        plugin_name, model.implementation = split_prefix(primary)
    +        if plugin_name is not None:
    +            model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +            if model.plugin_specification is None:
    +                context.validation.report(
    +                    'unknown plugin "%s" specified in operation implementation: %s'
    +                    % (plugin_name, primary),
    +                    locator=operation._get_child_locator('implementation', 'primary'),
    +                    level=Issue.BETWEEN_TYPES)
    +    
    +        relationship_edge = operation._get_extensions(context).get('relationship_edge')
    +        if relationship_edge is not None:
    --- End diff --
    
    We agreed that it's better to keep this "edge was not set" information.


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111368285
  
    --- Diff: aria/orchestrator/workflows/builtin/utils.py ---
    @@ -71,15 +71,13 @@ def relationship_tasks(
             operations.append(
                 OperationTask.for_relationship(relationship=relationship,
                                                interface_name=interface_name,
    -                                           operation_name=source_operation_name,
    -                                           runs_on='source')
    +                                           operation_name=source_operation_name)
             )
         if target_operation_name:
             operations.append(
                 OperationTask.for_relationship(relationship=relationship,
                                                interface_name=interface_name,
    -                                           operation_name=target_operation_name,
    -                                           runs_on='target')
    +                                           operation_name=target_operation_name,)
    --- End diff --
    
    yes i know, I just figured this is not what you wanted to have there :sweat_smile: anyway i see its fixed, thanks


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111442898
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -98,8 +98,8 @@ topology_template:
                   #token: { get_property: [ HOST, flavor_name ] }
           interfaces:
             Maintenance:
    -          enable: juju > charm.maintenance_on
    -          disable: juju > charm.maintenance_off
    +          enable: juju \> charm.maintenance_on
    --- End diff --
    
    I'm changing this back. :) Will explain the new behavior on Slack.


---
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 #95: ARIA-92 Automatic operation task confi...

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

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


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111147368
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -522,17 +524,10 @@ def properties(cls):
         __mapper_args__ = {'version_id_col': version} # Enable SQLAlchemy automatic version counting
     
         @property
    -    def ip(self):
    -        # TODO: totally broken
    -        if not self.host_fk:
    -            return None
    -        host_node = self.host
    -        if 'ip' in host_node.runtime_properties:  # pylint: disable=no-member
    -            return host_node.runtime_properties['ip']  # pylint: disable=no-member
    -        host_node = host_node.node_template  # pylint: disable=no-member
    -        host_ip_property = host_node.properties.get('ip')
    -        if host_ip_property:
    -            return host_ip_property.value
    +    def host_address(self):
    +        if self.host:
    --- End diff --
    
    &&? :P


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111162301
  
    --- Diff: tests/modeling/test_models.py ---
    @@ -583,48 +583,49 @@ def test_node_model_creation(self, node_template_storage, is_valid, name, runtim
                        node_template_storage.service.list()[0]
     
     
    -class TestNodeIP(object):
    +class TestNodeHostAddress(object):
    --- 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111196059
  
    --- Diff: aria/orchestrator/workflows/api/task.py ---
    @@ -66,98 +66,84 @@ def __init__(self,
                      actor_type,
                      interface_name,
                      operation_name,
    -                 runs_on=None,
    +                 inputs=None,
                      max_attempts=None,
                      retry_interval=None,
    -                 ignore_failure=None,
    -                 inputs=None):
    +                 ignore_failure=None):
             """
             Do not call this constructor directly. Instead, use :meth:`for_node` or
             :meth:`for_relationship`.
             """
     
    -        assert isinstance(actor, (models.Node, models.Relationship))
    -        assert actor_type in ('node', 'relationship')
             assert interface_name and operation_name
    -        assert runs_on in models.Task.RUNS_ON
             super(OperationTask, self).__init__()
     
    +        operation = None
    +        interface = actor.interfaces.get(interface_name)
    +        if interface is not None:
    +            operation = interface.operations.get(operation_name)
    +
    +        if operation is None:
    +            raise exceptions.OperationNotFoundException(
    +                'Could not find operation "{0}" on interface "{1}" for {2} "{3}"'
    +                .format(operation_name, interface_name, actor_type, actor.name))
    +
    +        if operation.implementation is None:
    --- End diff --
    
    What is the meaning of an empty implementation operation? The operation would fail in the orchestrator, seems much better to catch that here and not create that 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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111168470
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -1848,11 +1856,30 @@ def as_raw(self):
     
         def instantiate(self, container):
             from . import models
    +        from ..orchestrator import context
    +        plugin = None
    +        if self.plugin_specification is not None:
    +            # TODO: we are planning a separate "instantiation" module where this will be called or
    +            # moved to. There, we will probably have a context with a storage manager. Until then,
    +            # this is the only potentially available context, which of course will only be available
    +            # if we're in a workflow.
    +            plugin = None
    --- End diff --
    
    redundant


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111152020
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -1685,12 +1725,31 @@ def inputs(cls):
         # 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))
         executor = Column(Text)
         max_retries = Column(Integer)
         retry_interval = Column(Integer)
     
    +    def configure(self):
    +        from . import models
    +        if (self.implementation is None) or (self.interface is None):
    +            return
    +
    +        if self.plugin is None:
    +            arguments = configure_operation(self)
    --- End diff --
    
    please use `execution_plugin.instantiation.configure_operation` or so, there are enough `configure_operation` calls in this module as it is and it's easily confusing :)


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111183546
  
    --- Diff: aria/orchestrator/workflows/api/task.py ---
    @@ -66,98 +66,84 @@ def __init__(self,
                      actor_type,
                      interface_name,
                      operation_name,
    -                 runs_on=None,
    +                 inputs=None,
                      max_attempts=None,
                      retry_interval=None,
    -                 ignore_failure=None,
    -                 inputs=None):
    +                 ignore_failure=None):
             """
             Do not call this constructor directly. Instead, use :meth:`for_node` or
             :meth:`for_relationship`.
             """
     
    -        assert isinstance(actor, (models.Node, models.Relationship))
    -        assert actor_type in ('node', 'relationship')
             assert interface_name and operation_name
    -        assert runs_on in models.Task.RUNS_ON
             super(OperationTask, self).__init__()
     
    +        operation = None
    +        interface = actor.interfaces.get(interface_name)
    +        if interface is not None:
    +            operation = interface.operations.get(operation_name)
    +
    +        if operation is None:
    --- End diff --
    
    (FYI this section is refactored elsewhere in the CLI branch, so it will get overridden here too)


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r111367789
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/__init__.py ---
    @@ -677,21 +698,47 @@ def pattern(node_type, container): # pylint: disable=unused-argument
         return None
     
     
    -def parse_implementation_string(context, service_template, implementation):
    -    if not implementation:
    -        return None, ''
    +def split_prefix(string):
    +    """
    +    Splits the prefix on the first unescaped ">".
    +    """
     
    -    index = implementation.find('>')
    -    if index == -1:
    -        return None, implementation
    -    plugin_name = implementation[:index].strip()
    -    
    -    if plugin_name == 'execution':
    -        plugin_specification = None
    -    else:
    -        plugin_specification = service_template.plugin_specifications.get(plugin_name)
    -        if plugin_specification is None:
    -            raise ValueError('unknown plugin: "{0}"'.format(plugin_name))
    +    split = IMPLEMENTATION_PREFIX_REGEX.split(string, 2)
    +    if len(split) < 2:
    +        return None, string
    +    return split[0].strip(), split[1].lstrip()
    +
    +
    +def set_nested(the_dict, keys, value):
    +    """
    +    If the ``keys`` list has just one item, puts the value in the the dict. If there are more items,
    +    puts the value in a sub-dict, creating sub-dicts as necessary for each key.
     
    -    implementation = implementation[index+1:].strip()
    -    return plugin_specification, implementation
    +    For example, if ``the_dict`` is an empty dict, keys is ``['first', 'second', 'third']`` and
    +    value is ``'value'``, then the_dict will be: ``{'first':{'second':{'third':'value'}}}``.
    +
    +    :param the_dict: Dict to change
    +    :type the_dict: {}
    +    :param keys: Keys
    +    :type keys: [basestring]
    +    :param value: Value
    +    """
    +    key = keys.pop(0)
    +    if len(keys) == 0:
    +        the_dict[key] = value
    +    else:
    +        if key not in the_dict:
    +            the_dict[key] = StrictDict(key_class=basestring)
    +        set_nested(the_dict[key], keys, value)
    +
    +
    +def parse_implementation_string(context, service_template, presentation, model, implementation):
    +    plugin_name, model.implementation = split_prefix(implementation)
    +    if plugin_name is not None:
    +        model.plugin_specification = service_template.plugin_specifications.get(plugin_name)
    +        if model.plugin_specification is None:
    +            context.validation.report(
    +                'no policy for plugin "%s" specified in operation implementation: %s'
    --- End diff --
    
    use string format please :sweat_smile: 


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109884259
  
    --- Diff: tests/resources/service-templates/tosca-simple-1.0/node-cellar/node-cellar.yaml ---
    @@ -155,14 +155,34 @@ topology_template:
                     - scalable:
                         properties:
                           - max_instances: { greater_or_equal: 8 }
    -    
    +            relationship:
    +              interfaces:
    +                Configure:
    +                  target_changed:
    +                    implementation:
    +                      primary: changed.sh
    +                      dependencies:
    +                        - edge > target
    --- End diff --
    
    remove


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109878912
  
    --- Diff: aria/orchestrator/workflows/api/task.py ---
    @@ -65,86 +65,82 @@ def __init__(self,
                      actor,
                      actor_type,
                      interface_name,
    -                 operation_name,
    -                 runs_on=None,
    +                 operation_name, # remove configuration
    +                 inputs=None,
                      max_attempts=None,
                      retry_interval=None,
    -                 ignore_failure=None,
    -                 inputs=None):
    +                 ignore_failure=None):
             """
             Do not call this constructor directly. Instead, use :meth:`for_node` or
             :meth:`for_relationship`.
             """
     
    -        assert isinstance(actor, (models.Node, models.Relationship))
    -        assert actor_type in ('node', 'relationship')
             assert interface_name and operation_name
    -        assert runs_on in models.Task.RUNS_ON
             super(OperationTask, self).__init__()
     
             self.actor = actor
    +        self.actor_type = actor_type
    --- End diff --
    
    why are any changes needed here (besides runs_on)?


---
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 #95: ARIA-92 Automatic operation task confi...

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

    https://github.com/apache/incubator-ariatosca/pull/95#discussion_r109899936
  
    --- Diff: aria/modeling/service_instance.py ---
    @@ -529,6 +524,32 @@ def validate_capabilities(self):
                     satisfied = False
             return satisfied
     
    +    def find_host(self):
    +        def _find_host(node):
    +            if node.type.role == 'host':
    +                return node
    +            for the_relationship in node.outbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    +                    the_relationship.target_capability.type.role == 'host':
    +                    host = _find_host(the_relationship.target_node)
    +                    if host is not None:
    +                        return host
    +            for the_relationship in node.inbound_relationships:
    +                if (the_relationship.target_capability is not None) and \
    --- End diff --
    
    this seems odd, target_capability is basicaly self.capabilities, isn't it? why would i need to traverse all the inbound relationships an test each ones target_capability?


---
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 #95: ARIA-92 Automatic operation task confi...

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/95#discussion_r109884741
  
    --- Diff: aria/orchestrator/execution_plugin/instantiation.py ---
    @@ -0,0 +1,188 @@
    +# Licensed to the Apache Software Foundation (ASF) 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.
    +
    +from ...utils.formatting import full_type_name
    +from ...utils.collections import OrderedDict
    +from ...parser import validation
    +from ...parser.consumption import ConsumptionContext
    +
    +
    +def configure_operation(operation):
    --- End diff --
    
    almost everything in this module needs to go. this method is okay, the rest should be transparent to 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.
---