Enhancement doc to customize default pre-pinned resources on navigation

[debsmita1]

JIRA:
https://issues.redhat.com/browse/ODC-7181

Epic:
https://issues.redhat.com/browse/ODC-5012

[debsmita1]

/cc @jerolimov
/cc @JoelSpeed

[debsmita1]

/assign @spadgett

[jerolimov]

Just two nits and the recommendation to split the risk issues from the mitigations:

[JoelSpeed]

I don't have much feedback to add here right now, will continue the API review on the API PR and then I'd like to see this updated once we've cleared things up there, but otherwise, looks sensible

[jerolimov]

Lgtm!

/cc @JoelSpeed @spadgett

[spadgett]

What about unrecognized perspectives / perspectives contributed by plugins?

[debsmita1]

So, should we update the CEL to allow adding pinnedResources only to the "dev" perspective?
cc @jerolimov

[jerolimov]

If I'm correct, all other perspectives then admin supports pinned resources. All would use https://github.com/openshift/console/blob/master/frontend/packages/console-app/src/components/nav/PerspectiveNav.tsx

We just disable deny to configure it for the admin perspective.

[spadgett]

Hm, I don't see a way to pin resources to the "All Clusters" perspective for ACM/MCE when I test it in the UI. We should double check this.

@jerolimov We should also test that we don't have a race condition here:

https://github.com/openshift/console/blob/master/frontend/packages/console-app/src/components/nav/PerspectiveNav.tsx#L28-L31

We perform API discovery asynchronously, and the cached resources won't always match the current on initial console load. Also on first load there will be no cached resources, which could hide links to some resources when a user first uses the console. Really we should react when the models change.

[debsmita1]

Hm, I don't see a way to pin resources to the "All Clusters" perspective for ACM/MCE when I test it in the UI. We should double check this.

I have updated this to:

Note: The Administrator perspective (id="admin") and "All Clusters" perspective for ACM/MCE (id="acm") doesn't support pinned resources at the moment and can be excluded via CEL validation.

@jerolimov We should also test that we don't have a race condition here:

https://github.com/openshift/console/blob/master/frontend/packages/console-app/src/components/nav/PerspectiveNav.tsx#L28-L31

We perform API discovery asynchronously, and the cached resources won't always match the current on initial console load. Also on the first load there will be no cached resources, which could hide links to some resources when a user first uses the console. Really we should react when the models change.

We tested this by clearing the local storage and then refreshed the page and here's a small recording of that, please let us know if there's any other way to test this:

Screen.Recording.2022-12-13.at.7.51.39.PM.mov

[spadgett]

What do you mean by "incorrect"? Do you mean the resource doesn't exist on the cluster?

Have we considered how this will work with multi-cluster in 4.13 where console talks to different clusters that might have different sets of resources?

[jerolimov]

Pinned resources aren't shown when the CRD (model) isn't found. So switching should not be an issue.

And isn't the customization configuration per cluster? Is the index and Server flags reloaded when Switching a cluster?

We should def. test this.

[debsmita1]

When switching between clusters the invalid resources should disappear. We will test this

[spadgett]

The console customization comes from the hub since that is where the console is running. We don't read console configuration from the spoke clusters.

[spadgett]

I would clarify in the enhancement what you mean by "incorrect" here.

[spadgett]

Where is the warning displayed?

I'd probably only show the warning to admins who have permission to fix the problem.

[debsmita1]

we have added a regex to validate if the values entered for resource, version, and group are correct

[jerolimov]

We can show the warning just in the "console cluster configuration" UI, where the admin can configure it.

We now the available CRDs there and can show configured entries that will not shown to the user because there is no CRD/resource available.

Like this:

perspectives:
  id: dev
  pinnedResources:
  - resource: Secret
  - resource: Pipelinee # wrong with two e

The UI could show:

• (S) Secret
• (P) Pipelinee ⚠️ with a tooltip "Missing CRD, this pinned resource might not be shown."

[jerolimov]

@lokanandaprabhu please take care of invalid configurations in the UI and show them anyway with such a warning.

[spadgett]

This gets a bit tricky since we do API discovery in the background, but I wouldn't show a permanent warning to users who don't have a way to fix the problem. Normal users won't be able to edit this config. It would be better to hide the nav item IMO.

I guess there is the case where the user pins a custom resource themselves and then the CRD is removed, but we don't have a way to tell, correct?

[debsmita1]

when the user pins a custom resource themselves and the CRD is removed, the pinned resource is no longer shown on the navigation.

After discussing this with @jerolimov, we would show a warning alert icon on the customization form for the unrecognized resource, on the navigation the user would only see the recognized ones.

[spadgett]

After discussing this with @jerolimov, we would show a warning alert icon on the customization form for the unrecognized resource, on the navigation the user would only see the recognized ones.

Sounds good to me.

We should clarify again what we mean by "incorrect" here. "Missing" might be a better term.

[spadgett]

Won't this only catch the first time an admin changes the pinned resources? For instance, if the admin adds deployments one week and config maps the next... The config maps won't get added since the previous user setting was not the default (even if the user didn't customize anything).

[jerolimov]

We have three buckets with pinned resources per perspective

1. Pinned resources from the perspective
2. Pinned resources from the customization
3. Pinned resources from the user settings

We should NOT save pinned resources until the user changes them manually (Add/Remove/Reorder). Unluckily we do this at the moment. So would recommend changing this, and then we need also to migrate the old data.

I hope we can do this while loading the data the first time like this:

1. If the user has pinned resources in the user settings:
   1. If these from the user settings are "deep" equal to the pinned resources from the perspective => delete the PR from user settings.
   2. Otherwise use the pinned resources from the user settings.
2. If there are pinned resources in the customization use them
3. Fallback to the pinned resources from the plugin definition.

We could also think about a more explicit migration, but I would keep it simple here and just compare both lists. As long as the user settings contains pinned resource = Secret+ConfigMap I would expect that the user doesn't touched them.

[spadgett]

@jerolimov's proposal here looks reasonable to me. We should update the enhancement to cover this.

[spadgett]

No major concerns. Thanks @debsmita1

[spadgett]

I would clarify in the enhancement what you mean by "incorrect" here.

[spadgett]

I don't think other perspectives contributed by plugins will get pinned resources by default. We should clarify that here. Basically at the moment, we should only allow this for the dev perspective (unless I'm mistaken).

[spadgett]

After discussing this with @jerolimov, we would show a warning alert icon on the customization form for the unrecognized resource, on the navigation the user would only see the recognized ones.

Sounds good to me.

We should clarify again what we mean by "incorrect" here. "Missing" might be a better term.

[spadgett]

@jerolimov's proposal here looks reasonable to me. We should update the enhancement to cover this.

[spadgett]

/lgtm
/hold

@JoelSpeed did you have any other concerns before we merge? Feel free to remove the hold if not.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, 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:

• 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]

@debsmita1: all tests passed!

Full PR test history. Your PR dashboard.

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.

[JoelSpeed]

/lgtm
/hold cancel