-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(frontend): Support namespaced pipelines from the UI. Closes #5084 #8831
feat(frontend): Support namespaced pipelines from the UI. Closes #5084 #8831
Conversation
991e177
to
46ff7ce
Compare
46ff7ce
to
9f328a7
Compare
Thank you for your PR, we need to hide the |
@zijianjoy currently, the radio buttons and the tabs are hidden in single-user mode. You are suggesting that I should also take into account the |
Introduce the BuildInfo context and BuildInfoProvider class which is going pass the build information from the "/healthz" endpoint across the app. Signed-off-by: Tasos Alexiou <[email protected]>
Get the BuildInfo information from React's context instead of making the request to "/healthz" endpoint inside the SideNav component. Signed-off-by: Tasos Alexiou <[email protected]>
Add the namespace optional parameter when uploading a new pipeline. Signed-off-by: Tasos Alexiou <[email protected]>
Extend MD2Tabs component and add support for optional tooltips in tab's headers. Signed-off-by: Tasos Alexiou <[email protected]>
Make title property in ResourceSelector optional and hide the toolbar if it is not defined. Signed-off-by: Tasos Alexiou <[email protected]>
Add namespace as an option property in the PipelineList component. When the namespace is available the PipelineList component will display the namespaced pipelines. Signed-off-by: Tasos Alexiou <[email protected]>
Remove the UploadPipelineDialog from PipelineList component since it is no longer used. Signed-off-by: Tasos Alexiou <[email protected]>
Introduce the NamespacedAndSharedPipelines component which replaces the PipelineList component as the main component for the pipelines list page. This component displays both namespace and shared pipelines using tabs. Also fix the menu highlighting for the new page. Signed-off-by: Tasos Alexiou <[email protected]>
Introduce the PrivateSharedSelector component that is going to help if a new pipeline will be namespaced or shared. Signed-off-by: Tasos Alexiou <[email protected]>
Introduce the PipelinesDialog component which is going to be used to display both namespaced and shared pipelines using tabs. Signed-off-by: Tasos Alexiou <[email protected]>
Update the NewPipelineVersion component and add support to create namespaced pipelines. Signed-off-by: Tasos Alexiou <[email protected]>
Use the PrivateSharedSelector in the upload pipeline dialog and improve the wording in the helper texts. A user can now select if the new pipeline is going to be in the current namespace or shared. Signed-off-by: Tasos Alexiou <[email protected]>
Update the NewRun component and add support to create namespaced pipelines when creating a new run. Use the pipelines dialog when choosing namespaced or shared pipelines. Finally, disable the pipeline version chooser when there is no pipeline selected. Signed-off-by: Tasos Alexiou <[email protected]>
Signed-off-by: Tasos Alexiou <[email protected]>
46ff7ce
to
f315b65
Compare
if (!buildInfo?.apiServerMultiUser) { | ||
return <PipelineList {...props} />; | ||
} |
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.
If this is multi-user mode, I think we should show private and shared tab. The condition here shows a different logic. Would you like to explain the reason here?
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.
When apiServerMultiUser
is false
then we are not in multi-user mode and that is why we display a single tab.
return ( | ||
<div className={classes(commonCss.page, padding(20, 't'))}> | ||
<MD2Tabs | ||
tabs={[ | ||
{ | ||
header: PipelineTabsHeaders.PRIVATE, | ||
tooltip: PipelineTabsTooltips.PRIVATE, | ||
}, | ||
{ | ||
header: PipelineTabsHeaders.SHARED, | ||
tooltip: PipelineTabsTooltips.SHARED, | ||
}, | ||
]} | ||
selectedTab={props.view} | ||
onSwitch={tabSwitched} | ||
/> | ||
|
||
{props.view === PrivateAndSharedTab.PRIVATE && <PrivatePipelineList {...props} />} | ||
|
||
{props.view === PrivateAndSharedTab.SHARED && <PipelineList {...props} />} | ||
</div> | ||
); |
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 part of logics will appear when it is not multi-user mode.
However, there is no concept of private
in standalone mode.
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 section is displayed when apiServerMultiUser
is set to true
, which means we are in a multi-user mode and we want to display the private and shared tab.
@@ -176,6 +167,8 @@ class PipelineList extends Page<{}, PipelineListState> { | |||
request.pageSize, | |||
request.sortBy, | |||
request.filter, | |||
this.props.namespace ? 'NAMESPACE' : undefined, |
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.
I am not sure if the backend has already implemented the logics to support namespace-based listPipelines API call. cc @chensun to confirm.
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.
Namespaced pipelines are supported since #4835 PR.
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
/approve
Thank you Tasos for the change! I only have one comment which can be a subsequent PR. Approving the current PR.
</BuildInfoContext.Provider>, | ||
); | ||
await TestUtils.flushPromises(); | ||
expect(tree).toMatchSnapshot(); |
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 are moving away from snapshot test and prefer to validate certain UI component and actions in code. But we can fix it in subsequent PRs.
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.
@zijianjoy I noticed that in the codebase both enzyme and testing-library are used. Do you accept both of them or do you have a preference for new tests?
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.
React testing library is preferred for writing new UI tests. Enzyme was used in the past but it only checks for the snapshot of a frontend page, without validating the UI interaction with user behavior. But since it is a large effort to migrate, we prefer to execute it gradually. Whenever we write new tests or update old tests, we should use React testing library to test critical use cases if possible.
Reference: #5118 (comment)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zijianjoy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
…flow#5084 (kubeflow#8831) * feat(frontend): Introduce BuildInfo context Introduce the BuildInfo context and BuildInfoProvider class which is going pass the build information from the "/healthz" endpoint across the app. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Use BuildInfo context in SideNav Get the BuildInfo information from React's context instead of making the request to "/healthz" endpoint inside the SideNav component. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Add namespace optional parameter Add the namespace optional parameter when uploading a new pipeline. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Add tooltips in MD2Tabs component Extend MD2Tabs component and add support for optional tooltips in tab's headers. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Make title property in ResourceSelector optional Make title property in ResourceSelector optional and hide the toolbar if it is not defined. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Use namespace when displaying pipelines Add namespace as an option property in the PipelineList component. When the namespace is available the PipelineList component will display the namespaced pipelines. Signed-off-by: Tasos Alexiou <[email protected]> * chore(frontend): Remove UploadPipelineDialog from PipelineList Remove the UploadPipelineDialog from PipelineList component since it is no longer used. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Introduce tabbed pipelines list page Introduce the NamespacedAndSharedPipelines component which replaces the PipelineList component as the main component for the pipelines list page. This component displays both namespace and shared pipelines using tabs. Also fix the menu highlighting for the new page. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Introduce PrivateSharedSelector component Introduce the PrivateSharedSelector component that is going to help if a new pipeline will be namespaced or shared. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Introduce pipelines dialog Introduce the PipelinesDialog component which is going to be used to display both namespaced and shared pipelines using tabs. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Add support for namespaced pipeline creation Update the NewPipelineVersion component and add support to create namespaced pipelines. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Update upload pipeline dialog Use the PrivateSharedSelector in the upload pipeline dialog and improve the wording in the helper texts. A user can now select if the new pipeline is going to be in the current namespace or shared. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Add support for namespace pipelines in runs Update the NewRun component and add support to create namespaced pipelines when creating a new run. Use the pipelines dialog when choosing namespaced or shared pipelines. Finally, disable the pipeline version chooser when there is no pipeline selected. Signed-off-by: Tasos Alexiou <[email protected]> * feat(frontend): Add support for namespace pipelines in runs V2 Signed-off-by: Tasos Alexiou <[email protected]> --------- Signed-off-by: Tasos Alexiou <[email protected]>
The purpose of this PR is to introduce namespaced pipelines in the pipelines frontend.
Design doc: https://docs.google.com/document/d/1fM4y2L1IVqVj-iiNjYFRRktdCh7FQXgU2XpaYLaqt-A/edit?resourcekey=0-kd5loyP7w3PBD0ug6ECmLQ#heading=h.x9snb54sjlu9
The pipelines list page now has two tabs, one that displays the private pipelines and one for shared.
The new run page now includes radio buttons that help create a private or shared pipeline.
Similar to the pipelines list, the choose pipeline dialog has two tabs.
Finally, the upload pipeline dialog includes radio buttons for a private and shared pipeline.
In single-user mode, the pipelines list page and the choose pipeline dialog have no tabs. The radio buttons should also be hidden.
Part of #4197
Closes #5084
/cc @jlyaoyuli @zijianjoy @chensun @StefanoFioravanzo @elikatsis