You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ariatosca.apache.org by tliron <gi...@git.apache.org> on 2017/06/01 19:20:50 UTC

[GitHub] incubator-ariatosca pull request #143: ARIA-254 Create of multiple nodes per...

GitHub user tliron opened a pull request:

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

    ARIA-254 Create of multiple nodes per template

    * New aria.Scaling policy (and "scaling" role)
    * NodeTemplate model no longer stores scaling values (default_instances,
    etc.) but instead fetches them from applicable scaling policies
    * Some code cleanup

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

    $ git pull https://github.com/apache/incubator-ariatosca ARIA-254-multiple-nodes-per-template

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

    https://github.com/apache/incubator-ariatosca/pull/143.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 #143
    
----
commit acfc7f461b7ee6f01b6095d8eb4e28554df8928c
Author: Tal Liron <ta...@gmail.com>
Date:   2017-06-01T19:17:17Z

    ARIA-254 Create of multiple nodes per template
    
    * New aria.Scaling policy (and "scaling" role)
    * NodeTemplate model no longer stores scaling values (default_instances,
    etc.) but instead fetches them from applicable scaling policies
    * Some code cleanup

----


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

[GitHub] incubator-ariatosca pull request #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r126353859
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def scaling(self):
    +        scaling = {}
    +
    +        def extract_property(properties, name):
    +            if name in scaling:
    +                return
    +            prop = properties.get(name)
    +            if (prop is not None) and (prop.type_name == 'integer') and (prop.value is not None):
    +                scaling[name] = prop.value
    +
    +        def extract_properties(properties):
    +            extract_property(properties, 'min_instances')
    +            extract_property(properties, 'max_instances')
    +            extract_property(properties, 'default_instances')
    +
    +        def default_property(name, value):
    +            if name not in scaling:
    +                scaling[name] = value
    +
    +        # From our scaling capabilities
    +        for capability_template in self.capability_templates.itervalues():
    +            if capability_template.type.role == 'scaling':
    +                extract_properties(capability_template.properties)
    +
    +        # From service scaling policies
    +        for policy_template in self.service_template.policy_templates.itervalues():
    +            if policy_template.type.role == 'scaling':
    +                if policy_template.is_for_node_template(self.name):
    +                    extract_properties(policy_template.properties)
    +
    +        # Defaults
    +        default_property('min_instances', 0)
    +        default_property('max_instances', 1)
    +        default_property('default_instances', 1)
    +
    +        # Validate
    +        # pylint: disable=too-many-boolean-expressions
    +        if (scaling['min_instances'] < 0) or \
    --- End diff --
    
    I see no difference in readability between this and using `or`, seems to me just to be bowing to PyLint's weirdness... but I don't mind switching.


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125464412
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def scaling(self):
    +        scaling = {}
    +
    +        def extract_property(properties, name):
    +            if name in scaling:
    +                return
    +            prop = properties.get(name)
    +            if (prop is not None) and (prop.type_name == 'integer') and (prop.value is not None):
    +                scaling[name] = prop.value
    +
    +        def extract_properties(properties):
    +            extract_property(properties, 'min_instances')
    +            extract_property(properties, 'max_instances')
    +            extract_property(properties, 'default_instances')
    +
    +        def default_property(name, value):
    +            if name not in scaling:
    +                scaling[name] = value
    +
    +        # From our scaling capabilities
    +        for capability_template in self.capability_templates.itervalues():
    +            if capability_template.type.role == 'scaling':
    +                extract_properties(capability_template.properties)
    +
    +        # From service scaling policies
    +        for policy_template in self.service_template.policy_templates.itervalues():
    +            if policy_template.type.role == 'scaling':
    +                if policy_template.is_for_node_template(self.name):
    +                    extract_properties(policy_template.properties)
    +
    +        # Defaults
    +        default_property('min_instances', 0)
    +        default_property('max_instances', 1)
    +        default_property('default_instances', 1)
    +
    +        # Validate
    +        # pylint: disable=too-many-boolean-expressions
    +        if (scaling['min_instances'] < 0) or \
    --- End diff --
    
    may any? and remove the pylint issue?


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125463273
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    --- End diff --
    
    so is 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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125755316
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    --- End diff --
    
    Actually, this was intentional -- I thought it could be useful for outside callers. But no problem changed this and also `_next_name` similarly.


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125755887
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    +    raw_properties = OrderedDict()
    +
    +    # Merge properties from type
    +    from_property_defintions = from_capability_definition.properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties, from_property_defintions,
    +                                    'properties')
    +
    +    # Merge our properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties,
    +                                    capability_definition.properties, 'properties')
    --- End diff --
    
    Not exactly. This is a utility method called by `get_inherited_capability_definitions`. Its goal it to merge the current node type's capability definitions over the inherited parent node type's capability definitions. "from" here is the parent's.


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125460359
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    +    raw_properties = OrderedDict()
    +
    +    # Merge properties from type
    +    from_property_defintions = from_capability_definition.properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties, from_property_defintions,
    +                                    'properties')
    +
    +    # Merge our properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties,
    +                                    capability_definition.properties, 'properties')
    +
    +    if raw_properties:
    +        capability_definition._raw['properties'] = raw_properties
    +        capability_definition._reset_method_cache()
    +
    +    # Merge occurrences
    +    occurrences = from_capability_definition._raw.get('occurrences')
    +    if (occurrences is not None) and (capability_definition._raw.get('occurrences') is None):
    --- End diff --
    
    why the redundant parenthesis? 


