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.

Sign up for GitHub

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

Jump to bottom

Update static UI assets path in contrib doc #2548

Merged
merged 2 commits into from
Oct 9, 2020
Merged

Update static UI assets path in contrib doc #2548

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0f4db3
Update static ui assets path in contrib doc
Oct 9, 2020
c1281ca
Replace local build section with make run-all-in-one
Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,14 @@ Then install dependencies and run the tests:
git submodule update --init --recursive
make install-tools
make test
#if you wish to build platform binaries locally - the step below is needed.
# if you wish to build platform binaries locally - the step below is needed.
make build-ui
```

### Running local build with the UI

The `jaeger-ui` submodule contains 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`, e.g.:
Copy link
Member

Choose a reason for hiding this comment

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

The make target is great, but why remove the explanation? It's valuable information that now someone needs to dig out by reading source code.

Copy link
Member

Choose a reason for hiding this comment

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

Not to mention that this section is about the UI, not all-in-one, i.e the same approach is used with query-service.

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 is a valid point, @yurishkuro; how the UI assets are created and used deserves a more detailed explanation as it's not straightforward (at least to me) to understand from just looking at code and Makefiles.

I think though, that this instruction should belong in the Pre-requisites section as an explanation to what make build-ui does. So my proposal is (what do you think?):

Pre-requisites

Then install dependencies and run the tests:

git submodule update --init --recursive
make install-tools
make test

The jaeger-ui submodule contains 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:

# if you wish to build platform binaries locally - the step below is needed.
make build-ui

Running local build with the UI

The packaged assets (created by the make build-ui step above) can be enabled by providing a build tag ui. The following command will start Jaeger all-in-one with this ui tag.

make run-all-in-one

Copy link
Member

Choose a reason for hiding this comment

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

I prefer UI-related explanations to be together under one section.


```
$ go run -tags ui ./cmd/all-in-one/main.go
```

Alternatively, the path to the built UI assets can be provided via `--query.static-files` flag:

```
$ go run ./cmd/all-in-one/main.go --query.static-files jaeger-ui/build
$ make run-all-in-one
```

## Project Structure
Expand Down