Skip to content
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

Support MDS on workflows and workflow_detail pages #253

Closed
wants to merge 15 commits into from

Conversation

saimedhi
Copy link
Collaborator

Description

  • Added support for using MDS on workflows and workflow_detail pages
  • Update all route path to make it accept dataSourceId when it has truthy value

Issues Resolved

closes #228

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

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.

Signed-off-by: saimedhi <[email protected]>
Signed-off-by: Sai Medhini Reddy Maryada <[email protected]>
@saimedhi
Copy link
Collaborator Author

Recording.5.mp4

@jackiehanyang
Copy link
Collaborator

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

  • Turning on/off these two feature flags to ensure backward compatibility works fine.
# Set the value of this setting to true to enable multiple data source feature.
#data_source.enabled: false
# Set the value of this setting to true to hide local cluster in data source feature.
#data_source.hideLocalCluster: false
  • Switching between two remote data source clusters to ensure workflow-related data and data source IDs are shown correctly (in url, breadcrumbs.. any possible places).
  • Proving that the workflow you created when remote data source A was connected can also be found in cluster A.
  • Since we don't have tests here, try to click on all possible buttons to test all functionalities. For example, the delete and link button on the Manage Workflows page.
  • Making sure CIs are passing

@saimedhi saimedhi marked this pull request as draft July 31, 2024 23:56
Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall great start, thanks a lot for helping! Generally just some syntax issues and best practices, no big concerns.

common/interfaces.ts Outdated Show resolved Hide resolved
public/app.tsx Outdated Show resolved Hide resolved
public/app.tsx Outdated Show resolved Hide resolved
public/pages/workflow_detail/workflow_detail.tsx Outdated Show resolved Hide resolved
public/pages/workflow_detail/workflow_detail.tsx Outdated Show resolved Hide resolved
public/utils/helpers.ts Outdated Show resolved Hide resolved
server/plugin.ts Outdated Show resolved Hide resolved
server/plugin.ts Show resolved Hide resolved
server/routes/flow_framework_routes_service.ts Outdated Show resolved Hide resolved
server/utils/helpers.ts Outdated Show resolved Hide resolved
Signed-off-by: saimedhi <[email protected]>
saimedhi and others added 4 commits August 2, 2024 13:53
Signed-off-by: Sai Medhini Reddy Maryada <[email protected]>
Signed-off-by: saimedhi <[email protected]>
Signed-off-by: saimedhi <[email protected]>
Signed-off-by: saimedhi <[email protected]>
@saimedhi saimedhi marked this pull request as ready for review August 2, 2024 23:08
public/pages/workflow_detail/workflow_detail.tsx Outdated Show resolved Hide resolved
public/pages/workflows/new_workflow/new_workflow.tsx Outdated Show resolved Hide resolved
public/pages/workflows/workflows.tsx Outdated Show resolved Hide resolved
public/services.ts Outdated Show resolved Hide resolved
public/plugin.ts Outdated Show resolved Hide resolved
public/utils/constants.ts Outdated Show resolved Hide resolved
public/utils/utils.ts Outdated Show resolved Hide resolved
server/utils/helpers.ts Outdated Show resolved Hide resolved
@jackiehanyang
Copy link
Collaborator

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?

@saimedhi saimedhi marked this pull request as draft August 5, 2024 21:27
Signed-off-by: saimedhi <[email protected]>
@saimedhi saimedhi force-pushed the add/MDS_1 branch 2 times, most recently from 6468d73 to 5a78f5c Compare August 6, 2024 03:53
@ohltyler
Copy link
Member

ohltyler commented Aug 6, 2024

@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 :)

Signed-off-by: saimedhi <[email protected]>
@saimedhi saimedhi marked this pull request as ready for review August 6, 2024 19:59
@saimedhi
Copy link
Collaborator Author

saimedhi commented Aug 6, 2024

screen-capture.2.webm

Signed-off-by: Sai Medhini Reddy Maryada <[email protected]>
@saimedhi saimedhi requested a review from ohltyler August 6, 2024 20:12
@ohltyler
Copy link
Member

ohltyler commented Aug 6, 2024

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.

Signed-off-by: Sai Medhini Reddy Maryada <[email protected]>
Copy link
Collaborator

@jackiehanyang jackiehanyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-08-07 at 12 40 57
Screenshot 2024-08-07 at 12 41 06

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?

Signed-off-by: saimedhi <[email protected]>
@saimedhi
Copy link
Collaborator Author

saimedhi commented Aug 7, 2024

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 .

public/app.tsx Outdated
) => (
<WorkflowDetail
setActionMenu={setHeaderActionMenu}
landingDataSourceId={dataSourceId}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this props, and I don't see any usage of this props later. I have this for AD because AD has multiple pages. So I need this prop for AD to track the dataSourceId for landing pages to differentiate:

  • if the page is opened as a landing page, we select the default remote data source
  • if the page is opened by being redirected from another page, we keep the dataSourceId consistent as the previous page instead of selecting the default remote data source.

None of this is applicable to Flow Framework, so please remove unnecessary and unused props.

export const constructHrefWithDataSourceId = (
basePath: string,
dataSourceId: string = '',
withHash: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this flag. I see you always pass true here

Signed-off-by: saimedhi <[email protected]>
@saimedhi
Copy link
Collaborator Author

saimedhi commented Aug 8, 2024

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
Copy link
Collaborator

jackiehanyang commented Aug 8, 2024

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:

  • when you opening flow framework from the side nav, default remote data source should be selected
  • when you opening flow framework from a bookmarked url, the data source that's in the url should be selected
  • when you select a non default remote data source, it should still be selected if you refresh the page, instead of jumping back to the default one.
  • data source should be consistent through breadcrumbs
  • data source should be consistent through in-app side nav
  • considering the project goals and customer perspective, test all possible workflows to ensure that multiple data sources are supported correctly.

Signed-off-by: saimedhi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Support Multiple Data Source in Flow Framework Dashboards Plugin
3 participants