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

Expose Internal APIs #9230

Merged
merged 1 commit into from Jul 28, 2021
Merged

Conversation

bipuladh
Copy link
Contributor

@bipuladh bipuladh commented Jun 14, 2021

Exposes the following Components:

Hooks

  • usePrometheusPoll

Dashboard Related Components

  • DashboardGrid
  • HealthItem
  • AlertItem
  • AlertsBody
  • PrometheusUtilizationItem
  • ResourceInventoryItem
  • UtilizationBody
  • RecentEventsBody
  • OngoingActivityBody
  • DashboardCard
  • DashboardCardBody
  • DashboardCardHeader
  • DashboardCardTitle
  • DetailsItem
  • DetailsBody
  • UtilizationItem
  • ActivityItem
  • ActivityBody

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2021
@openshift-ci openshift-ci bot added component/ceph Related to ceph-storage-plugin component/sdk Related to console-plugin-sdk labels Jun 14, 2021
@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dashboard Related to dashboard component/dev-console Related to dev-console component/insights Related to insights plugin component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/shared Related to console-shared labels Jun 17, 2021
@bipuladh bipuladh changed the title [WIP] Start exposing APIs for dashboard Expose APIs for dashboard, list page, details page and utils Jun 17, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 17, 2021
@rawagner
Copy link
Contributor

I have a PRs which are adding types to ListPage and Table and also another ones which are updating the APIs of the components so its easier for consumer to use them.

types for list page
#8792
new list page API
#8827

types for Table
#8756
new Table API
#8829

@bipuladh
Copy link
Contributor Author

/assign @christianvogt @vojtechszocs

@bipuladh bipuladh requested a review from rawagner June 29, 2021 11:55
@bipuladh bipuladh changed the title Expose APIs for dashboard, list page, details page and utils Expose APIs for dashboard Jun 29, 2021
@@ -7,4 +8,5 @@ export const exposePluginAPI = () => {
useK8sWatchResources: require('@console/internal/components/utils/k8s-watch-hook')
.useK8sWatchResources,
};
Object.assign(window.api, exposedAPIs);
Copy link
Contributor Author

@bipuladh bipuladh Jul 1, 2021

Choose a reason for hiding this comment

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

I din't make a new object for it. Should I add it to window.provisional instead? @vojtechszocs

@bipuladh
Copy link
Contributor Author

It looks like there are more components exposed in the PR than listed in the descriptions. Were these needed because they are dependencies of the components we're exposing?

@bipuladh @rawagner How much of the basic dashboard layout like DashboardGrid and DashboardCard can be accomplished with PatternFly components? DashboardCard for instance appears to wrap the PatternFly Card and only adds one or two CSS classes.

IMO all of it can be accomplished from PF. However, exposing these simple components should allow us to maintain similarities across dashboards and changing CSS in the core console would allow us to revamp all the dashboards(wouldn't have to wait for two releases) without changing plugin code.

@bipuladh
Copy link
Contributor Author

bipuladh commented Jul 12, 2021

I don't think we want to put PageHeading and HorizontalNav under internalAPI since we'll need these for kubevirt in 4.9 as part of the SDK. I feel like we should look at cleaning up and simplifying these APIs. I'm not sure we want to expose them as-is.

I have created a wrapper for PageHeading #9465 based on the commonly used props. Apparently only a few of the defined are commonly used.
Will do the same for HorizontalNav.

Can we consider having only one hook for polling Prometheus (and always storing the data in Redux if that's needed)?

usePrometheusQuery is tied close to the dashboard. Maybe we could add a prop isDashboard which would trigger Dashboard related Actions else it could work exactly like usePrometheusPoll or just use it internally....?

@bipuladh bipuladh force-pushed the expose-api branch 2 times, most recently from 72df9dd to dd3c8ff Compare July 12, 2021 12:39
@rawagner
Copy link
Contributor

Can we consider having only one hook for polling Prometheus (and always storing the data in Redux if that's needed)?

usePrometheusQuery is tied close to the dashboard. Maybe we could add a prop isDashboard which would trigger Dashboard related Actions else it could work exactly like usePrometheusPoll or just use it internally....?

+1 for having one prometheus hook and storing data to redux. I wanted to do it for some time but cannot find the time

@rawagner
Copy link
Contributor

It looks like there are more components exposed in the PR than listed in the descriptions. Were these needed because they are dependencies of the components we're exposing?
@bipuladh @rawagner How much of the basic dashboard layout like DashboardGrid and DashboardCard can be accomplished with PatternFly components? DashboardCard for instance appears to wrap the PatternFly Card and only adds one or two CSS classes.

IMO all of it can be accomplished from PF. However, exposing these simple components should allow us to maintain similarities across dashboards and changing CSS in the core console would allow us to revamp all the dashboards(wouldn't have to wait for two releases) without changing plugin code.

As @bipuladh said. That is the idea behind these simple components.

@christianvogt
Copy link
Contributor

Changes lgtm

We should be working towards promoting these extensions to proper APIs.
I don't want to get into having to maintain backwards compatibility for these APIs. Something we touched on in our meetings but need to further discuss.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2021
@bipuladh bipuladh force-pushed the expose-api branch 2 times, most recently from e099218 to 7bad0f5 Compare July 22, 2021 16:17
@cloudbehl
Copy link
Contributor

/test e2e-gcp-console

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 28, 2021

@bipuladh: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

  • /test analyze
  • /test backend
  • /test ceph-storage-plugin
  • /test e2e-gcp-console
  • /test frontend
  • /test images
  • /test kubevirt-plugin

Use /test all to run the following jobs:

  • pull-ci-openshift-console-master-analyze
  • pull-ci-openshift-console-master-backend
  • pull-ci-openshift-console-master-e2e-gcp-console
  • pull-ci-openshift-console-master-frontend
  • pull-ci-openshift-console-master-images

In response to this:

/test tide

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@SanjalKatiyar
Copy link
Contributor

Have tested the PR for the following cases:

  • Components used by ODF Dashboard:

usePrometheusPoll

Screenshot from 2021-07-28 15-09-28
Screenshot from 2021-07-28 15-09-50

HealthItem

Screenshot from 2021-07-28 15-08-53

RecentEventsBody

Screenshot from 2021-07-28 15-09-06

DashboardCard, DashboardCardBody, DashboardCardHeader, DashboardCardTitle

Screenshot from 2021-07-28 18-26-05


  • Rest of console dashboards working fine with these changes:

Screenshot from 2021-07-28 17-49-06
Screenshot from 2021-07-28 18-01-21
Screenshot from 2021-07-28 18-05-16

@christianvogt
Copy link
Contributor

/lgtm
/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 28, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, christianvogt

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2021
@spadgett
Copy link
Member

@yapei has given us the OK for development to test the PR.

/label qe-approved

This PR has no user visible changes and has no doc impact. These APIs are internal and can change at any time.

/label px-approved
/label docs-approved

@ahardin-rh @sferich888 FYI

/hold cancel

@openshift-ci openshift-ci bot added qe-approved Signifies that QE has signed off on this PR px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 28, 2021
@openshift-merge-robot openshift-merge-robot merged commit 2bed10f into openshift:master Jul 28, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/dev-console Related to dev-console component/insights Related to insights plugin component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/monitoring Related to monitoring component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants