Make the ReactUI the default one #4081

Nexucis · 2021-04-20T10:31:51Z

This PR is switching the default UI from the old one to the ReactAPP by default.

bwplotka

Nice, thanks for the work! Looks great, just one main question.

pkg/ui/query.go

bwplotka

Looks good from outside. Have you tested it? (:

Nexucis · 2021-04-20T15:32:34Z

yes I just had some trouble to verify if everything is working fine for the thanos-ruler. (mainly because of my setup), but hopefully everything is fine.

And yeah hopefully for me I tested it, at the first shot, the old UI wasn't working anymore 😅

kakkoyun

LGTM AFAIT. I'd feel safer if we have a green light from @onprem as well?

onprem

Great work! Everything looks good but when testing locally I noticed that you missed the Block Viewer UI (pkg/ui/bucket.go). Let's update the routes there as well otherwise it'll break the block viewer.

Nexucis · 2021-04-20T21:20:05Z

ah yeah indeed, I completely missed the bucket part. Sorry about that, I will update it. Thanks !

Nexucis · 2021-04-20T21:39:53Z

Updated. The only big change is just there was an option to activate or not the new UI. Well like it is becoming the default one, I removed this boolean. Hope you don't mind.

Nexucis · 2021-04-20T22:45:16Z

pkg/ui/bucket.go

+	// If we are coming from the old UI, it will use the second route.
+	r.Get(path.Join("/classic", b.uiPrefix), instrf("bucket", ins, b.bucket))
+	r.WithPrefix(b.uiPrefix).Get("/classic", instrf("bucket", ins, b.bucket))
+	r.WithPrefix(b.uiPrefix).Get("/classic/static/*filepath", instrf("static", ins, b.serveStaticAsset))


this part is quite ugly, but, I fixed every possible path that you could follow. So we can now move from the old UI to the new one without any issue.

But definitely dropping the old UI will simplify a lot the way to serve the UI.

I didn't really find the good way to have the same route everywhere. It's mainly becaused the prefix of the UI is not managed in the same way on the old UI and in the UI. (At least that's what I am seing and what I understood)

Nexucis · 2021-04-20T22:52:40Z

looks like I succeeded to break the e2e test :/ no idea how. If someone has a clue on what I broke, that would be cool

kakkoyun · 2021-04-21T08:58:10Z

I'll run e2e tests locally to confirm if it's a flake or a real case.

kakkoyun · 2021-04-21T10:44:18Z

@Nexucis Could you please rebase one last time?

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

Nexucis · 2021-04-21T12:01:21Z

@kakkoyun I rebased the PR. But I still have the errors. Still strange, I will have to take a deeper look at it to understand what I broke.

kakkoyun · 2021-04-21T14:49:46Z

@Nexucis Let me know if anyways I can help. I want to have this in v0.20.0 :)

Nexucis · 2021-04-21T15:33:22Z

@kakkoyun well if you are able to find more quickly than me why it doesn't work I won't say no :D.

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

sonatype-lift · 2021-04-21T17:36:10Z

pkg/ui/bucket.go

 func (b *Bucket) root(w http.ResponseWriter, r *http.Request) {
 	b.executeTemplate(w, "bucket.html", GetWebPrefix(b.logger, path.Join(b.externalPrefix, strings.TrimPrefix(b.uiPrefix, "/")), b.prefixHeader, r), b)
 }

+// Handle / of bucket UIs.
+func (b *Bucket) bucket(w http.ResponseWriter, r *http.Request) {


U1000: func (*Bucket).bucket is unused
(at-me in a reply with help or ignore)

Nexucis · 2021-04-21T17:49:35Z

ah that's interesting, when I'm moving back the code in charge of the UI routing for the bucket, the errors relative to the bucket disappear

pkg/ui/bucket.go

onprem · 2021-04-21T20:27:31Z

The only big change is just there was an option to activate or not the new UI. Well like it is becoming the default one, I removed this boolean.

@Nexucis At that time I didn't really get what you meant but I got it now. What you removed was a boolean governing if the routes will be registered or not. This was because in compactor the Block Viewer UI is actually registered two times, this is an artifact from the old UI times. So, the old UI needed to be registered twice, but the new UI only needs to be registered only once. That's why that boolean was created and I think we need to keep that (at least until we get rid of the classic UI completely).

Nexucis · 2021-04-21T21:49:00Z

thanks a lot @onprem for the tips ! I will try that

onprem · 2021-04-21T22:07:04Z

@Nexucis I started playing with this and found out that there is a lot of work we need to do because there can be multiple instances of Block Viewer in a single component (compactor has two separate classic UIs running on /loaded and /global).

Take a look at this onprem@1862dce I made some changes on top of your work (modulo the last revert commit) to make this work. Feel free to use it however you want.

PS: I know this is hard to get right, but this complexity is somewhat expected as we have multiple UI and now on top of that, multiple instances of those UIs as well. This will get so much simpler after we remove the classic UI.

Nexucis · 2021-04-21T22:32:09Z

I started playing with this and found out that there is a lot of work we need to do because there can be multiple instances of Block Viewer in a single component (compactor has two separate classic UIs running on /loaded and /global).

By looking about your changes, I'm feeling you fix the issues. Or do you see others problems ?

Thanks btw for code you shared :)

onprem · 2021-04-22T07:36:11Z

Or do you see others problems ?

Nope, everything else looked perfectly fine to me.

kakkoyun · 2021-04-22T08:50:12Z

@Nexucis I guess we have solution now. Looking forward to it.

Nexucis · 2021-04-22T09:17:58Z

@kakkoyun yes totally :) thanks to @onprem ! I just have to merge what he did with my code and we will be good.

pkg/ui/bucket.go

Signed-off-by: Augustin Husson <husson.augustin@gmail.com> Co-authored-by: Prem Saraswat <prmsrswt@gmail.com>

Nexucis · 2021-04-22T12:00:21Z

alright I fixed the bucket. There is something odd in the bucket somehow if the template {{ uiPrefix }} is used in bucket.html and bucket_menu.html, then it is replaced by an empty string whatever is put in tmplFuncs.

But the {{ pathPrefix }} is correctly replaced so I injected the path /classic/... in the pathPrefix instead. Hope it is fine.

So if we forget about this strange things, it should be fine now.

Nexucis · 2021-04-22T12:03:03Z

by looking at the e2e test error, it doesn't look like it is related to what I did, but I'm maybe wrong here :D. So many things are strange when changing the way to serve the UI.

Nexucis · 2021-04-22T13:27:46Z

and by looking at another PR (#4094) , it looks like the e2e test are not working well too with the same error than here. Sooooo I would say all good.

kakkoyun · 2021-04-22T14:07:32Z

The Github CI seems broken, failures are not related, so mering this one.

Nexucis force-pushed the feature/switch-ui branch from 62bb1c8 to 6682e0e Compare April 20, 2021 10:32

bwplotka reviewed Apr 20, 2021

View reviewed changes

pkg/ui/query.go Show resolved Hide resolved

bwplotka previously approved these changes Apr 20, 2021

View reviewed changes

bwplotka requested a review from kakkoyun April 20, 2021 15:28

kakkoyun previously approved these changes Apr 20, 2021

View reviewed changes

onprem reviewed Apr 20, 2021

View reviewed changes

Nexucis dismissed stale reviews from kakkoyun and bwplotka via 2ab2a74 April 20, 2021 21:38

Nexucis force-pushed the feature/switch-ui branch 2 times, most recently from 511f497 to e3e24b7 Compare April 20, 2021 22:39

Nexucis commented Apr 20, 2021

View reviewed changes

Nexucis added 5 commits April 21, 2021 13:38

make the reactUI the default one

2b390c3

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

prefix all static path to having back the classic UI

96a365c

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

add missing react route

3ec9b62

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

remove unused method

deeac34

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

update bucket ui to use the react app by default

0ec81af

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

Nexucis force-pushed the feature/switch-ui branch from e3e24b7 to 0ec81af Compare April 21, 2021 11:44

rollback the bucket to see if it fixed the e2e

991cf2e

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

sonatype-lift bot reviewed Apr 21, 2021

View reviewed changes

onprem reviewed Apr 21, 2021

View reviewed changes

pkg/ui/bucket.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Apr 22, 2021

View reviewed changes

pkg/ui/bucket.go Outdated Show resolved Hide resolved

fix bucket display

d7643c3

Signed-off-by: Augustin Husson <husson.augustin@gmail.com> Co-authored-by: Prem Saraswat <prmsrswt@gmail.com>

Nexucis force-pushed the feature/switch-ui branch from 3dc8cbe to d7643c3 Compare April 22, 2021 11:51

kakkoyun merged commit 393a471 into thanos-io:main Apr 22, 2021

Nexucis deleted the feature/switch-ui branch April 22, 2021 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the ReactUI the default one #4081

Make the ReactUI the default one #4081

Nexucis commented Apr 20, 2021

bwplotka left a comment

bwplotka left a comment

Nexucis commented Apr 20, 2021

kakkoyun left a comment

onprem left a comment

Nexucis commented Apr 20, 2021

Nexucis commented Apr 20, 2021

Nexucis Apr 20, 2021

Nexucis commented Apr 20, 2021

kakkoyun commented Apr 21, 2021

kakkoyun commented Apr 21, 2021

Nexucis commented Apr 21, 2021

kakkoyun commented Apr 21, 2021

Nexucis commented Apr 21, 2021

sonatype-lift bot Apr 21, 2021

Nexucis commented Apr 21, 2021

onprem commented Apr 21, 2021

Nexucis commented Apr 21, 2021

onprem commented Apr 21, 2021 •

edited

Loading

Nexucis commented Apr 21, 2021

onprem commented Apr 22, 2021

kakkoyun commented Apr 22, 2021

Nexucis commented Apr 22, 2021

Nexucis commented Apr 22, 2021

Nexucis commented Apr 22, 2021

Nexucis commented Apr 22, 2021 •

edited

Loading

kakkoyun commented Apr 22, 2021 •

edited

Loading

Make the ReactUI the default one #4081

Make the ReactUI the default one #4081

Conversation

Nexucis commented Apr 20, 2021

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Nexucis commented Apr 20, 2021

kakkoyun left a comment

Choose a reason for hiding this comment

onprem left a comment

Choose a reason for hiding this comment

Nexucis commented Apr 20, 2021

Nexucis commented Apr 20, 2021

Nexucis Apr 20, 2021

Choose a reason for hiding this comment

Nexucis commented Apr 20, 2021

kakkoyun commented Apr 21, 2021

kakkoyun commented Apr 21, 2021

Nexucis commented Apr 21, 2021

kakkoyun commented Apr 21, 2021

Nexucis commented Apr 21, 2021

sonatype-lift bot Apr 21, 2021

Choose a reason for hiding this comment

Nexucis commented Apr 21, 2021

onprem commented Apr 21, 2021

Nexucis commented Apr 21, 2021

onprem commented Apr 21, 2021 • edited Loading

Nexucis commented Apr 21, 2021

onprem commented Apr 22, 2021

kakkoyun commented Apr 22, 2021

Nexucis commented Apr 22, 2021

Nexucis commented Apr 22, 2021

Nexucis commented Apr 22, 2021

Nexucis commented Apr 22, 2021 • edited Loading

kakkoyun commented Apr 22, 2021 • edited Loading

onprem commented Apr 21, 2021 •

edited

Loading

Nexucis commented Apr 22, 2021 •

edited

Loading

kakkoyun commented Apr 22, 2021 •

edited

Loading