Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

adrien-f
Copy link
Member

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Add React UI accesible from the Query only for the moment as only the graph endpoint has been migrated.

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

image

@adrien-f adrien-f force-pushed the feature/react-ui branch 2 times, most recently from d171702 to f013d2e Compare February 12, 2020 08:57
@adrien-f adrien-f mentioned this pull request Feb 12, 2020
6 tasks
Signed-off-by: Adrien Fillon <adrien.fillon@gmail.com>
@bwplotka
Copy link
Member

@adrien-f can you rebase a bit?

@d-ulyanov @geobeau can you help in review?

Copy link
Contributor

@geobeau geobeau left a 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
Copy link
Contributor

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
Copy link
Contributor

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",
Copy link
Contributor

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
Copy link

Choose a reason for hiding this comment

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

Suggested change
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.

@vankop
Copy link

vankop commented Feb 25, 2020

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
Copy link

@vankop vankop Feb 25, 2020

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>
Copy link

Choose a reason for hiding this comment

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

Prometheus => Thanos?

@vankop
Copy link

vankop commented Feb 25, 2020

Could we add typescript check in Makefile and in CI? What do you think @adrien-f ?

package.json

scripts: {"types": "tsc --pretty --noEmit"}

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" />);
Copy link

Choose a reason for hiding this comment

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

missing thanosComponent

@vankop
Copy link

vankop commented Mar 10, 2020

@bwplotka @adrien-f friendly ping on this 🤗

Are we waiting for more reviewers? or there is other blockers to this PR?

@GiedriusS
Copy link
Member

@bwplotka @adrien-f friendly ping on this

Are we waiting for more reviewers? or there is other blockers to this PR?

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.

@stale
Copy link

stale bot commented Apr 9, 2020

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.

@stale stale bot added the stale label Apr 9, 2020
@bwplotka
Copy link
Member

bwplotka commented Apr 9, 2020

Looking for GSoC help here (:

cc @prmsrswt

@GiedriusS
Copy link
Member

Merged via #2412 :P closing this one. Thank you to everyone for your efforts!

@GiedriusS GiedriusS closed this May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants