-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare to make all Elyra-owned properties object-valued #2957
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refactoring is great! Just had a few minor comments and nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff - thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Supports #2942. This PR to be merged before merge of the shared memory PR.
What changes were proposed in this pull request?
This PR introduces some implementation details that simplify the addition of new properties by making all properties object-valued. The bulk of the change is the renaming of the
ui_details_map
toattribute_details
, and also includes a major change to the data structure. Whereas before this structure was a dictionary, it is now a list ofPropertyAttribute
objects, each of which contains the information needed to render and process a single property attribute (e.g. "env_var" and "value" are thePropertyAttribute
s for theEnvironmentVariable
class). Additionally, the creation of the properties schema now assumes that all properties are eitherobject
s orarray
s (with the exception ofDisableNodeCaching
, explained below). All the above allows us to generalize, and removes the need for acreate_instance
method on each class.Note that the
DisableNodeCaching
property is still single-valued for now. The reason being that fully converting this property to an object would require a pipeline migration, and we would like to avoid a migration so soon after the 3.12 migration, if possible.No changes to the frontend are included or necessary, unless we decide that we want to migrate
DisableNodeCaching
.Finally, a distinction is made between Elyra-owned properties (those with a corresponding
ElyraProperty
class) and properties that should be propagated. There is currently only one case where the set of propagated properties != the set of Elyra-owned properties, which is the propagation ofruntime_image
for generic components. This is handled in a new property on theNode
object calledpropagated_properties
, which is accessed in the node propagation loop.How was this pull request tested?
Manually tested several scenarios. Changes are self-contained and only one minor test update was required.
Developer's Certificate of Origin 1.1