Bug 1801584: Fixed Inconsistency in kebab and actions menu for storage cluster #4352

gnehapk · 2020-02-18T13:32:06Z

openshift-ci-robot · 2020-02-18T13:32:09Z

@gnehapk: This pull request references Bugzilla bug 1801584, which is invalid:

expected the bug to target the "4.4.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1801584: Fixed Inconsistency in kebab and actions menu for storage cluster

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.

gnehapk · 2020-02-18T14:33:40Z

@spadgett Please review.

gnehapk · 2020-02-18T15:02:31Z

/test e2e-gcp-console
/test analyze

gnehapk · 2020-02-19T06:25:20Z

/bugzilla refresh

openshift-ci-robot · 2020-02-19T06:25:22Z

@gnehapk: This pull request references Bugzilla bug 1801584, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "4.4.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

gnehapk · 2020-02-19T06:45:56Z

/bugzilla refresh

openshift-ci-robot · 2020-02-19T06:46:02Z

@gnehapk: This pull request references Bugzilla bug 1801584, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

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.

spadgett

Can you give more detail about what was happening previously to cause the bug? It's hard to suggest a good solution since the function doesn't have types.

spadgett · 2020-02-19T13:40:25Z

frontend/packages/operator-lifecycle-manager/src/components/operand.tsx

