Skip to content

Migrate Helm actions on topology to use the new extensions#9313

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
debsmita1:helm-actions
Jul 9, 2021
Merged

Migrate Helm actions on topology to use the new extensions#9313
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
debsmita1:helm-actions

Conversation

@debsmita1
Copy link
Copy Markdown
Contributor

@debsmita1 debsmita1 commented Jun 22, 2021

JIRA ticket:
https://issues.redhat.com/browse/ODC-5693

Solution description:

  • Updated the topology context menu actions for helm release to use the new extension

GIF/Screenshot:
Screenshot from 2021-07-08 01-25-23
topology-context-menu
Peek 2021-07-08 01-37

@openshift-ci openshift-ci bot added component/core Related to console core functionality component/helm Related to helm-plugin component/topology Related to topology labels Jun 22, 2021
@debsmita1
Copy link
Copy Markdown
Contributor Author

/assign @rohitkrai03

Copy link
Copy Markdown
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.

/hold

@debsmita1 This PR seems to be just using ActionLoader to get extension based action but still using the old KebabMenu components. The goal of the story was to use new KebabMenu components potentially after https://issues.redhat.com/browse/ODC-5749 which would lead to some more refactoring in how topology uses actions.

@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 Jun 22, 2021
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this actually work? Type of the actions coming from extensions is a bit different than that of old actions. Seems like this would break somewhere.

@debsmita1 debsmita1 changed the title Migrate Helm actions on topology to use the new extensions [WIP] Migrate Helm actions on topology to use the new extensions Jun 23, 2021
@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 23, 2021
@openshift-ci openshift-ci bot added the component/shared Related to console-shared label Jul 6, 2021
@debsmita1 debsmita1 changed the title [WIP] Migrate Helm actions on topology to use the new extensions Migrate Helm actions on topology to use the new extensions Jul 6, 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 Jul 6, 2021
@debsmita1 debsmita1 requested a review from rohitkrai03 July 6, 2021 13:10
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added this so that the menu items in the context menu are rendered with proper spacing and are focused when the cursor hovers on them

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might add some unnecessary styles to normal menu items.

@debsmita1
Copy link
Copy Markdown
Contributor Author

Fixing the alignment of the Sub Menu and grouped menu items in the Context Menu

@rohitkrai03
Copy link
Copy Markdown
Contributor

@debsmita1 #9306 just merged. So you can remove the duplicate changes from this PR now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might add some unnecessary styles to normal menu items.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sending an onClick handler keeps the context menu open even after the callback has been called.
Screenshot 2021-07-07 at 5 41 50 PM

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed the implementation here. PTAL

@openshift-ci openshift-ci bot added the component/sdk Related to console-plugin-sdk label Jul 7, 2021
@debsmita1
Copy link
Copy Markdown
Contributor Author

Fixing the alignment of the Sub Menu and grouped menu items in the Context Menu

Created util createContextMenuItems which returns the context menu items in a way similar to the menu rendering logic in ActionMenuContent

Screenshot from 2021-07-08 01-25-23

@debsmita1 debsmita1 requested a review from rohitkrai03 July 7, 2021 20:16
@debsmita1
Copy link
Copy Markdown
Contributor Author

/cc @christianvogt
/cc @sahil143

@openshift-ci openshift-ci bot requested review from christianvogt and sahil143 July 7, 2021 20:17
@debsmita1
Copy link
Copy Markdown
Contributor Author

/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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we need this prop. Since all new extensions are hook based, strings will be translated in the hook itself

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

key here can be a constant because there's no conflicts
topology is sufficient

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We also need resource contextId for this util to be reused for other nodes.

@rohitkrai03 we need a way to avoid doing it this way and have a single loader

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I am working on adding support for resource actions in action loader. So we can provide the resource model along with contextId to the action loader and get back all the actions for a specific resource as well as a context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

do we put this PR on hold till then ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can get this merged now. The follow up story that I am working on will take care of resource actions.

Copy link
Copy Markdown
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 9, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Jul 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 9, 2021
@openshift-merge-robot openshift-merge-robot merged commit c224f28 into openshift:master Jul 9, 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/core Related to console core functionality component/helm Related to helm-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared component/topology Related to topology lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants