Simplify contributing.md

[albertteoh]

This proposal is motivated by a PR comment originally posted by @yurishkuro in #2548 (comment).

Requirement - what kind of business use case are you trying to solve?

• Bring back the explanation of what make build-ui does under the hood, which contains valuable information for contributors.
• Reduce the number of manual steps required to get started in contributing.md instructions to at least:

make install-tools
make test

• More correct make targets:
  • Only run build-ui when UI assets are required.
  • Clean up redundant make target dependencies and only assign the build-ui dependency to the most specific targets.
  • Add missing build-ui dependencies to make targets that depend on UI assets, e.g. build-all-in-one should have a dependency on UI assets, but why only build-all-in-one-linux takes the build-ui dependency?

Problem - what in Jaeger blocks you from solving the requirement?

contributing.md currently requires these prerequisite steps:

git submodule update --init --recursive
make install-tools
make test
# if you wish to build platform binaries locally - the step below is needed.
make build-ui

git submodule update --init --recursive and make build-ui are specific to UI builds. Instead of making these prerequisites, could we make these an explicit dependency within our Makefile and only run them when performing UI-related builds?

This has the added benefit of automating these steps where required and, IMHO, is more correct. Consider the following scenarios:

• Only run/build non-UI components; for example: testing agent->collector->kafka and validating with a consumer.
• The jaeger-ui submodule commit is modified in upstream during development and we rebase to pick these changes up.

Proposal - what do you suggest to solve the problem or improve the existing situation?

• Reinstate what make build-ui does under the hood (which was removed in Update static UI assets path in contrib doc #2548) within the Makefile as part of the build-ui target, similar to the proto make target.

• Add build-ui dependency to the "lowest common denominator" make targets, namely build-query, build-all-in-one and run-all-in-one.

• yarn install and yarn build steps take well over a minute to complete even after prior calls, which is particularly undesirable for something like make run-all-in-one.

  • Make build-ui idempotent by checking for existence of the asset files (cmd/query/app/ui/*/gen_assets.go). If there's a need to force UI asset rebuilds, either delete asset files via the clean target (this sounds nicer?) or introduce a build-ui-force option target.

• As above, move build-ui dependency from build-all-in-one-linux to the more general build-all-in-one target to fix other OS builds.

• Add git submodule update --init --recursive as part of the build-ui target given their fates are tied to one another (unless if I'm mistaken). This call is idempotent and so is relatively quick to complete after making the initial call. Added benefit of ensuring the correct commit is used in each build/run if it was modified in the upstream master.

• Remove redundant build-ui from docker target given it is introduced as a dependency in build-query (build-binaries-linux -> build-platform-binaries -> build-query -> build-ui).

• Simplify the contributing.md Prerequisites section down to:

make install-tools
make test

Any open questions to address

• Maybe there are good reasons to having git submodule update... and make build-ui as explicit prerequisite steps that I have overlooked?
• What is the reason for requiring make test as a prerequisite?

[yurishkuro]

Add missing build-ui dependencies to make targets that depend on UI assets, e.g. build-all-in-one should have a dependency on UI assets, but why only build-all-in-one-linux takes the build-ui dependency?

I can answer this one: build-ui is incredibly expensive to run, takes several minutes, and is actually rarely needed unless it's part of the CI run. If the target itself was sensitive to changes in the jaeger-ui directory, then it would probably be ok to add it as dependency in many places.

[yurishkuro]

Add git submodule update --init --recursive as part of the build-ui target

I am not in favor of this. A target that verifies that jaeger-ui submodule has been initialized would be preferable. Doing a forced update makes it difficult to test small UI changes where the submodule may not be on the committed version.

[yurishkuro]

Maybe there are good reasons to having git submodule update.. and make build-ui as explicit prerequisite steps that I have overlooked?

afaik they were not prerequisites, but just included under "how to build UI" sections.

What is the reason for requiring make test as a prerequisite?

I think it generally validates that the local environment is properly set up. After all we're talking about contributing to Jaeger, if someone can't run 'make test' for whatever reason, they are probably not ready to contribute (we do not want to encourage using Travis for testing little fixes). For comparison, jaeger-ui project even has a commit hook that forces you to run yarn test before committing.

[yurishkuro]

+1 on most suggestions.