Use User settings hook for topology layout data, topology view state (graph/list) and cloudshell terminal

[invincibleJai]

Fixes:
https://issues.redhat.com/browse/ODC-5082

Analysis / Root cause:
User preferences were saved in localStorage so not persisted across browsers

Implemented this hook for

• Topology layout Data
• Topology view state (graph/list)
• Cloudshell terminal

Depends on : https://github.com/openshift/console/pull/7301/files

Solution Description:
Implemented useUserSettings for three features

Browser conformance:

• Chrome
• Firefox
• Safari
• Edge

[christoph-jerolimov]

/assign
/assign christianvogt

[invincibleJai]

/retest

[christianvogt]

/hold

We cannot merge this until the backend support is ready. I suggest splitting this PR into one just for the utility function and tests and another for the subsequent updates to the existing code to move from localStorage to user settings.

[christianvogt]

I would have to look into this further but looks like this is a change in behavior because setCloudShellNamespace only updated what was stored in localStorage. Now setNamespace also updates the state.

Unless absolutely sure, I suggest keeping the state namespace separate from the user settings.

[invincibleJai]

i tried verifying it and we had setCloudShellNamespace to set in localStorage and getCloudShellNamespace to get from LocalStorage so with setNamespace it'll update the ns in ConfigMap, don't we need that.

@sahil143 can you as well verify once, if you get time?

[sahil143]

I tested it locally cloud shell seems to work as expected.

[christianvogt]

Verified as well that setting this, while it does trigger an effect, will not cause any issues.

[sahil143]

/lgtm

[christoph-jerolimov]

Tested it again together with api change, works fine.

@invincibleJai needs rebase after #7199

[christoph-jerolimov]

Still works fine 😄

/lgtm

[christoph-jerolimov]

/title Use User settings hook for topology layout data, topology view state (graph/list) and cloudshell terminal

[christianvogt]

Looks like replace doesn't work if the entry isn't present.
I'm seeing the following error trying to patch the configmap:

request url:

PATCH
/api/kubernetes/api/v1/namespaces/console-user-settings/configmaps/user-settings-d15fe175-17a2-463a-bfbc-d6f7da23e31c

request payload:

[{op: "replace", path: "/data/console.terminal.namespace", value: "cd-dev2"}]

422 error response:

{
  "kind": "Status",
  "apiVersion": "v1",
  "metadata": {
    
  },
  "status": "Failure",
  "message": "the server rejected our request due to an error in our request",
  "reason": "Invalid",
  "details": {
    
  },
  "code": 422
}

[christoph-jerolimov]

422 error response:

@christianvogt @invincibleJai The reason is that the ConfigMap contains no data. I fixed this in my PR #7327 by using a different patch format (merge-patch+json instead of patch+json). See:

https://github.com/openshift/console/blob/b06c1a73b9c46c84cbb57b8b931b4bdcb57cab43/frontend/packages/console-shared/src/utils/user-settings.ts#L18-L49

We can merge that as part of my PR or Jai can also integrate this into this PR.

As workaround you need to add some fake data: aaa: bbb to your ConfigMap.

[invincibleJai]

422 error response:

@christianvogt @invincibleJai The reason is that the ConfigMap contains no data. I fixed this in my PR #7327 by using a different patch format (merge-patch+json instead of patch+json). See:

https://github.com/openshift/console/blob/b06c1a73b9c46c84cbb57b8b931b4bdcb57cab43/frontend/packages/console-shared/src/utils/user-settings.ts#L18-L49

We can merge that as part of my PR or Jai can also integrate this into this PR.

As workaround you need to add some fake data: aaa: bbb to your ConfigMap.

Thanks @jerolimov @christianvogt the problem is as you described if data is not there in Configmap and tries operation replace with patch request. what we can do is in op make a check if key doesn't exists instead of always doing replace perform add and we should good.

I updated the code to perform replace only if key exists in Configmap

https://github.com/openshift/console/pull/7051/files#diff-be39cbd2abb0f20ca51f378837f9c219a3920437816474dd2954942b85569fe2R51-R58

Let me know if you see issue.

[christoph-jerolimov]

/hold cancel
/lgtm

[christoph-jerolimov]

/assign @christianvogt

[christianvogt]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, jeff-phillips-18, jerolimov, sahil143

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:

• frontend/OWNERS [christianvogt]

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

[openshift-merge-robot]

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

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	7e4b457760dbb319cb1c23f056b05faba7936f98	link	/test kubevirt-plugin
ci/prow/e2e-gcp-console	cf554d0	link	/test e2e-gcp-console

Full PR test history. Your PR dashboard.

Details

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.

[christoph-jerolimov]

/retest