-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
ui: add React UI from upstream Prometheus #2121
Conversation
d171702
to
f013d2e
Compare
Signed-off-by: Adrien Fillon <adrien.fillon@gmail.com>
f013d2e
to
19cf55b
Compare
@adrien-f can you rebase a bit? @d-ulyanov @geobeau can you help in review? |
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.
Mostly nitpicking, I couldn't review most of the code as it is:
- very big
- mostly from Prometheus
- I'm not react expert
|
||
Yarn consults the `package.json` and `yarn.lock` files for dependencies to install. It creates a `node_modules` directory with all installed dependencies. | ||
|
||
## Running a local development server |
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.
This part should be adapted for Thanos
|
||
**NOTE:** You will likely not need to do this directly. Instead, this is taken care of by the `build` target in the main Prometheus `Makefile` when building the full binary. | ||
|
||
## Integration into Prometheus |
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.
Prometheus -> Thanos
@@ -0,0 +1,15 @@ | |||
{ | |||
"short_name": "Prometheus UI", |
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.
Prometheus -> Thanos
.PHONY: react-app-test | ||
react-app-test: | $(REACT_APP_NODE_MODULES_PATH) react-app-lint | ||
@echo ">> running React app tests" | ||
cd $(REACT_APP_PATH) && yarn test --no-watch --coverage |
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.
cd $(REACT_APP_PATH) && yarn test --no-watch --coverage | |
cd $(REACT_APP_PATH) && export CI=true && yarn test --no-watch --coverage |
Looks like jest
can not understand that it runs in CI, that's why CI did not pass yet.
All unit tests passing.. Don't know how to help with review more.. |
@@ -1,5 +1,5 @@ | |||
# Available from https://hub.docker.com/r/circleci/golang/ | |||
FROM circleci/golang:1.13.6 | |||
FROM circleci/golang:1.13.6-node |
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.
Did not find which Node.js version runs in this image (Could possibly lead to a build problems later). Better to freeze it in package.json
to LTS if possible https://docs.npmjs.com/files/package.json#engines
</DropdownMenu> | ||
</UncontrolledDropdown> | ||
<NavItem> | ||
<NavLink href="https://prometheus.io/docs/prometheus/latest/getting_started/">Help</NavLink> |
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.
Prometheus => Thanos?
Could we add typescript check in Makefile and in CI? What do you think @adrien-f ? package.json
Right now there are several errors |
import { Alerts, Config, Flags, Rules, ServiceDiscovery, Status, Targets, TSDBStatus, PanelList } from './pages'; | ||
|
||
describe('App', () => { | ||
const app = shallow(<App pathPrefix="/path/prefix" />); |
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.
missing thanosComponent
I think this has been postponed for a little bit because in Prometheus this UI code is still very much in flux so we've been recommended to wait for a bit. :-) I guess once it will be moved out of the "experimental" phase we will be able to continue with this. Perhaps @adrien-f knows more information. |
This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions. |
Looking for GSoC help here (: cc @prmsrswt |
Merged via #2412 :P closing this one. Thank you to everyone for your efforts! |
Changes
I've been putting Thanos related modifications in the
ui/react-app/src/thanos
directory but there are some modifications in the root files. The goal is to be able to just copy paste the source from upstream and let the Thanos directory do the rest to ease upgrades.Verification