---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120455950
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -613,6 +597,56 @@ def dump(self):
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def default_instances(self):
    +        # TODO: currently finds the first matching policy; but we should emit a validation error
    +        # if more than one policy applies to the same node
    +        for policy_template in self.service_template.policy_templates.itervalues():
    +            if policy_template.type.role == 'scaling':
    +                if policy_template.is_for_node_template(self.name):
    +                    default_instances = policy_template.properties.get('default_instances')
    +                    if (default_instances is not None) \
    +                        and (default_instances.type_name == 'integer'):
    --- End diff --
    
    We cannot expect anybody be to using the ARIA profile as is -- all we are relying here is that there is an attribute with that name. I am adding that extra validation of type to make sure that we don't cause breakage by somebody messing with the parameters. Note that we also do this validation in setting "default" attributes, like `tosca_name` and `tosca_id`.


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125462685
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    +    raw_properties = OrderedDict()
    +
    +    # Merge properties from type
    +    from_property_defintions = from_capability_definition.properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties, from_property_defintions,
    +                                    'properties')
    +
    +    # Merge our properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties,
    +                                    capability_definition.properties, 'properties')
    --- End diff --
    
    if i get this right, capability_definitions is the "global" definition of this capability, while "from..." if the definitions for this instance. 
    and we first apply our own definitions and only then try and merge the global one - while maintaining the idea that the more specific definitions will be the ones to hold?


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125851056
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    +    raw_properties = OrderedDict()
    +
    +    # Merge properties from type
    +    from_property_defintions = from_capability_definition.properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties, from_property_defintions,
    +                                    'properties')
    +
    +    # Merge our properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties,
    +                                    capability_definition.properties, 'properties')
    +
    +    if raw_properties:
    +        capability_definition._raw['properties'] = raw_properties
    +        capability_definition._reset_method_cache()
    +
    +    # Merge occurrences
    +    occurrences = from_capability_definition._raw.get('occurrences')
    +    if (occurrences is not None) and (capability_definition._raw.get('occurrences') is None):
    --- End diff --
    
    well i do agree when it comes to an elaborate conditions :)


---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120455451
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/types.py ---
    @@ -13,7 +13,7 @@
     # See the License for the specific language governing permissions and
     # limitations under the License.
     
    -from aria.utils.collections import FrozenDict, FrozenList
    +from aria.utils.collections import (FrozenDict, FrozenList)
    --- End diff --
    
    I thought we agreed that that kind of style is a recommendation but not a requirement. If we switch to that style here it would make a lot of the parser code extremely verbose and frankly unreadable.


