-
Notifications
You must be signed in to change notification settings - Fork 349
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
Add support for pipeline properties #1708
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
@marthacryan Pipeline Editor 0.4.0 has been published now. |
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
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.
Big bug where the api in infinitely called once the properties is opened, otherwise the code looks good.
The actual properties aren't functional though, specifically env vars and default runtime. I don't think we should add those properties without adding a use case for them. So either we should remove them for now or implement an initial use case for them in this PR. Though I am also unsure how @lresende intended to leverage them
Fixed! |
changeHandler(); | ||
}) | ||
.catch(error => RequestErrors.serverError(error)); | ||
|
||
return (): void => { | ||
currentContext.model.contentChanged.disconnect(changeHandler); | ||
}; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
We shouldn't ignore these warnings
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.
After taking a closer look at this, the pipelineProperties shouldn't be a state. We should only store the runtime images in the state and that way we can derive the properties without rerunning the effect
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.
It caused an infinite loop when I added that dependency - what would you suggest as the solution for that?
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 should be addressed in a follow-up PR that adds support for default runtime image - for now, this issue doesn't need to be addressed yet.
What's the point of adding properties that have no effect? If there's no backend support they should not be exposed in the UI. |
Removing the env var and default runtime image fields - planning for those to be added in a follow-up PR. |
summary of the follow up plan for runtime images: The backend will handle filling in the default runtime for nodes that are missing it. Also validation will only flag empty runtimes if a default is also not set. |
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
What changes were proposed in this pull request?
Adds the ability to edit pipeline properties (as opposed to node properties). Adds a tab to the right panel in the pipeline editor for the pipeline properties editor.
This relies on elyra-ai/pipeline-editor#122 being merged / released, so I'll keep the blocked label / draft state until that's released. For now, it uses the code sandbox link so that testing is easier.
Appearance:

Todo:
Fixes #1699
Fixes #1421
Closes #602
Developer's Certificate of Origin 1.1