Bug 1908687: Use localstorage fallback when running a local bridge #7586

jerolimov · 2020-12-17T09:55:24Z

Fixes:
https://issues.redhat.com/browse/CONSOLE-2510
https://bugzilla.redhat.com/show_bug.cgi?id=1908687

Analysis / Root cause:
With the new user settings feature its hard for console developers who shares and switch between many clusters.

The settings could confuse other developers on the same cluster / user. And they are often missed when switching to another cluster.

Solution Description:

This PR introduce a new bridge cli option --user-settings-location=configmap|localstorage.
It was automatically set to localstorage in the oc-enviconment.sh script. So its automatically active for all console developers which runs a local bridge.
The flag is available in frontend as SERVER_FLAGS.userSettingsLocation
The user settings hook(s) automatically check this flags and use the localStorage if the flag is set to localstorage.

It's possible to override this settings with cli option like bin/bridge --user-settings-location=configmap

Screen shots / Gifs for design review:

Unit test coverage report:

Test setup:

Browser conformance:

Chrome
Firefox
Safari
Edge

jerolimov · 2020-12-17T10:35:36Z

/assign @jhadvig @christianvogt
/cc @invincibleJai

openshift-ci-robot · 2020-12-17T10:59:15Z

@jerolimov: This pull request references Bugzilla bug 1908687, 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.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1908687: Use localstorage fallback when running a local bridge

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.

jerolimov · 2020-12-17T11:01:52Z

/bugzilla refresh

openshift-ci-robot · 2020-12-17T11:02:00Z

@jerolimov: This pull request references Bugzilla bug 1908687, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

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.

jerolimov · 2021-01-04T17:54:57Z

/retest

jerolimov · 2021-01-04T18:51:27Z

/retest

jerolimov · 2021-01-04T22:41:20Z

/retest

jerolimov · 2021-01-04T23:17:45Z

/cc @christianvogt

christianvogt · 2021-01-07T20:15:19Z

/lgtm
/approve

@jerolimov it would be good to allow the dev to maybe specify a fixed user name. While logging in as kubeadmin, the configmap name is consistent, when using consoledeveloper, the configmap name gets concatenated with the user UID which differs on each dev cluster.

christianvogt · 2021-01-07T20:16:07Z

fyi @spadgett

spadgett · 2021-01-11T19:46:24Z

cmd/bridge/main.go

