opensearch-project / dashboards-flow-framework

A UI designer for constructing AI applications with OpenSearch
Apache License 2.0
9 stars 6 forks source link

Support MDS on workflows and workflow_detail pages #253

Closed saimedhi closed 3 weeks ago

saimedhi commented 1 month ago

Description

Issues Resolved

closes #228

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

saimedhi commented 1 month ago

https://github.com/user-attachments/assets/0c5091fe-4aba-4685-8613-1092f4071861

jackiehanyang commented 1 month ago

Could you mark the PR as a draft until you have completed the end-to-end testing? It includes:

jackiehanyang commented 1 month ago

Thanks for the change! Two things I noticed from the screen recording:

  1. I don't see dataSourceId= in the url on new_workflow page when local cluster is selected. Let's add that to keep all pages url in consistent when the feature flag is enabled
  2. Could you click on the delete and link icon button on the Workflows page and to see if they all work as expected?
ohltyler commented 1 month ago

@saimedhi looking great! Once my remaining comments on public/route_service.ts are addressed and you can confirm all of the functionality is still working as expected, I am happy to approve :)

saimedhi commented 1 month ago

screen-capture (2).webm

ohltyler commented 1 month ago

Changes LGTM! One more request to add details in the PR description, including some of the low-level implementation details. See existing merged PRs as an example. This makes it much easier to track down issues that may come up in the future, and keeps a detailed log of what and when changes were made, and the motivation behind them.

saimedhi commented 1 month ago

Remote data source should be consistent in breadcrumbs. When go back to workflow page, local cluster should be selected instead of selecting the default data source. Could you address this bug?

@jackiehanyang, I will fix it in a followup PR. Thank you.

saimedhi commented 1 month ago

Remote data source should be consistent in breadcrumbs. When go back to workflow page, local cluster should be selected instead of selecting the default data source. Could you address this bug?

@jackiehanyang, I will fix it in a followup PR. Thank you.

  • I will make sure when we go back to workflow page, data source selected will be retained .

screen-capture (3).webm

jackiehanyang commented 1 month ago

I see a bug - when you opening flow framework from the side nav, default remote data source should be selected and the dataSourceId should exist in the url

Screenshot 2024-08-08 at 11 34 59

Please thoroughly test the following scenarios: