-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
ui: replace go-bindata with embed #3816
Conversation
Why with go-bindata, we ignored |
One check blocked by prometheus/golang-builder#109 |
Source maps increase the binary's size by a lot so that would hurt more than add benefits, and I don't remember about bootstrap. IIRC it's because we use the minified versions in the final build:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!!! Thanks @longngn
However could you please extract to go upgrade part into another PR? And follow https://github.com/thanos-io/thanos/blob/master/docs/contributing/how-to-change-go-version.md
It makes life easier for us to track down the issues.
@longngn Let's rebase and move forward with this as well. |
Hey @longngn 👋 We're going to cut a release candidate soon, is there any chance for you to push this to the finish line? |
@kakkoyun I've rebased with the main branch. AFAIK there's no way to exclude certain pattern with go embed, so the resulting binary might be heavier. On my Mac the binary size is 67MB. |
@longngn Thanks for the heads up. |
Is this something we can tell the Go I don't think 67MB is considered big, but for safety I would release Thanos 0.20 without it. WDYT? |
We still have a build process that produces the build artifacts of the react app, no? What's the problem with deleting everything except the resulting artifact for release binaries? |
I think we are excluding the source-maps from the build artifacts currently which contributes to the size reduction. Since we are deleting these source-maps anyways, I vote for not even generating them (this can be done setting env variable |
I just did three builds locally,
There's about 5M of difference between bindata (without sourcemaps) and go-embed without sourcemaps which sounds acceptable to me. |
@longngn Could you please rebase? |
Signed-off-by: Nguyen Le Vu Long <vulongvn98@gmail.com>
Signed-off-by: Nguyen Le Vu Long <vulongvn98@gmail.com>
Signed-off-by: Nguyen Le Vu Long <vulongvn98@gmail.com>
Signed-off-by: Nguyen Le Vu Long <vulongvn98@gmail.com>
Signed-off-by: Nguyen Le Vu Long <vulongvn98@gmail.com>
Done. |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Fixes #3782
Changes
Verification
thanos query
andthanos tools bucket web
and saw no error