---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120005524
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -613,6 +597,56 @@ def dump(self):
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def default_instances(self):
    +        # TODO: currently finds the first matching policy; but we should emit a validation error
    +        # if more than one policy applies to the same node
    +        for policy_template in self.service_template.policy_templates.itervalues():
    +            if policy_template.type.role == 'scaling':
    +                if policy_template.is_for_node_template(self.name):
    +                    default_instances = policy_template.properties.get('default_instances')
    +                    if (default_instances is not None) \
    +                        and (default_instances.type_name == 'integer'):
    --- End diff --
    
    how can it not be 'integer'?


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r126353998
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    --- End diff --
    
    Because I prefer it to fit with the style of the rest of code right now, to keep it consistent. I would like to change it, but everything together and give it some more thought.


---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120005412
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/types.py ---
    @@ -13,7 +13,7 @@
     # See the License for the specific language governing permissions and
     # limitations under the License.
     
    -from aria.utils.collections import FrozenDict, FrozenList
    +from aria.utils.collections import (FrozenDict, FrozenList)
    --- End diff --
    
    if adding parentheses, why not try and follow the style guide? i.e. `from aria.utils import collections`


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125851907
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    +    raw_properties = OrderedDict()
    +
    +    # Merge properties from type
    +    from_property_defintions = from_capability_definition.properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties, from_property_defintions,
    +                                    'properties')
    +
    +    # Merge our properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties,
    +                                    capability_definition.properties, 'properties')
    --- End diff --
    
    I do get the logic of the entire method, i was referring to the order of the `merge_raw_parameter_definitions`...


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125461970
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    --- End diff --
    
    the hard to follow comment applies here as well 


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125464166
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    --- End diff --
    
    All the new functions are seem to be for orchestrator use only. Why would the user need to use these?


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125852641
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def scaling(self):
    +        scaling = {}
    +
    +        def extract_property(properties, name):
    +            if name in scaling:
    +                return
    +            prop = properties.get(name)
    +            if (prop is not None) and (prop.type_name == 'integer') and (prop.value is not None):
    +                scaling[name] = prop.value
    +
    +        def extract_properties(properties):
    +            extract_property(properties, 'min_instances')
    +            extract_property(properties, 'max_instances')
    +            extract_property(properties, 'default_instances')
    +
    +        def default_property(name, value):
    +            if name not in scaling:
    +                scaling[name] = value
    +
    +        # From our scaling capabilities
    +        for capability_template in self.capability_templates.itervalues():
    +            if capability_template.type.role == 'scaling':
    +                extract_properties(capability_template.properties)
    +
    +        # From service scaling policies
    +        for policy_template in self.service_template.policy_templates.itervalues():
    --- End diff --
    
    Oh, didn't notice it. I would think that there is not real need to check, just override the inherited properties. But this is my personal preference 


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125457719
  
    --- Diff: extensions/aria_extension_tosca/profiles/tosca-simple-1.0/capabilities.yaml ---
    @@ -132,6 +132,7 @@ capability_types:
           specification: tosca-simple-1.0
           specification_section: 5.4.10
           specification_url: 'http://docs.oasis-open.org/tosca/TOSCA-Simple-Profile-YAML/v1.0/cos01/TOSCA-Simple-Profile-YAML-v1.0-cos01.html#DEFN_TYPE_CAPABILITIES_SCALABLE'
    +      role: scaling
    --- End diff --
    
    what does it mean to have a scaling role?


---
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 #143: ARIA-254 Create multiple nodes per te...

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

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


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125754130
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    +    raw_properties = OrderedDict()
    +
    +    # Merge properties from type
    +    from_property_defintions = from_capability_definition.properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties, from_property_defintions,
    +                                    'properties')
    +
    +    # Merge our properties
    +    merge_raw_parameter_definitions(context, presentation, raw_properties,
    +                                    capability_definition.properties, 'properties')
    +
    +    if raw_properties:
    +        capability_definition._raw['properties'] = raw_properties
    +        capability_definition._reset_method_cache()
    +
    +    # Merge occurrences
    +    occurrences = from_capability_definition._raw.get('occurrences')
    +    if (occurrences is not None) and (capability_definition._raw.get('occurrences') is None):
    --- End diff --
    
    I do not consider them redundant at all. Nobody should have to remember operation precedence, you should be able to tell at a glance. I recommend everybody follow this practice in every language. :)


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125754664
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    --- End diff --
    
    +1, but I suggest refactoring some other time


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125853047
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def scaling(self):
    +        scaling = {}
    +
    +        def extract_property(properties, name):
    +            if name in scaling:
    +                return
    +            prop = properties.get(name)
    +            if (prop is not None) and (prop.type_name == 'integer') and (prop.value is not None):
    +                scaling[name] = prop.value
    +
    +        def extract_properties(properties):
    +            extract_property(properties, 'min_instances')
    +            extract_property(properties, 'max_instances')
    +            extract_property(properties, 'default_instances')
    +
    +        def default_property(name, value):
    +            if name not in scaling:
    +                scaling[name] = value
    +
    +        # From our scaling capabilities
    +        for capability_template in self.capability_templates.itervalues():
    +            if capability_template.type.role == 'scaling':
    +                extract_properties(capability_template.properties)
    +
    +        # From service scaling policies
    +        for policy_template in self.service_template.policy_templates.itervalues():
    +            if policy_template.type.role == 'scaling':
    +                if policy_template.is_for_node_template(self.name):
    +                    extract_properties(policy_template.properties)
    +
    +        # Defaults
    +        default_property('min_instances', 0)
    +        default_property('max_instances', 1)
    +        default_property('default_instances', 1)
    +
    +        # Validate
    +        # pylint: disable=too-many-boolean-expressions
    +        if (scaling['min_instances'] < 0) or \
    --- End diff --
    
    ```
    if any([
     scaling['min_instances'] < 0,
     scaling['max_instances'] < 0,
     scaling['default_instances'] < 0,
     scaling['max_instances'] < scaling['min_instances'],
     scaling['default_instances'] < scaling['min_instances'],
     scaling['default_instances'] > scaling['max_instances']
    ]):
    ...
    ```



