-
Notifications
You must be signed in to change notification settings - Fork 472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom menu via /api/config with project links as defaults #78
Conversation
Nice!
|
Great work. I'm a bit impartial about the "Create Ticket" as a default config. What do you guys think? If you provide your own, will it override existing or merge the two? |
I think thats a good idea, the build process can create an .env file and put a JSON list of the config so its technically static. Just like the GA Analytics key. If we need it to be dynamic without rebuilding, then the |
I think for external users, it makes sense for them to ping their own tracing/infra team first before opening a ticket against us so even if we agree to have the "create ticket" option, I don't think it should be so prominent in the UI. |
@yurishkuro It needs to happen in jaeger-query. I'll add info to the README when it's implemented.
@yurishkuro That will need to be provided as part of the config that is loaded. Would providing something like the following at the
@yurishkuro I'll move them around.
@saminzadeh Right now, if the user provides their own config, it will fully replace the current config; they're not merged.
@saminzadeh We discussed using a
@black-adder Currently there is only one level of promience, pretty much everything is the same. So, if we want to deemphasize the create ticket links, we should take them out. @yurishkuro Any thoughts on taking out the create ticket links? |
@tiffon If the payload is stored in the .env file, why not just have create-react-app build with it so its available as a static JSON object in the UI code? |
@saminzadeh The current thinking is to parameterize the jaeger query-service so a custom |
I would take them out, completely agree with Won's comment. Internally we'll have Create Ticket link that goes to Phab. |
Make sense but if we don't plan on supporting changing the config dynamically through an endpoint, we should use the env file since query service builds it anyways right? jaeger-query$: echo 'MENU={ "name": "Root", "menu": [..] }' > ui/my-custom-ui-config.env
jaeger-query$: cd ui && npm run build # create-react-app reads .env file on build and injects it If we plan on making a config endpoint in the query service, might as well move all options there including the Google analytics key (which is currently done through .env file)
It would make sense via GET /config My point is, we should pick one for all UI config options |
@saminzadeh We are planning to support dynamic config through the endpoint. It would be Moving the Google Analytics key to the config is a great suggestion, nice 👍 |
Ok, in that case your approach sounds good. |
I've taken out the create ticket links and moved the custom links to the right side of the nav. |
@saminzadeh Will look into moving the google analytics to the config in a separate ticket. Right now the GA integration is not working, I think it might be related to the upgrading react-router. Secondly, moving it to the config will kill GA integration (once it's working again) until the query-service supports the config endpoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@yurishkuro This change implies a change in the query service. It can function without the change, but I need to make sure it will handle the default response of the query service, properly, once the change is in place. What will the default response be from the query service? We discussed it being |
it can be that, or a 404 |
I would avoid a 404 (browser shows it as an error in the console, kinda annoys me). I'm more a fan of returning some default object, maybe version number and other useful information then having the config be a property on that main object even if people don't set it up. ex:
|
…cing#78) * Custom menu via /api/config with friendly default * Move custom menu to right, remove new ticket links Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Fix #44.
/api/config
, errors cause fall-back to defaultlabel
andurl
for each linklabel
anditems
whereitems
is an array of linksDefault menu config:
Create a ticketScreenshot updated
![screen shot 2017-09-21 at 2 37 09 pm](https://user-images.githubusercontent.com/2304337/30712608-94d114e6-9eda-11e7-952a-b44a09a7e967.png)