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

Make the ReactUI the default one #4081

Merged
merged 7 commits into from
Apr 22, 2021
Merged

Conversation

Nexucis
Copy link
Contributor

@Nexucis Nexucis commented Apr 20, 2021

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

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

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

pkg/ui/query.go Show resolved Hide resolved
bwplotka
bwplotka previously approved these changes Apr 20, 2021
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

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

@bwplotka bwplotka requested a review from kakkoyun April 20, 2021 15:28
@Nexucis
Copy link
Contributor Author

Nexucis commented Apr 20, 2021

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
kakkoyun previously approved these changes Apr 20, 2021
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

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

Nexucis commented Apr 20, 2021

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

@Nexucis Nexucis dismissed stale reviews from kakkoyun and bwplotka via 2ab2a74 April 20, 2021 21:38
@Nexucis
Copy link
Contributor Author

Nexucis commented Apr 20, 2021

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 Nexucis force-pushed the feature/switch-ui branch 2 times, most recently from 511f497 to e3e24b7 Compare April 20, 2021 22:39
pkg/ui/bucket.go Outdated
// 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))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Nexucis commented Apr 20, 2021

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

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

@kakkoyun
Copy link
Member

@Nexucis Could you please rebase one last time?

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@Nexucis
Copy link
Contributor Author

Nexucis commented Apr 21, 2021

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

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

@Nexucis
Copy link
Contributor Author

Nexucis commented Apr 21, 2021

@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>
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) {
Copy link

Choose a reason for hiding this comment

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

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

@Nexucis
Copy link
Contributor Author

Nexucis commented Apr 21, 2021

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 Outdated Show resolved Hide resolved
@onprem
Copy link
Member

onprem commented Apr 21, 2021

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

Nexucis commented Apr 21, 2021

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

@onprem
Copy link
Member

onprem commented Apr 21, 2021

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

Nexucis commented Apr 21, 2021

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

onprem commented Apr 22, 2021

Or do you see others problems ?

Nope, everything else looked perfectly fine to me.

@kakkoyun
Copy link
Member

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

@Nexucis
Copy link
Contributor Author

Nexucis commented Apr 22, 2021

@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 Outdated Show resolved Hide resolved
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

Co-authored-by: Prem Saraswat <prmsrswt@gmail.com>
@Nexucis
Copy link
Contributor Author

Nexucis commented Apr 22, 2021

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

Nexucis commented Apr 22, 2021

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

Nexucis commented Apr 22, 2021

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

kakkoyun commented Apr 22, 2021

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

@kakkoyun kakkoyun merged commit 393a471 into thanos-io:main Apr 22, 2021
@Nexucis Nexucis deleted the feature/switch-ui branch April 22, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants