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

create topology side panel using dynamic plugin extensions #9306

Merged
merged 5 commits into from Jul 7, 2021

Conversation

sahil143
Copy link
Contributor

fixes: https://issues.redhat.com/browse/ODC-5845

This PR creates the topology side panel to use the dynamic plugin extensions

Screenshot 2021-06-21 at 6 38 14 PM

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/helm Related to helm-plugin labels Jun 21, 2021
@openshift-ci openshift-ci bot added component/sdk Related to console-plugin-sdk component/shared Related to console-shared component/topology Related to topology labels Jun 21, 2021
@sahil143
Copy link
Contributor Author

/retest

@sahil143 sahil143 force-pushed the topology-sidepanel branch 2 times, most recently from 84bcbcd to bae8517 Compare June 22, 2021 15:14
@openshift-ci openshift-ci bot added the component/dev-console Related to dev-console label Jun 22, 2021
@sahil143 sahil143 changed the title [WIP] create topology side panel using dynamic plugin extensions create topology side panel using dynamic plugin extensions Jun 22, 2021
@openshift-ci openshift-ci bot added kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 22, 2021
Comment on lines +17 to +20
? resurceLinkExtension
.sort((a, b) => a.properties.priority - b.properties.priority)
.find(({ properties: { link } }) => !!link(element))
.properties?.link?.(element)
Copy link
Contributor

Choose a reason for hiding this comment

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

link function will be called twice on the last match instead of only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is intentional as we need to find the callback that doesn't return null and undefined and get result from that callback.
Couldn't find a lodash utility that does this.

<div className="overview__sidebar-pane-head resource-overview__heading">
<h1 className="co-m-pane__heading">
<div className="co-m-pane__name co-resource-item">{resourceLink}</div>
<div className="co-actions">{/** actions */}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a WIP for another PR? If so add a comment.

If not, you should make use of the actions extensions to populate a menu.

@@ -0,0 +1,3 @@
.topology-sidebar-alert {
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow conventions.

useResourceAlertsContent?: (element: GraphElement) => DetailsResourceAlertContent;
element: GraphElement;
}> = ({ title, dismissible = false, useResourceAlertsContent, element }) => {
const [showAlert, setShowAlert] = React.useState<boolean>(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we store these values in user settings for health checks?

@@ -103,4 +103,5 @@ export type DetailsResourceAlertContent = {
content: React.Component | undefined;
variant?: 'success' | 'danger' | 'warning' | 'info' | 'default';
actionLinks?: React.ReactNode;
onClose?: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this added to the API?
I don't think an alert needs to know when it is dismissed.

linkForResource?: (obj: K8sResourceKind) => React.ReactElement;
linkForResource?: (obj) => React.ReactElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the type?

<div key={model.kind}>
<div key={'kind'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

keys should be unique

import { TYPE_HELM_RELEASE } from '../../components/const';
import TopologyHelmReleaseNotesPanel from '../../TopologyHelmReleaseNotesPanel';

export const useHelmReleasePanelDetailsTabSection = (element: GraphElement) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to have a lot of sections.
The first check must be made efficiently to filter out all the sections which do not apply to the given element.
These should not be rendered as a hook.

After successfully passing the check, they should return a component which itself can use hooks within.

Comment on lines 8 to 12
const resourceLink = useDetailsResourceLink(element);
return (
<div className="overview__sidebar-pane-head resource-overview__heading">
<h1 className="co-m-pane__heading">
<div className="co-m-pane__name co-resource-item">{resourceLink}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

There needs to be a fallback resource link if one is not supplied via extension.

  1. extract the resource and create a ResourceLink
  2. render a non-linkable text based on the element's label

Comment on lines 140 to 141
"insertAfter": "helm-release-panel-tab-section-details",
"insertBefore": "helm-release-panel-tab-section-releaseNotes"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why both before and after properties?

Note that the order in which they are defined should remain in tact so long as they are defined by the same plugin.
Therefore if the helm plugin is contributing all help sections, we should not even need to define the before or after properties.

@rohitkrai03
Copy link
Contributor

/assign

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/approve

Changes looks good to me. Tested locally. Works as expected.

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@rohitkrai03
Copy link
Contributor

rohitkrai03 commented Jul 4, 2021

/hold

CI is continuously failing.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 4, 2021
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2021
Comment on lines 34 to 42
const { t } = useTranslation();
const actions = React.useMemo(
() => [
getHelmUpgradeAction(scope, t),
getHelmRollbackAction(scope, t),
getHelmDeleteAction(scope, t),
],
[scope, t],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can actually re use the original provider.

Suggested change
const { t } = useTranslation();
const actions = React.useMemo(
() => [
getHelmUpgradeAction(scope, t),
getHelmRollbackAction(scope, t),
getHelmDeleteAction(scope, t),
],
[scope, t],
);
const actions = useHelmActionProvider(scope);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rohitkrai03 fixed!

@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2021
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 5, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rohitkrai03, sahil143

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 5, 2021
@sahil143
Copy link
Contributor Author

sahil143 commented Jul 6, 2021

/retest

@sahil143
Copy link
Contributor Author

sahil143 commented Jul 7, 2021

/retest

@rohitkrai03
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2021
@openshift-merge-robot openshift-merge-robot merged commit d5d65d4 into openshift:master Jul 7, 2021
@spadgett spadgett added this to the v4.9 milestone Jul 14, 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/dev-console Related to dev-console component/helm Related to helm-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared component/topology Related to topology kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants