Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Customize persist key #747

Merged
merged 2 commits into from Oct 15, 2018
Merged

Customize persist key #747

merged 2 commits into from Oct 15, 2018

Conversation

clyang82
Copy link
Contributor

Signed-off-by: Chun Lin Yang clyang@cn.ibm.com

** Describe the change **

right now, the persist key is hard-coded as root in config. it may conflict with other application which is also using persist:root as key in localStorage. Since Kaili supports customize base path (fixed in #569) so that we should get the WEB_ROOT instead of hardcode.

** Backwards compatible? **

Yes

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>
@rhqci
Copy link

rhqci commented Oct 12, 2018

Can one of the admins verify this patch?

@pilhuhn
Copy link
Contributor

pilhuhn commented Oct 12, 2018

@clyang82 Thanks for your contribution, I think that makes sense.
One question though: if you are worried that 'root' is taken and that when you put a context root of e.g. '/monitoring' in, wouldn't there be an equal chance that a persist key of 'monitoring' is taken?

Would it make sense here to do a composite key, that starts with e.g. 'io.kiali' to indicate that whatever you use it points to Kiali?

@jmazzitelli
Copy link
Contributor

adding @mtho11 as reviewer

@jmazzitelli
Copy link
Contributor

Would it make sense here to do a composite key, that starts with e.g. 'io.kiali' to indicate that whatever you use it points to Kiali?

+1 I think we should have the root key be more specific to "kiali" regardless of context root (why not just rename "root" to "kiali", for example?")

@@ -16,8 +16,11 @@ import { INITIAL_STATUS_STATE } from '../reducers/HelpDropdownState';

declare const window;

const webRoot = (window as any).WEB_ROOT ? (window as any).WEB_ROOT : undefined;
const persistKey = webRoot && webRoot !== '/' ? webRoot.substring(1) : 'root';
Copy link
Contributor

@mtho11 mtho11 Oct 12, 2018

Choose a reason for hiding this comment

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

This is a great idea! could we also maybe change 'root' to 'kiali-root'

Copy link
Contributor Author

@clyang82 clyang82 Oct 15, 2018

Choose a reason for hiding this comment

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

@mtho11 Thanks for your comments. I would like to using 'kiali' as a prefix.

@clyang82
Copy link
Contributor Author

Would it make sense here to do a composite key, that starts with e.g. 'io.kiali' to indicate that whatever you use it points to Kiali?

@pilhuhn That is great idea to using composite key to ensure it is unique. I would like to using 'kiali' as a prefix. Thanks.

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>
@pilhuhn
Copy link
Contributor

pilhuhn commented Oct 15, 2018

With that change and as @mtho11 approved, I am merging this.

Thanks a lot @clyang82 👍

@pilhuhn pilhuhn merged commit c16c4db into kiali:master Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants