KIALI-2426 Get serverConfig from static env.js#1012
KIALI-2426 Get serverConfig from static env.js#1012jotak wants to merge 2 commits intokiali:masterfrom
Conversation
jotak
added
the
requires server PR
jotak
force-pushed
the
kiali-2426
branch
2 times, most recently
from
2516fce to
6834173
Compare
| } | ||
| }; | ||
|
|
||
| const tmpConfig: ServerConfig = process.env.TEST_RUNNER |
There was a problem hiding this comment.
Note, I'm rewriting here for tests what is basically the content of env.js ... if someone has a better solution, I don't know how I could make jest tests load "/public/env.js" as a pre-requisite.
israel-hdez
left a comment
israel-hdez
left a comment
There was a problem hiding this comment.
I just quickly checked. It looks good, but I'll let @jshaughn to do final approval.
| label={String(validDurations[validDuration])} | ||
| options={validDurations} | ||
| handleSelect={key => this.props.setDuration(Number(key))} | ||
| value={this.props.duration} |
There was a problem hiding this comment.
I am more comfortable using the validDuration. It' at least, was preventing errors when kiali pages are bookmarked. But this is minor. I won't complain if it's merged as is.
There was a problem hiding this comment.
I see, ok, I'll put it back
There was a problem hiding this comment.
Looking at that more closely, I'm not sure it's really recommended to change duration in "render" like that. It would mean that, if I set an incorrect value from the URL, the dropdown will show something different that what is actually used to perform the prometheus queries. Which could really be misleading.
Or we would have to make a broader refactor to validate durations not on rendering, but on URL parsing.
If we keep without validating the duration from URL, it doesn't work too bad: metrics are fetched using that "custom" time, which works, the UI just shows an empty dropdown, but it's still functional and the list of options is there to allow to switch to a "recognized" duration. I think that's better because we're not like "cheating" with the user.
There was a problem hiding this comment.
Yes, you are right in that it's better a layer of URL validation. I think that's under the scope of this Jira: https://issues.jboss.org/browse/KIALI-420
In the meantime, I think it's not bad to do that here.
There was a problem hiding this comment.
Ok, so moving this validation to URL parsing : see commit 914d7a2
So this is removed from redux (lot of changes) and used instead as a standard constant. This constant is written in 'window' object, in env.js, by the server. File is imported in index.html as static js. Valid durations are now computed statically once for all.
While adding functions in HistoryManager to validate URL, taking that opportunity to do some refactoring here: - Graph components were calling functions from ListPageHelper, which it isn't supposed to do, so moving these functions to HistoryManager - Rename enum URLParams => URLParam - More homogene usage of "undefined" instead of "null" in getting URL params
jshaughn
left a comment
jshaughn
left a comment
There was a problem hiding this comment.
@jotak, see kiali/kiali#863 (comment) before merging. Although you must also resolve conflicts.
|
closing in favor of KIALI-2479 |
3 participants
serverConfig is removed from redux (which makes a lot of changes) and used instead as a standard constant.
This constant is written in 'window' object, in env.js, by the server. File is imported in index.html as static js.
Valid durations are now computed statically once for all.
Also,
window.WEB_ROOTis nowwindow.serverConfig.webRootJIRA: https://issues.jboss.org/browse/KIALI-2426
backend PR: kiali/kiali#863