Support key-value type list properties in a more generic way

[kiersten-stokes]

Fixes #2678

Supports #2675

What changes were proposed in this pull request?

First, key-value type properties are identified in the canvas JSON with the addition of a key keyValueEntries in the data stanza.

The list built-in type is extended in this PR in a class called KeyValueList. Key-value pair properties will have values of this type. The KeyValueList type has various methods that perform conversion of a list of character-separated key-value pairs (e.g. "key=value") to a dictionary and vice-versa. Node and pipeline key-value list properties identified with the keyValueEntries metadata mentioned above have their values converted ASAP, during the construction of the PipelineDefinition object. A new function coerce_kv_pair_properties is added that takes place before default property propagation that makes this conversion from list to KeyValueList so that later functions dealing with the processing of these values will succeed.

This allows for the removal of some duplicate code and explicit references to env_vars.

How was this pull request tested?

A new test case (test_convert_kv_properties) was added to ensure all key-value props are converted to KeyValueList types.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

[elyra-bot]

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link:

[kevin-bates]

This looks good so far. I had some suggestions that might make things cleaner from a class perspective.

[kevin-bates]

I understand stripping off the elyra_ prefix in order to check if this property is a KV pairs property, but that's only because the elyra_ prefix is stripped when building the kv_pair_properties list anyway. Since we don't appear to persist the non-prefixed values, can we forgo both of these "stripping" locations?

I'm not finding any place in the old code where these prefixes are removed so just wondering why we need to do that.

[kiersten-stokes]

Unfortunately I don't think we can. Here and in the function you linked, we're taking property names directly from the templates where the elyra_ prefix is present. The property name that we get during submit/export (and that we're checking for matches with the key-value pair property names), doesn't have the prefix - the frontend removes it at some point.

I wonder if we could forego the elyra_ prefix altogether at some point and just indicate this in the canvas JSON data stanza in some other way.

[kevin-bates]

I see. Ok, no worries.

I wonder if we could forego the elyra_ prefix altogether at some point and just indicate this in the canvas JSON data stanza in some other way.

This sounds like something that might be a helpful update at some point.
cc: @ajbozarth

[kiersten-stokes]

Moved this into WIP right now because I haven't been able to test the recent changes as thoroughly as I'd like to. I still have to add new test case(s) as well.

[kevin-bates]

LGTM - thank you @kiersten-stokes!

[ptitzler]

Perhaps I am missing something, but should this processing be still required https://github.com/kiersten-stokes/elyra/blob/generic-kv-props/elyra/pipeline/processor.py#L596-L604 when volume mounts are exposed as kv properties?