@@ -122,7 +122,8 @@ func main() {

 	fLoadTestFactor := fs.Int("load-test-factor", 0, "DEV ONLY. The factor used to multiply k8s API list responses for load testing purposes.")

-	fDevCatalogCategories := fs.String("developer-catalog-categories", "", "Allow catalog categories customization. (JSON as string)")
+	fDevCatalogCategories := fs.String("developer-catalog-categories", "", "DEV ONLY. Allow catalog categories customization. (JSON as string)")


Is this flag really dev-only? I thought this was used in production (set by the operator).

You're right. I removed this again.

spadgett · 2021-01-11T19:53:02Z

cmd/bridge/main.go

@@ -122,7 +122,8 @@ func main() {

 	fLoadTestFactor := fs.Int("load-test-factor", 0, "DEV ONLY. The factor used to multiply k8s API list responses for load testing purposes.")

-	fDevCatalogCategories := fs.String("developer-catalog-categories", "", "Allow catalog categories customization. (JSON as string)")
+	fDevCatalogCategories := fs.String("developer-catalog-categories", "", "DEV ONLY. Allow catalog categories customization. (JSON as string)")
+	fUserSettings := fs.String("user-settings", "configmap", "DEV ONLY. Define if the user settings should be stored in a shared ConfigMap or in users browser (configmap | localstorage).")


We should add validation when reading the flag.

nit: maybe user-settings-location?

Added a validation to pkg/serverconfig/validate.go

Renamed everything to userSettingsLocation or user-settings-location

openshift-ci · 2021-01-12T13:51:04Z

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

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	`bf7e424`	link	`/test kubevirt-plugin`

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.

jerolimov · 2021-01-12T13:57:26Z

/retest

@spadgett Thanks for the review. I applied all your requests. Can you please check again.

@christianvogt @invincibleJai I also added this commit f206ace to do NOT append the user uid (or kubeadmin) if this -user-settings-location configmap is used.

It's not beautiful because I added the localStorage getItem/setItem key to the method signature. But I also don't want check the SERVER_FLAGS twice. This also enables me to remove the redux dependency from useUserSettingsLocalStorage. Wdyt?

invincibleJai · 2021-01-12T15:40:05Z

/lgtm

Thanks @jerolimov , verified both in cluster and off cluster seems to work as expected. can update description as well to use bin/bridge --user-settings-location=configmap

openshift-ci-robot · 2021-01-12T17:07:42Z

@jerolimov: This pull request references Bugzilla bug 1908687, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1908687: Use localstorage fallback when running a local bridge

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.

christianvogt · 2021-01-12T19:16:38Z

nice
I like how my pinned nav items and other setting stick with me in local dev

/approve

/assign @spadgett

and skip second use of redux useSelector in inner useUserSettingsLocalStorage

spadgett

/lgtm

openshift-ci-robot · 2021-01-13T17:39:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, 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-robot · 2021-01-13T23:22:40Z

@jerolimov: All pull requests linked via external trackers have merged:

openshift/console#7586

Bugzilla bug 1908687 has been moved to the MODIFIED state.

In response to this:

Bug 1908687: Use localstorage fallback when running a local bridge

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 the component/backend Related to backend label Dec 17, 2020

openshift-ci-robot requested review from jhadvig and zherman0 December 17, 2020 09:55

openshift-ci-robot added the component/shared Related to console-shared label Dec 17, 2020

openshift-ci-robot assigned christianvogt and jhadvig Dec 17, 2020

openshift-ci-robot requested a review from invincibleJai December 17, 2020 10:35

jerolimov force-pushed the console-2510 branch from fff4701 to bf7e424 Compare December 17, 2020 10:51

jerolimov changed the title ~~Use localstorage fallback when running a local bridge~~ Bug 1908687: Use localstorage fallback when running a local bridge Dec 17, 2020

openshift-ci-robot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Dec 17, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels Dec 17, 2020

jerolimov force-pushed the console-2510 branch from bf7e424 to a22efa5 Compare January 4, 2021 14:26

jerolimov mentioned this pull request Jan 4, 2021

dispatch storage event to sync current window jerolimov/console#1

Closed

openshift-ci-robot requested a review from christianvogt January 4, 2021 23:17

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2021

spadgett reviewed Jan 11, 2021

View reviewed changes

jerolimov force-pushed the console-2510 branch from fca383c to e89b463 Compare January 12, 2021 10:41

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021

openshift-ci-robot assigned invincibleJai Jan 12, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021

openshift-ci-robot assigned spadgett Jan 12, 2021

jerolimov requested a review from spadgett January 12, 2021 20:24

jerolimov and others added 4 commits January 13, 2021 18:30

Use localstorage fallback when running a local bridge

7f0f368

Align userSettingsLocalStorage sync option to usersettings hook

450d77f

dispatch storage event to sync current window

6826f29

Skip user suffix for local developments in localStorage

6d71d3c

and skip second use of redux useSelector in inner useUserSettingsLocalStorage

jerolimov force-pushed the console-2510 branch from f206ace to 6d71d3c Compare January 13, 2021 17:31

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021

spadgett approved these changes Jan 13, 2021

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2021

openshift-merge-robot merged commit 3d36cd1 into openshift:master Jan 13, 2021

spadgett added this to the v4.7 milestone Jan 18, 2021

jerolimov mentioned this pull request Jan 22, 2021

Add scripts to get and set user settings on a cluster #7384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1908687: Use localstorage fallback when running a local bridge #7586

Bug 1908687: Use localstorage fallback when running a local bridge #7586

jerolimov commented Dec 17, 2020 •

edited

jerolimov commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

jerolimov commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

jerolimov commented Jan 4, 2021

jerolimov commented Jan 4, 2021

jerolimov commented Jan 4, 2021

jerolimov commented Jan 4, 2021

christianvogt commented Jan 7, 2021

christianvogt commented Jan 7, 2021

spadgett Jan 11, 2021

jerolimov Jan 12, 2021

spadgett Jan 11, 2021 •

edited

spadgett Jan 11, 2021

jerolimov Jan 12, 2021 •

edited

openshift-ci bot commented Jan 12, 2021 •

edited

jerolimov commented Jan 12, 2021

invincibleJai commented Jan 12, 2021

openshift-ci-robot commented Jan 12, 2021

christianvogt commented Jan 12, 2021

spadgett left a comment

openshift-ci-robot commented Jan 13, 2021

openshift-ci-robot commented Jan 13, 2021

Bug 1908687: Use localstorage fallback when running a local bridge #7586

Bug 1908687: Use localstorage fallback when running a local bridge #7586

Conversation

jerolimov commented Dec 17, 2020 • edited

jerolimov commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

jerolimov commented Dec 17, 2020

openshift-ci-robot commented Dec 17, 2020

jerolimov commented Jan 4, 2021

jerolimov commented Jan 4, 2021

jerolimov commented Jan 4, 2021

jerolimov commented Jan 4, 2021

christianvogt commented Jan 7, 2021

christianvogt commented Jan 7, 2021

spadgett Jan 11, 2021

Choose a reason for hiding this comment

jerolimov Jan 12, 2021

Choose a reason for hiding this comment

spadgett Jan 11, 2021 • edited

Choose a reason for hiding this comment

spadgett Jan 11, 2021

Choose a reason for hiding this comment

jerolimov Jan 12, 2021 • edited

Choose a reason for hiding this comment

openshift-ci bot commented Jan 12, 2021 • edited

jerolimov commented Jan 12, 2021

invincibleJai commented Jan 12, 2021

openshift-ci-robot commented Jan 12, 2021

christianvogt commented Jan 12, 2021

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 13, 2021

openshift-ci-robot commented Jan 13, 2021

jerolimov commented Dec 17, 2020 •

edited

spadgett Jan 11, 2021 •

edited

jerolimov Jan 12, 2021 •

edited

openshift-ci bot commented Jan 12, 2021 •

edited