@@ -64,12 +64,19 @@ const csvName = () =>
    );

 const getActions = (selectedObj: any) => {
+  let group = '';
+
+  if (selectedObj.apiVersion.includes('/')) {


We shouldn't be doing any parsing of the apiVersion string here. All of that logic is already handled elsewhere.

In order to verify the group, I have passed the kindObj<k8sKind> from here which does not have apiVersion as a property. Hence, I need to do these changes to get the group.

line basically checks whether the actions(from the ClusterServiceVersionAction extension) need to be added or not.

As the checks have been added here only, hence I have also used the method to do so.

You should rename the parameter and the type. It's no longer a k8s object. I'd suggest model: K8sKind

Why does the kind object not have an api version property? It should.

Oh, are we somethings passing a k8s object and sometimes a model? That's not a good pattern. We should always pass the same type to this function and not attempt to do any detection.

I see for having the CSV action enabled, we need to match kind and apigroup (apigroup because of this).
The getActions(selectedObj) is called twice: for Kebab actions and menu actions.
For Kebab Actions what we are passing is a k8sResource but for menuActions a string

When rather passing K8sKind object (instead of model ref string) to getActions call, it can show CSV actions, only if we drill down the right apiversion to this format "group/version".

One approach i can think of:

groupVersionFor(apiVersionForModel(selectedObj)).group === action.properties.apiGroup

@gnehapk @spadgett ^^

We should always pass the model to this method. We shouldn't sometimes pass the model and sometimes pass an instance. We have the model in both contexts already.

@spadgett thanks for the inputs, any pointers on the models for kebabactions ? The OperandTableRowProps seems to provide only a k8sresource.

Or one more thing we can do if you consider:

Just passing kind and apigroup in the getActions function only.

const getActions = ({kind, apiGroup}) => { ....

Thanks!

I think the simplest thing to do is to change getActions to take a referene

const getActions = (reference: K8sResourceKindReference) => {

and pass it

<ResourceKebab actions={getActions(referenceFor(obj)} kind={referenceFor(obj)} resource={obj} />

and

menuActions={getActions(props.modelRef)}

Indeed 👍

spadgett · 2020-02-19T13:40:54Z

frontend/packages/operator-lifecycle-manager/src/components/operand.tsx

+  if (selectedObj.apiVersion.includes('/')) {
+    group = groupVersionFor(selectedObj.apiVersion).group;
+  } else {
+    group = selectedObj.apiGroup;


What is selectedObj? k8s resources don't have this field.

I have passed the kindObj<k8sKind> from here which does not have apiVersion as a property but have apiGroup in order to get the group.

spadgett · 2020-02-19T13:41:45Z

frontend/packages/operator-lifecycle-manager/src/components/operand.tsx

@@ -64,12 +64,19 @@ const csvName = () =>
    );

 const getActions = (selectedObj: any) => {


Not new, but we should have a type other than any here. It's hard to recommend a fix since it's unclear what kind of obj this is.

gnehapk · 2020-02-20T05:41:47Z

@spadgett All the actions that are present inside the kebab menu for storagecluster i.e.

are not there in the storage cluster details view(by clicking the storage cluster name's link from above)

These changes target this bug. Hope this explains.

openshift-ci-robot · 2020-02-25T18:51:23Z

@gnehapk: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/verify	`f4b7d0f`	link	`/test verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

gnehapk · 2020-02-26T04:58:33Z

/assign @spadgett

…e cluster instance`

spadgett

/approve
/lgtm

openshift-ci-robot · 2020-02-26T13:20:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnehapk, spadgett

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:

~~frontend/packages/operator-lifecycle-manager/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-02-26T14:38:29Z

@gnehapk: All pull requests linked via external trackers have merged. Bugzilla bug 1801584 has been moved to the MODIFIED state.

In response to this:

Bug 1801584: Fixed Inconsistency in kebab and actions menu for storage cluster

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.

openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 18, 2020

openshift-ci-robot requested review from jeff-phillips-18 and spadgett February 18, 2020 13:33

openshift-ci-robot added the component/olm Related to OLM label Feb 18, 2020

gnehapk force-pushed the bug-1801584 branch from d5291e0 to f4b7d0f Compare February 18, 2020 14:03

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 19, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 19, 2020

spadgett requested changes Feb 19, 2020

View reviewed changes

gnehapk force-pushed the bug-1801584 branch from f4b7d0f to 0057d26 Compare February 26, 2020 04:37

openshift-ci-robot assigned spadgett Feb 26, 2020

Bug 1801584: Fixed Inconsistency in kebab and actions menu for storag…

e3c4fc1

…e cluster instance`

gnehapk force-pushed the bug-1801584 branch from 0057d26 to e3c4fc1 Compare February 26, 2020 05:40

spadgett approved these changes Feb 26, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2020

openshift-merge-robot merged commit edd00c2 into openshift:master Feb 26, 2020

spadgett added this to the v4.5 milestone Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1801584: Fixed Inconsistency in kebab and actions menu for storage cluster #4352

Bug 1801584: Fixed Inconsistency in kebab and actions menu for storage cluster #4352

gnehapk commented Feb 18, 2020

openshift-ci-robot commented Feb 18, 2020

gnehapk commented Feb 18, 2020

gnehapk commented Feb 18, 2020

gnehapk commented Feb 19, 2020

openshift-ci-robot commented Feb 19, 2020

gnehapk commented Feb 19, 2020

openshift-ci-robot commented Feb 19, 2020

spadgett left a comment

spadgett Feb 19, 2020

gnehapk Feb 20, 2020

gnehapk Feb 20, 2020

spadgett Feb 20, 2020

spadgett Feb 20, 2020

afreen23 Feb 25, 2020 •

edited

spadgett Feb 25, 2020

afreen23 Feb 25, 2020

spadgett Feb 25, 2020

afreen23 Feb 25, 2020

spadgett Feb 19, 2020

gnehapk Feb 20, 2020

spadgett Feb 19, 2020

gnehapk commented Feb 20, 2020

openshift-ci-robot commented Feb 25, 2020 •

edited

gnehapk commented Feb 26, 2020

spadgett left a comment

openshift-ci-robot commented Feb 26, 2020

openshift-ci-robot commented Feb 26, 2020

		@@ -64,12 +64,19 @@ const csvName = () =>
		);

		const getActions = (selectedObj: any) => {

Bug 1801584: Fixed Inconsistency in kebab and actions menu for storage cluster #4352

Bug 1801584: Fixed Inconsistency in kebab and actions menu for storage cluster #4352

Conversation

gnehapk commented Feb 18, 2020

openshift-ci-robot commented Feb 18, 2020

gnehapk commented Feb 18, 2020

gnehapk commented Feb 18, 2020

gnehapk commented Feb 19, 2020

openshift-ci-robot commented Feb 19, 2020

gnehapk commented Feb 19, 2020

openshift-ci-robot commented Feb 19, 2020

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Feb 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Feb 20, 2020

openshift-ci-robot commented Feb 25, 2020 • edited

gnehapk commented Feb 26, 2020

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 26, 2020

openshift-ci-robot commented Feb 26, 2020

afreen23 Feb 25, 2020 •

edited

openshift-ci-robot commented Feb 25, 2020 •

edited