---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120455265
  
    --- Diff: tests/end2end/test_nodecellar.py ---
    @@ -39,4 +39,4 @@ def _verify_deployed_service_in_storage(service_name, model_storage):
         service = service_templates[0].services[service_name]
         assert service.name == service_name
         assert len(service.executions) == 0  # dry executions leave no traces
    -    assert len(service.nodes) == 10
    +    assert len(service.nodes) == 13
    --- End diff --
    
    The policy is on the `node_cellar_group` which contains three 3 templates. So we get 3 extra nodes.


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125756533
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def scaling(self):
    +        scaling = {}
    +
    +        def extract_property(properties, name):
    +            if name in scaling:
    +                return
    +            prop = properties.get(name)
    +            if (prop is not None) and (prop.type_name == 'integer') and (prop.value is not None):
    +                scaling[name] = prop.value
    +
    +        def extract_properties(properties):
    +            extract_property(properties, 'min_instances')
    +            extract_property(properties, 'max_instances')
    +            extract_property(properties, 'default_instances')
    +
    +        def default_property(name, value):
    +            if name not in scaling:
    +                scaling[name] = value
    +
    +        # From our scaling capabilities
    +        for capability_template in self.capability_templates.itervalues():
    +            if capability_template.type.role == 'scaling':
    +                extract_properties(capability_template.properties)
    +
    +        # From service scaling policies
    +        for policy_template in self.service_template.policy_templates.itervalues():
    +            if policy_template.type.role == 'scaling':
    +                if policy_template.is_for_node_template(self.name):
    +                    extract_properties(policy_template.properties)
    +
    +        # Defaults
    +        default_property('min_instances', 0)
    +        default_property('max_instances', 1)
    +        default_property('default_instances', 1)
    +
    +        # Validate
    +        # pylint: disable=too-many-boolean-expressions
    +        if (scaling['min_instances'] < 0) or \
    --- End diff --
    
    I'm not sure how `any` would work here. How would you write 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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125851301
  
    --- Diff: extensions/aria_extension_tosca/profiles/tosca-simple-1.0/capabilities.yaml ---
    @@ -132,6 +132,7 @@ capability_types:
           specification: tosca-simple-1.0
           specification_section: 5.4.10
           specification_url: 'http://docs.oasis-open.org/tosca/TOSCA-Simple-Profile-YAML/v1.0/cos01/TOSCA-Simple-Profile-YAML-v1.0-cos01.html#DEFN_TYPE_CAPABILITIES_SCALABLE'
    +      role: scaling
    --- 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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125756168
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def scaling(self):
    +        scaling = {}
    +
    +        def extract_property(properties, name):
    +            if name in scaling:
    +                return
    +            prop = properties.get(name)
    +            if (prop is not None) and (prop.type_name == 'integer') and (prop.value is not None):
    +                scaling[name] = prop.value
    +
    +        def extract_properties(properties):
    +            extract_property(properties, 'min_instances')
    +            extract_property(properties, 'max_instances')
    +            extract_property(properties, 'default_instances')
    +
    +        def default_property(name, value):
    +            if name not in scaling:
    +                scaling[name] = value
    +
    +        # From our scaling capabilities
    +        for capability_template in self.capability_templates.itervalues():
    +            if capability_template.type.role == 'scaling':
    +                extract_properties(capability_template.properties)
    +
    +        # From service scaling policies
    +        for policy_template in self.service_template.policy_templates.itervalues():
    --- End diff --
    
    No, the opposite. There is no overriding: the `extract_property` function will not extract if the value is already there. First to come wins. :)


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125853623
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -162,6 +164,30 @@ def convert_capability_from_definition_to_assignment(context, presentation, cont
         return CapabilityAssignment(name=presentation._name, raw=raw, container=container)
     
     
    +def merge_capability_definition(context, presentation, capability_definition,
    +                                from_capability_definition):
    --- End diff --
    
    well, i might go with it regarding the first naming issue, but this is a new function, why not rename its arguments properly? 


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125461884
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -52,12 +52,13 @@ def get_inherited_capability_definitions(context, presentation, for_presentation
         Allows overriding all aspects of parent capability properties except data type.
         """
     
    +    if for_presentation is None:
    +        for_presentation = presentation
    +
         # Get capability definitions from parent
         parent = presentation._get_parent(context)
    -    capability_definitions = get_inherited_capability_definitions(context, parent,
    -                                                                  for_presentation=presentation) \
    -                                                                  if parent is not None \
    -                                                                  else OrderedDict()
    +    capability_definitions = get_inherited_capability_definitions(
    +        context, parent, for_presentation) if parent is not None else OrderedDict()
     
         # Add/merge our capability definitions
         our_capability_definitions = presentation.capabilities
    --- End diff --
    
    it's really hard to follow with name like 
    capability_definitions and our_capability_definitions


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125753807
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/modeling/capabilities.py ---
    @@ -52,12 +52,13 @@ def get_inherited_capability_definitions(context, presentation, for_presentation
         Allows overriding all aspects of parent capability properties except data type.
         """
     
    +    if for_presentation is None:
    +        for_presentation = presentation
    +
         # Get capability definitions from parent
         parent = presentation._get_parent(context)
    -    capability_definitions = get_inherited_capability_definitions(context, parent,
    -                                                                  for_presentation=presentation) \
    -                                                                  if parent is not None \
    -                                                                  else OrderedDict()
    +    capability_definitions = get_inherited_capability_definitions(
    +        context, parent, for_presentation) if parent is not None else OrderedDict()
     
         # Add/merge our capability definitions
         our_capability_definitions = presentation.capabilities
    --- End diff --
    
    I tend to agree. :( This is some very tricky code that has gone through a lot of changes. I do want to fix this (there are a bunch of related files in this module) but I don't think this PR is the right place.


---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120559934
  
    --- Diff: extensions/aria_extension_tosca/simple_v1_0/types.py ---
    @@ -13,7 +13,7 @@
     # See the License for the specific language governing permissions and
     # limitations under the License.
     
    -from aria.utils.collections import FrozenDict, FrozenList
    +from aria.utils.collections import (FrozenDict, FrozenList)
    --- End diff --
    
    if by adding the module prefix makes the code unreadable, maybe we need to refactor the structure? 


---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120560207
  
    --- Diff: tests/end2end/test_nodecellar.py ---
    @@ -39,4 +39,4 @@ def _verify_deployed_service_in_storage(service_name, model_storage):
         service = service_templates[0].services[service_name]
         assert service.name == service_name
         assert len(service.executions) == 0  # dry executions leave no traces
    -    assert len(service.nodes) == 10
    +    assert len(service.nodes) == 13
    --- End diff --
    
    kewl


---
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 #143: ARIA-254 Create of multiple nodes per...

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/143#discussion_r120005385
  
    --- Diff: tests/end2end/test_nodecellar.py ---
    @@ -39,4 +39,4 @@ def _verify_deployed_service_in_storage(service_name, model_storage):
         service = service_templates[0].services[service_name]
         assert service.name == service_name
         assert len(service.executions) == 0  # dry executions leave no traces
    -    assert len(service.nodes) == 10
    +    assert len(service.nodes) == 13
    --- End diff --
    
    why 13? I though you added just one other 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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125463253
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    --- End diff --
    
    cannot remain public 


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125465104
  
    --- Diff: aria/modeling/service_template.py ---
    @@ -690,19 +666,104 @@ def dump(self):
                 console.puts(context.style.meta(self.description))
             with context.style.indent:
                 console.puts('Type: {0}'.format(context.style.type(self.type.name)))
    -            console.puts('Instances: {0:d} ({1:d}{2})'.format(
    -                self.default_instances,
    -                self.min_instances,
    -                ' to {0:d}'.format(self.max_instances)
    -                if self.max_instances is not None
    -                else ' or more'))
                 utils.dump_dict_values(self.properties, 'Properties')
                 utils.dump_dict_values(self.attributes, 'Attributes')
                 utils.dump_interfaces(self.interface_templates)
                 utils.dump_dict_values(self.artifact_templates, 'Artifact templates')
                 utils.dump_dict_values(self.capability_templates, 'Capability templates')
                 utils.dump_list_values(self.requirement_templates, 'Requirement templates')
     
    +    @property
    +    def next_index(self):
    +        """
    +        Next available node index.
    +
    +        :returns: node index
    +        :rtype: int
    +        """
    +
    +        max_index = 0
    +        if self.nodes:
    +            max_index = max(int(n.name.rsplit('_', 1)[-1]) for n in self.nodes)
    +        return max_index + 1
    +
    +    @property
    +    def next_name(self):
    +        """
    +        Next available node name.
    +
    +        :returns: node name
    +        :rtype: basestring
    +        """
    +
    +        return '{name}_{index}'.format(name=self.name, index=self.next_index)
    +
    +    @property
    +    def scaling(self):
    +        scaling = {}
    +
    +        def extract_property(properties, name):
    +            if name in scaling:
    +                return
    +            prop = properties.get(name)
    +            if (prop is not None) and (prop.type_name == 'integer') and (prop.value is not None):
    +                scaling[name] = prop.value
    +
    +        def extract_properties(properties):
    +            extract_property(properties, 'min_instances')
    +            extract_property(properties, 'max_instances')
    +            extract_property(properties, 'default_instances')
    +
    +        def default_property(name, value):
    +            if name not in scaling:
    +                scaling[name] = value
    +
    +        # From our scaling capabilities
    +        for capability_template in self.capability_templates.itervalues():
    +            if capability_template.type.role == 'scaling':
    +                extract_properties(capability_template.properties)
    +
    +        # From service scaling policies
    +        for policy_template in self.service_template.policy_templates.itervalues():
    --- End diff --
    
    so are we overriding the more specific properties with the policy based properties?


---
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 #143: ARIA-254 Create multiple nodes per te...

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/143#discussion_r125753620
  
    --- Diff: extensions/aria_extension_tosca/profiles/tosca-simple-1.0/capabilities.yaml ---
    @@ -132,6 +132,7 @@ capability_types:
           specification: tosca-simple-1.0
           specification_section: 5.4.10
           specification_url: 'http://docs.oasis-open.org/tosca/TOSCA-Simple-Profile-YAML/v1.0/cos01/TOSCA-Simple-Profile-YAML-v1.0-cos01.html#DEFN_TYPE_CAPABILITIES_SCALABLE'
    +      role: scaling
    --- End diff --
    
    The `role` meta data is the way to specify special behavior for types in ARIA. This is much safer than checking if name == `aria.Scaling`, or even if it inherits from that. Roles are inherited with the type, so subtypes will have the same role (unless they override it). We use this in a few cases, for example there is `host` role for Compute and for HostedOn relationships.


---
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.
---