UI: Add new codemirror-promql-based expression editor

[Nexucis]

based on the work done by @juliusv

Should fix #4028

[Nexucis]

ah yeah that's true, I forgot that, nodejs should be upgraded to the v14 at least

[Nexucis]

is a strong requirement to support node v10 + v12 ? Or is it fine to drop them and move to node v14 ?

[wiardvanrij]

is a strong requirement to support node v10 + v12 ? Or is it fine to drop them and move to node v14 ?

I'm not too famliair with the UI part here, but if Prometheus 'forces' a minimum of node v14 then I see no reason to stick with 10 & 12. It seems v14 is the default at the moment too.

I also checked the code but it only looks like Thanos is testing against 10 & 12, not that it is 'used' perse?

In the config.yaml for Thanos .circleci this is used: - image: cimg/go:1.15-node which is: https://github.com/CircleCI-Public/cimg-go/blob/master/1.15/node/Dockerfile
Which then uses -> https://raw.githubusercontent.com/CircleCI-Public/cimg-node/master/ALIASES and then grep on LTS, which is lts=14.16.0

Soooo.... I think we build on node v14 and we should test against v14 and not 'bother' with < v14 anymore.

[Nexucis]

alright the build is finally passing with nodeJS v14.

I supposed then the build of Node 10/12 should be unchecked in the settings to remove them from the required list.

Hope it is fine for you as well :)

[Nexucis]

ah the e2e test are not passing. Mmm that's quite weird I didn't touch at the golang part.

Did I break something unexpectedly ?

[wiardvanrij]

ah the e2e test are not passing. Mmm that's quite weird I didn't touch at the golang part.

Did I break something unexpectedly ?

Always blame the user for breaking tests :p I have no idea actually, I'll try a local test :)

• edit: It seems to be magically fixed... :)

[Nexucis]

Nice thanks @wiardvanrij !

[bwplotka]

I am good with that, but I did not look on React code in details. Relying on our expert @onprem here.
Thank you for porting this! 🤗 Amazing 💪🏽

I only would propose to enable experimental editor by default. WDYT? Also I am not sure, but did we already enabled new UI by default? If not, we could too

[Nexucis]

thanks :) and np !

I only would propose to enable experimental editor by default. WDYT?

I guess it's more depending if you want to rely on the Prometheus choice or not ^^. It's not enable by default on Prometheus side, so I assumed it would be the same here. From my point of view, it's fine to enable it by default :) specially since it should be the case in the next Prometheus release (I hope)

Also I am not sure, but did we already enabled new UI by default? If not, we could too

I'm not sure as well, but when I tried locally I was on the old UI at the beginning. So I supposed the new UI is not enabled by default.

[Nexucis]

btw for the record, I'm relying on the code done here: prometheus/prometheus#8634

[Nexucis]

alright I enabled the experimental editor by default, but I didn't find how to enable the new ui by default :/. If you can point me what boolean I should change @bwplotka or @onprem , I would be greatful :D

I also managed to use the pathPrefix in the experimental editor (I forgot to use it). Not sure that's the correct way, so let me know if it's fine like that.

[Nexucis]

lol the e2e tests are failing again. Guess a retry will fix it ? :D

[onprem]

Hey @Nexucis, thanks for the PR!

is a strong requirement to support node v10 + v12 ? Or is it fine to drop them and move to node v14 ?

I'd say, no. At the time when these CI Jobs were added, Node v12 was the LTS version so we were testing on 10, 12, and 14. Now that the v14 is LTS, we can move the CI jobs to v14 and maybe v15.

enabled the experimental editor by default, but I didn't find how to enable the new ui by default :/. If you can point me what boolean I should change

There isn't a single variable which you can flip to make the new UI default. It will require a bit more work than that (changing prefixes in react routes, handling the go code that serves the UI etc). Let's do that in a separate PR.

[onprem]

This looks good to me but we have one additional step for UI related PRs, you have to run make assets in the repository root to update the bindata.go file with new assets and then just push those changes.

Lets merge this after that.

[Nexucis]

it's done @onprem thanks for review :).

Btw could be a good idea actually to generate the assets at the ci level (and so to ignore this file in git). Like that the update of the assets cannot be missed

[onprem]

Btw could be a good idea actually to generate the assets at the ci level (and so to ignore this file in git). Like that the update of the assets cannot be missed

Yes, we discussed that briefly during office hours I guess but there was no concrete discussion (like a GitHub issue) on that point. I think that moving make assets step to build-time will reduce a lot of friction on the contributors' side but there are a few cons as well, for example, Nodejs will be required for anyone who's building Thanos from the source.

[onprem]

LGTM!

[Nexucis]

there are a few cons as well, for example, Nodejs will be required for anyone who's building Thanos from the source.

well when you are starting to build from the source a project, then it's quite fine to install the language required to build it. As long as you are ready to install golang, installing nodejs shouldn't be an issue :D

Then if it's more a developer issue, like backend developer doesn't want to be bothered by nodejs, then adding a default index.html in case the folder dist doesn't exist should solve the issue. The build will pass and so you can focus on the backend only.