-
Notifications
You must be signed in to change notification settings - Fork 360
Fix bug with propagation of node caching property #3012
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
Conversation
|
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
0395165 to
9707a12
Compare
Signed-off-by: Kiersten Stokes <[email protected]>
Signed-off-by: Kiersten Stokes <[email protected]>
Signed-off-by: Kiersten Stokes <[email protected]>
Signed-off-by: Kiersten Stokes <[email protected]>
Signed-off-by: Kiersten Stokes <[email protected]>
29a4181 to
36eb293
Compare
kevin-bates
left a comment
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 looks good. My only comment is regarding the dual types that need to be handled (which is kind of a hassle) and just wondering if this is historical (and avoids a migration).
elyra/tests/pipeline/resources/sample_pipelines/pipeline_valid_with_pipeline_default.json
Show resolved
Hide resolved
Signed-off-by: Kiersten Stokes <[email protected]>
ptitzler
left a comment
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!
kevin-bates
left a comment
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
Fixes #3010
What changes were proposed in this pull request?
DisableNodeCachingcurrently requires special handling, as it is our only Elyra-owned property that is not stored and processed as an object. As a result, values weren't being stored in their instances properly and therefore weren't propagating properly. This PR adds ashould_discardmethod toDisableNodeCachingin order to: 1.) fix the propagation issue now, and 2.) prepare for when this property is finally converted into an object-valued property in the pipeline JSON.How was this pull request tested?
I added a case to an existing test because this has unfortunately come up before. We could potentially remove this case if desired once we switch the handling/storage of
DisableNodeCachingto an object-valued properly. Instead, however, we should probably have a test case for each Elyra-owned property.Developer's Certificate of Origin 1.1