From 7df69752f33878f107e93b44d192b706a256a8c9 Mon Sep 17 00:00:00 2001 From: Mahad Zaryab <43658574+mahadzaryab1@users.noreply.github.com> Date: Sun, 1 Sep 2024 23:55:27 -0400 Subject: [PATCH] Simplify Bundling of UI assets (#5917) ## Which problem is this PR solving? - Closes #5913 ## Description of the changes - Created one file `assets.go` that holds both the `placeholder` and `actual` assets. - Added a helper `GetStaticFiles` to get the assets based on whether the actual filesystem has an `index.html.gz`. If it does, we return the assets. Otherwise, we return the placeholder `index.html` file. ## How was this change tested? Added unit tests and performed the following manual tests ### Test 1 Makefile Target ```Makefile .PHONY: run-all-in-one run-all-in-one: go run ./cmd/all-in-one --log-level debug ``` Clearing out the `actual` directory and ran `make run-all-in-one`. Going to `http://localhost:16686/` renders the placeholder HTML page. ### Test 2 Makefile Target ```Makefile .PHONY: run-all-in-one run-all-in-one: build-ui go run ./cmd/all-in-one --log-level debug ``` Ran `make run-all-in-one`. Going to `http://localhost:16686/` renders the Jaeger UI. ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Mahad Zaryab --- CONTRIBUTING.md | 6 --- Makefile | 2 +- Makefile.BuildBinaries.mk | 3 -- cmd/query/app/static_handler.go | 6 +-- cmd/query/app/static_handler_test.go | 15 ++++-- cmd/query/app/ui/actual.go | 21 -------- cmd/query/app/ui/assets.go | 32 ++++++++++++ cmd/query/app/ui/assets_test.go | 49 ++++++++++++++++++ cmd/query/app/ui/doc.go | 5 +- cmd/query/app/ui/placeholder.go | 20 ------- cmd/query/app/ui/placeholder/index.html | 2 +- .../app/ui/testdata/actual/index.html.gz | Bin 0 -> 211 bytes cmd/query/app/ui/testdata/empty_test.go | 14 +++++ cmd/query/app/ui/testdata/mock.go | 9 ++++ 14 files changed, 121 insertions(+), 63 deletions(-) delete mode 100644 cmd/query/app/ui/actual.go create mode 100644 cmd/query/app/ui/assets.go create mode 100644 cmd/query/app/ui/assets_test.go delete mode 100644 cmd/query/app/ui/placeholder.go create mode 100644 cmd/query/app/ui/testdata/actual/index.html.gz create mode 100644 cmd/query/app/ui/testdata/empty_test.go create mode 100644 cmd/query/app/ui/testdata/mock.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 50cef5cfe49..ea54b85e259 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,12 +76,6 @@ the source code for the UI assets (requires Node.js 6+). The assets must be compiled first with `make build-ui`, which runs Node.js build and then packages the assets into a Go file that is `.gitignore`-ed. -The packaged assets can be enabled by providing a build tag `ui`, for example: - -``` -$ go run -tags ui ./cmd/all-in-one/main.go -``` - `make run-all-in-one` essentially runs Jaeger all-in-one by combining both of the above steps into a single `make` command. diff --git a/Makefile b/Makefile index ffad18dee8d..d1c8fbf2460 100644 --- a/Makefile +++ b/Makefile @@ -186,7 +186,7 @@ lint-go: $(LINT) .PHONY: run-all-in-one run-all-in-one: build-ui - go run -tags ui ./cmd/all-in-one --log-level debug + go run ./cmd/all-in-one --log-level debug .PHONY: changelog changelog: diff --git a/Makefile.BuildBinaries.mk b/Makefile.BuildBinaries.mk index f82d3d4042d..bade512f140 100644 --- a/Makefile.BuildBinaries.mk +++ b/Makefile.BuildBinaries.mk @@ -74,13 +74,11 @@ _build-a-binary-%: .PHONY: build-jaeger build-jaeger: BIN_NAME = jaeger -build-jaeger: GO_TAGS = -tags ui build-jaeger: BUILD_INFO = $(BUILD_INFO_V2) build-jaeger: build-ui _build-a-binary-jaeger$(SUFFIX)-$(GOOS)-$(GOARCH) .PHONY: build-all-in-one build-all-in-one: BIN_NAME = all-in-one -build-all-in-one: GO_TAGS = -tags ui build-all-in-one: build-ui _build-a-binary-all-in-one$(SUFFIX)-$(GOOS)-$(GOARCH) .PHONY: build-agent @@ -89,7 +87,6 @@ build-agent: _build-a-binary-agent$(SUFFIX)-$(GOOS)-$(GOARCH) .PHONY: build-query build-query: BIN_NAME = query -build-query: GO_TAGS = -tags ui build-query: build-ui _build-a-binary-query$(SUFFIX)-$(GOOS)-$(GOARCH) .PHONY: build-collector diff --git a/cmd/query/app/static_handler.go b/cmd/query/app/static_handler.go index 32852e392a5..02faad69dce 100644 --- a/cmd/query/app/static_handler.go +++ b/cmd/query/app/static_handler.go @@ -76,15 +76,11 @@ type loadedConfig struct { // NewStaticAssetsHandler returns a StaticAssetsHandler func NewStaticAssetsHandler(staticAssetsRoot string, options StaticAssetsHandlerOptions) (*StaticAssetsHandler, error) { - assetsFS := ui.StaticFiles + assetsFS := ui.GetStaticFiles(options.Logger) if staticAssetsRoot != "" { assetsFS = http.Dir(staticAssetsRoot) } - if options.Logger == nil { - options.Logger = zap.NewNop() - } - h := &StaticAssetsHandler{ options: options, assetsFS: assetsFS, diff --git a/cmd/query/app/static_handler_test.go b/cmd/query/app/static_handler_test.go index 9a07ca506f1..f8ed15ce5a7 100644 --- a/cmd/query/app/static_handler_test.go +++ b/cmd/query/app/static_handler_test.go @@ -29,7 +29,9 @@ import ( //go:generate mockery -all -dir ../../../pkg/fswatcher func TestNotExistingUiConfig(t *testing.T) { - handler, err := NewStaticAssetsHandler("/foo/bar", StaticAssetsHandlerOptions{}) + handler, err := NewStaticAssetsHandler("/foo/bar", StaticAssetsHandlerOptions{ + Logger: zap.NewNop(), + }) require.Error(t, err) assert.Contains(t, err.Error(), "no such file or directory") assert.Nil(t, handler) @@ -155,11 +157,18 @@ func TestRegisterStaticHandler(t *testing.T) { } func TestNewStaticAssetsHandlerErrors(t *testing.T) { - _, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{UIConfigPath: "fixture/invalid-config"}) + _, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{ + UIConfigPath: "fixture/invalid-config", + Logger: zap.NewNop(), + }) require.Error(t, err) for _, base := range []string{"x", "x/", "/x/"} { - _, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{UIConfigPath: "fixture/ui-config.json", BasePath: base}) + _, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{ + UIConfigPath: "fixture/ui-config.json", + BasePath: base, + Logger: zap.NewNop(), + }) require.Errorf(t, err, "basePath=%s", base) assert.Contains(t, err.Error(), "invalid base path") } diff --git a/cmd/query/app/ui/actual.go b/cmd/query/app/ui/actual.go deleted file mode 100644 index a8b28acef20..00000000000 --- a/cmd/query/app/ui/actual.go +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright (c) 2018 The Jaeger Authors. -// SPDX-License-Identifier: Apache-2.0 - -//go:build ui -// +build ui - -package ui - -import ( - "embed" - "net/http" - - "github.com/jaegertracing/jaeger/pkg/gzipfs" - "github.com/jaegertracing/jaeger/pkg/httpfs" -) - -//go:embed actual/* -var assetsFS embed.FS - -// StaticFiles provides http filesystem with static files for UI. -var StaticFiles = httpfs.PrefixedFS("actual", http.FS(gzipfs.New(assetsFS))) diff --git a/cmd/query/app/ui/assets.go b/cmd/query/app/ui/assets.go new file mode 100644 index 00000000000..39c72bc524e --- /dev/null +++ b/cmd/query/app/ui/assets.go @@ -0,0 +1,32 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package ui + +import ( + "embed" + "net/http" + + "go.uber.org/zap" + + "github.com/jaegertracing/jaeger/pkg/gzipfs" + "github.com/jaegertracing/jaeger/pkg/httpfs" +) + +//go:embed actual/* +var actualAssetsFS embed.FS + +//go:embed placeholder/index.html +var placeholderAssetsFS embed.FS + +// GetStaticFiles gets the static assets that the Jaeger UI will serve. If the actual +// assets are available, then this function will return them. Otherwise, a +// non-functional index.html is returned to be used as a placeholder. +func GetStaticFiles(logger *zap.Logger) http.FileSystem { + if _, err := actualAssetsFS.ReadFile("actual/index.html.gz"); err != nil { + logger.Warn("ui assets not embedded in the binary, using a placeholder", zap.Error(err)) + return httpfs.PrefixedFS("placeholder", http.FS(placeholderAssetsFS)) + } + + return httpfs.PrefixedFS("actual", http.FS(gzipfs.New(actualAssetsFS))) +} diff --git a/cmd/query/app/ui/assets_test.go b/cmd/query/app/ui/assets_test.go new file mode 100644 index 00000000000..4b61ee80ca4 --- /dev/null +++ b/cmd/query/app/ui/assets_test.go @@ -0,0 +1,49 @@ +// Copyright (c) 2024 The Jaeger Authors. +// SPDX-License-Identifier: Apache-2.0 + +package ui + +import ( + "embed" + "io" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/jaegertracing/jaeger/cmd/query/app/ui/testdata" + "github.com/jaegertracing/jaeger/pkg/testutils" +) + +func TestGetStaticFiles_ReturnsPlaceholderWhenActualNotPresent(t *testing.T) { + // swap out the assets FS for an empty one and then replace it back when the + // test completes + currentActualAssets := actualAssetsFS + actualAssetsFS = embed.FS{} + t.Cleanup(func() { actualAssetsFS = currentActualAssets }) + + logger, logBuf := testutils.NewLogger() + fs := GetStaticFiles(logger) + file, err := fs.Open("/index.html") + require.NoError(t, err) + bytes, err := io.ReadAll(file) + require.NoError(t, err) + require.Contains(t, string(bytes), "This is a placeholder for the Jaeger UI home page") + require.Contains(t, logBuf.String(), "ui assets not embedded in the binary, using a placeholder") +} + +func TestGetStaticFiles_ReturnsActualWhenPresent(t *testing.T) { + // swap out the assets FS for a dummy one and then replace it back when the + // test completes + currentActualAssets := actualAssetsFS + actualAssetsFS = testdata.TestFS + t.Cleanup(func() { actualAssetsFS = currentActualAssets }) + + logger, logBuf := testutils.NewLogger() + fs := GetStaticFiles(logger) + file, err := fs.Open("/index.html") + require.NoError(t, err) + bytes, err := io.ReadAll(file) + require.NoError(t, err) + require.NotContains(t, string(bytes), "This is a placeholder for the Jaeger UI home page") + require.NotContains(t, logBuf.String(), "ui assets not embedded in the binary, using a placeholder") +} diff --git a/cmd/query/app/ui/doc.go b/cmd/query/app/ui/doc.go index b54f61e4108..059e119a509 100644 --- a/cmd/query/app/ui/doc.go +++ b/cmd/query/app/ui/doc.go @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 // Package ui bundles UI assets packaged with stdlib/embed. -// By default it imports the placeholder, non-functional index.html. -// When building with "-tags ui", it imports the real UI assets -// generated in build-ui Makefile target. +// If the real UI assets are available, they are embedded and +// used at runtime. Otherwise, the non-functional index.html is used. package ui diff --git a/cmd/query/app/ui/placeholder.go b/cmd/query/app/ui/placeholder.go deleted file mode 100644 index 24d8ec8d669..00000000000 --- a/cmd/query/app/ui/placeholder.go +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) 2018 The Jaeger Authors. -// SPDX-License-Identifier: Apache-2.0 - -//go:build !ui -// +build !ui - -package ui - -import ( - "embed" - "net/http" - - "github.com/jaegertracing/jaeger/pkg/httpfs" -) - -//go:embed placeholder/index.html -var assetsFS embed.FS - -// StaticFiles provides http filesystem with static files for UI. -var StaticFiles = httpfs.PrefixedFS("placeholder", http.FS(assetsFS)) diff --git a/cmd/query/app/ui/placeholder/index.html b/cmd/query/app/ui/placeholder/index.html index 35dcd6369a4..8ae137b65c8 100644 --- a/cmd/query/app/ui/placeholder/index.html +++ b/cmd/query/app/ui/placeholder/index.html @@ -7,7 +7,7 @@

🥋: This is not the Jaeger UI you are looking for!

This is a placeholder for the Jaeger UI home page. - If you are seeing this, you are running a binary that was not compiled with the UI assets (-tags ui). + If you are seeing this, you are running a binary that was not compiled with the UI assets. See these instructions.

UI configuration

diff --git a/cmd/query/app/ui/testdata/actual/index.html.gz b/cmd/query/app/ui/testdata/actual/index.html.gz new file mode 100644 index 0000000000000000000000000000000000000000..d4c9c2742737f963e39e5c3c769821be5f96be3c GIT binary patch literal 211 zcmV;^04)C>iwFqsE!AcK18Ht#Wq2-VbZu+^HIGjVfoPB{4#(v1y5Y zk;EH&{$E|OCD0Asd?5v7$Y>YUol>?Lo4h|?E{@?PIqZZSBV4edAf}cf!km;GjFDGo zA87ew$QLEP?N^bpc46i?kGBV9$1Exr9Ka{n-dN%n)YD)~#?)}C8