onecommons / unfurl

A command line tool for deploying services and applications using git.
https://docs.unfurl.run
MIT License
98 stars 14 forks source link

Uniqueness of Type Names #341

Open milesstoetzner opened 2 months ago

milesstoetzner commented 2 months ago

Are type names unique per type collection (node types, artifact types, interface types, etc.) or per service template?

I am currently developing my own normative types. Thereby, I also define some root types for each type collection and call them all root. I assumed that even though these types have the same name, they are distinct since they are from different collections. However, this does not seem to be the case in Unfurl.

I did not find anything regarding the uniqueness of type names in the TOSCA spec.

Nevertheless, the current behaviour of Unfurl seems odd to me. There should be at least an error when two types have the same name (and not overridden?).

Here is a simple example.

# Ensemble A
apiVersion: unfurl/v1alpha1
kind: Ensemble
spec:
  service_template:
    artifact_types:
      root: 
        derived_from: tosca.artifacts.Root

      my_artifact_type:
        derived_from: root

    interface_types:
      root: 
        derived_from: tosca.interfaces.Root
        metadata: 
          key: value

    node_types:
      my_app:
        derived_from: tosca.nodes.Root

    topology_template:
      node_templates:
        my_app:
          type: my_app

          artifacts:
            my_artifact:
              type: my_artifact_type
              file: my_artifact_file

Validation will fail as follows since due to some reason interface_types.root is treated as artifact_types.root and, hence, throws the validation error mentioned in https://github.com/onecommons/unfurl/issues/340.

(.venv) stoetzms@storm:~/unfurl-issue$ unfurl validate
   INFO   UNFURL Using home project at: "/home/stoetzms/.unfurl_home/unfurl.yaml"
   INFO   UNFURL Loaded project at /home/stoetzms/unfurl-issue/unfurl.yaml
   INFO   UNFURL Vault password found, configuring vault ids: ['unfurl-issueVM', '.unfurl_homeF0']
   INFO   UNFURL Validating TOSCA template at /home/stoetzms/unfurl-issue/ensemble/ensemble.yaml
Exiting with error: TOSCA validation failed for /home/stoetzms/unfurl-issue/ensemble/ensemble.yaml:
UnknownFieldError: Artifacttype "root" contains unknown field "metadata". Refer to the definition to verify valid values. in node template "my_app"
UnknownFieldError: Artifacttype "root" contains unknown field "metadata". Refer to the definition to verify valid values. in node template "my_app"

Having unique names solves this issue.

# Ensemble B
apiVersion: unfurl/v1alpha1
kind: Ensemble
spec:
  service_template:
    artifact_types:
      root: 
        derived_from: tosca.artifacts.Root

      my_artifact_type:
        derived_from: root

    interface_types:
      interfaces.root:  # <------------ renamed this
        derived_from: tosca.interfaces.Root
        metadata: 
          key: value

    node_types:
      my_app:
        derived_from: tosca.nodes.Root

    topology_template:
      node_templates:
        my_app:
          type: my_app

          artifacts:
            my_artifact:
              type: my_artifact_type
              file: my_artifact_file

I tried this on Unfurl v1.0.0 and v1.1.0.

Best regards Miles

aszs commented 2 weeks ago

The tosca parser puts them all in one namespace and it isn't an error to reassign, though it should probably be a warning at least. If you think of tosca as a programing language, most languages declaring name binding in one scope will just shadow or override the same name declared in another scope. I think that's the reasoning at least... Regardless, changing this would be a breaking change, but I'll keep this issue open to at least warn, or maybe there could be a strict mode that you could opt-in to, maybe via metadata on the service template.

milesstoetzner commented 2 weeks ago

Thanks for the insights.

though it should probably be a warning at least

A warning would be great.

but I'll keep this issue open to at least warn

I am also fine with closing this issue as long as you do not intend to implement anything regarding this issue, e.g., the warning. A closed issue can still be found via the search.