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

remove the old ui #4163

Closed
wants to merge 7 commits into from
Closed

Conversation

Nexucis
Copy link
Contributor

@Nexucis Nexucis commented May 4, 2021

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

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

Changes

Verification

Manual testing by starting the binary locally with the query/ store mode

Nexucis and others added 4 commits May 5, 2021 11:47
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>

Co-authored-by: Prem Saraswat <prmsrswt@gmail.com>
@Nexucis Nexucis force-pushed the feature/remove-old-ui branch from 34d83d5 to 56124ac Compare May 5, 2021 09:48
@Nexucis
Copy link
Contributor Author

Nexucis commented May 5, 2021

alright thanks to @onprem we managed to cleanup a bit more the code of the bucket UI.

I believe a refactoring of the way to serve the UI should be done. I'm feeling we can simplify things but I think that should be done in another PR. This cleanup is already good :D.
And also I think this refactoring should be done in Prometheus first.

So anyway we should be good to merge now :). And let's for the refactoring later.

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@bwplotka
Copy link
Member

bwplotka commented May 5, 2021

Perfect PR:
image ((:

And also I think this refactoring should be done in Prometheus first.

Do you suggest it's a requirement for this PR?

@Nexucis
Copy link
Contributor Author

Nexucis commented May 5, 2021

Oh no I mean if a refactoring should be performed to simplify the way to serve the UI, I believe it should be done first on Prometheus.

This PR doesn't imply any refactoring, so we are good to merge.
Removing the old Ui in Prometheus will be done as well, but there are some issues to solve before being able to do that (on Prometheus side)

Signed-off-by: Augustin Husson <augustin.husson@amadeus.com>
@Nexucis Nexucis force-pushed the feature/remove-old-ui branch from e75fd51 to 5a1ee84 Compare May 22, 2021 12:20
@Nexucis
Copy link
Contributor Author

Nexucis commented May 22, 2021

@onprem I updated the PR regarding the last changes on the main branches.
The only thing that remains is the build of the assets. I have some trouble on my current setup (nothing related to this PR), so I can't build it somehow.

But until I found why my laptop doesn't want to run this command, could you (again) take a look at it and tell me if it should be ok to merge it once the assets are updated ?

@Nexucis
Copy link
Contributor Author

Nexucis commented May 22, 2021

or if you want, since I allowed edits by maintainer of this PR, you can run the command and push it to the PR.

From my side, excepting the assets, it's ok to merge it.

@roidelapluie
Copy link

Be careful that the new UI has some big flaws:

prometheus/prometheus#8256

prometheus/prometheus#8256 (comment)

@Nexucis
Copy link
Contributor Author

Nexucis commented May 23, 2021

ah yeah the color issue can be a bit annoying indeed.
But in the other hand, the benefits of removing the old UI is quite interesting as well. And maybe when having only one UI will increase the contribution on this one only. ( yeah that's a bit an hostage situation hem :D)

If Thanos maintainer think it's a blocking issue and actually we should make some effort to fix it first on Prometheus side first ( the one you linked @roidelapluie and also others like prometheus/prometheus#6881), fine for me.

It can make sense to be able to remove the UI first on Prometheus side and then here. It will just take much more time

@onprem
Copy link
Member

onprem commented May 23, 2021

But until I found why my laptop doesn't want to run this command, could you (again) take a look at it and tell me if it should be ok to merge it once the assets are updated ?

No worries, I'll take a look at the assets part.

Also, I am in favor of delaying this PR a bit and try to get the issues mentioned by @roidelapluie fixed first. I agree that removing the old UI will remove a lot of friction on the development side, but I don't want to have that at the cost of user inconvenience.

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

Nexucis commented May 24, 2021

alright no problem :). I think I can decline this PR. We can reopen it or recreate it once it is the good time ^^. I don't see a good reason to keep it open

@Nexucis Nexucis closed this May 24, 2021
@Nexucis Nexucis deleted the feature/remove-old-ui branch November 26, 2021 17:17
@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 2, 2022

we removed the old ui in Prometheus. So we can restart this one \o/

@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 2, 2022

but probably you will want to get the perfomance improvement done in the Target, Alert and Service Discovery paged before removing it

@Nexucis Nexucis mentioned this pull request Feb 11, 2022
2 tasks
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