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

Upgrade ui #1066

Merged
merged 7 commits into from May 15, 2019
Merged

Upgrade ui #1066

merged 7 commits into from May 15, 2019

Conversation

jojohappy
Copy link
Member

Changes

fixes: #1042
Upgrade the Thanos ui to the Prometheus 2.9.1, new style of ui theme would be used, although I don't like it.

Verification

To visit the Thanos Query and Rule page.

@adrien-f Please help me to verify the store page, you are the original committer, thanks!

@bwplotka @povilasv Please help me to verify all of the pages, especially the rule page, I have not a rule component, thanks!

@povilasv
Copy link
Member

@jojohappy nice work!

Add an entry to chanelog please :)

Do you have a docker image in dockerhub with this commit so that I can play around with it?

@bwplotka
Copy link
Member

Nice!

@jojohappy
Copy link
Member Author

@povilasv @bwplotka @adrien-f you can get the image by jojohappy/thanos:upgrade_ui. Thanks!

@GiedriusS
Copy link
Member

Nice but it's kind of hard to review so many different (generated) files. Could you tell us how did you do this? Did you copy/paste over the relevant files? 😄

type="button"
name="dec_end"
title="Rewind the end time.">
<i class="glyphicon glyphicon-backward"></i>
</button>
</button><!--
Copy link
Member

Choose a reason for hiding this comment

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

do we need this comment, for example? that's why I'm wondering 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Just following the upstream (: I don't know why it was not deleted by Prometheus too. ):

@@ -15,15 +13,21 @@

<script src="{{ pathPrefix }}/static/vendor/mustache/mustache.min.js?v={{ buildVersion }}"></script>
<script src="{{ pathPrefix }}/static/vendor/js/jquery.selection.js?v={{ buildVersion }}"></script>
<script src="{{ pathPrefix }}/static/vendor/js/jquery.hotkeys.js?v={{ buildVersion }}"></script>
<!-- <script src="{{ pathPrefix }}/static/vendor/js/jquery.hotkeys.js?v={{ buildVersion }}"></script> -->
Copy link
Member

Choose a reason for hiding this comment

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

why is this commented out too, for example?

@jojohappy
Copy link
Member Author

jojohappy commented Apr 24, 2019

@GiedriusS Maybe we could play around this PATCH by the image jojohappy/thanos:upgrade_ui (:

Because we are following the Prometheus ui, as you said, I copied and pasted the relevant files and upgraded the frontend vendors for bootstrap 4.0. Exclude the store page, it wrote by @adrien-f . I just updated the style of label element.

The biggest changes for this PR is bootstrap, so a lot of style of dom elements would be updated, such as navbar and labels.

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.

Thanks for this @jojohappy Looking good so far, will build docker image to test this out for all.

.gitignore Outdated
@@ -1,6 +1,7 @@
/prometheus
/thanos
vendor/
!pkg/ui/static/vendor
Copy link
Member

Choose a reason for hiding this comment

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

Let's change the upper vendor to /vendor/ then instead of this ?

@bwplotka
Copy link
Member

Are we sure the security patch for query history issue is there as well?

@jojohappy
Copy link
Member Author

jojohappy commented Apr 25, 2019

Are we sure the security patch for query history issue is there as well?

@bwplotka I don't know what security patch you said, but I searched the list of prometheus issues, is this issue you mentioned?

@GiedriusS
Copy link
Member

GiedriusS commented May 10, 2019

I'd love to continue with this PR since it would solve an important pain point. @jojohappy, could you please point me from which exact commit in https://github.com/prometheus/prometheus you've pulled this from? I'd love to double check.

@povilasv
Copy link
Member

I've tested Thanos Rule and I get template: :64: function "since" not defined when opening /rules page.

@povilasv
Copy link
Member

povilasv commented May 10, 2019

apart from that it looks awesome

2019-05-10-162828

2019-05-10-162836

2019-05-10-163110

@jojohappy
Copy link
Member Author

I'd love to continue with this PR since it would solve an important pain point. @jojohappy, could you please point me from which exact commit in https://github.com/prometheus/prometheus you've pulled this from? I'd love to double check.

Thanks @GiedriusS ! I followed the ui of Prometheus v2.9.1: commit.

Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
@jojohappy
Copy link
Member Author

@povilasv @GiedriusS I have updated the image for this PR: jojohappy/thanos:upgrade_ui and fixed the missing template function. PTAL, thanks!

@adrien-f
Copy link
Member

Awesome work 🎉 !

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Double checked with that commit and everything seems clean AFAICT, and it works much better with the updated versions! Thank you a lot ❤️

@povilasv
Copy link
Member

🥇 amazing

2019-05-14-154408

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.

LGTM, I resolved conflicts and will merge on green & deploy it to our clusters (:

@bwplotka bwplotka merged commit 7a85812 into thanos-io:master May 15, 2019
@bwplotka
Copy link
Member

Thanks for massive work on this @jojohappy !! Really great help ❤️

@povilasv
Copy link
Member

povilasv commented May 16, 2019

❤️ @jojohappy , can't wait to run it in production :)

@jojohappy jojohappy deleted the upgrade_ui branch August 21, 2019 02:47
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.

ui: Upgrade Thanos UI to newest fixes and improvements from upstream
5 participants