Add github action for jaeger all-in-one image

[Ashmita152]

Signed-off-by: Ashmita Bohara [email protected]

Which problem is this PR solving?

This PR adds the build job of all-in-one image to github actions.

Part of #2645

Short description of the changes

1. To make the docker login action work we need to add two environment variables like this

2. To get the branch name we are using github action https://github.com/nelonoel/branch-name which populates env var BRANCH_NAME

3. I tried using nodejs official github action (https://github.com/actions/setup-node) instead of calling scripts/travis/install-ui-deps.sh but it didn't work and I wasn't able to make it work after several retries. So thought do it in the same way how we do in travis for now.

[Ashmita152]

Until we add two environment variables, the build will fail.

Sample Success Run: https://github.com/Ashmita152/jaeger/pull/8/checks?check_run_id=1466574790

[codecov]

Codecov Report

Merging #2663 (712331d) into master (556e3ba) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2663      +/-   ##
==========================================
+ Coverage   95.10%   95.11%   +0.01%     
==========================================
  Files         213      213              
  Lines        9491     9491              
==========================================
+ Hits         9026     9027       +1     
+ Misses        388      387       -1     
  Partials       77       77              

Impacted Files	Coverage Δ
plugin/storage/integration/integration.go	77.90% <0.00%> (+0.55%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 556e3ba...712331d. Read the comment docs.

[yurishkuro]

I tried using nodejs official github action (https://github.com/actions/setup-node) instead of calling scripts/travis/install-ui-deps.sh but it didn't work

Could you include some details about what didn't work?

Not a big deal, I think it's fine to use scripts/travis/install-ui-deps.sh, but if you already spent some time investigating it's good to record the findings in case someone wants to try.

BTW, once we're completely off Travis, it would be nice to move scripts/travis/* to just scripts/.

[Ashmita152]

Could you include some details about what didn't work?

The issue was mainly around GitHub actions install node at path /opt/hostedtoolcache/node/10.23.0/x64/bin/node so when source /home/runner/.nvm/nvm.sh was executing it was failing with VERSION being populated as N/A and hence that script
was failing with

nvm_err 'You need to run "nvm install 10" to install it before using it.'

I felt it maybe I am missing something.

[yurishkuro]

if we already know that we're running in the correct Node version, we do not need nvm use.

[yurishkuro]

What is this error about? https://github.com/jaegertracing/jaeger/runs/1467144626?check_suite_focus=true#step:8:1046

fatal: No names found, cannot describe anything.

[yurishkuro]

I have a few questions about this:

1. If the branch is directly available, and is typically only used by a single step in the workflow (last step in this case), then why bother with an action instead of just export BRANCH=${GITHUB_REF##*/} inside the run: |?
2. why do you use ##*/? Wouldn't this work the same: export BRANCH=${GITHUB_REF}?
3. GITHUB_REF is defined as "The branch or tag ref that triggered the workflow". As I understand it, for a pull request from a fork this would be the branch in the fork, which technically can be master (it's not recommended, but sometimes people create PRs directly from their fork's master branch). I wonder if this will confuse any of the scripts.

[Ashmita152]

If the branch is directly available, and is typically only used by a single step in the workflow (last step in this case), then why bother with an action instead of just export BRANCH=${GITHUB_REF##*/} inside the run: |?

You're right, in that case we don't need to define an action.

why do you use ##*/? Wouldn't this work the same: export BRANCH=${GITHUB_REF}?

Generally GITHUB_REF is set as /ref/head/branch-name

$ export GITHUB_REF=/refs/head/foobar
$ echo ${GITHUB_REF##*/}
foobar

GITHUB_REF is defined as "The branch or tag ref that triggered the workflow". I wonder if this will confuse any of the scripts.

I will look at older travis runs and see what was the value of BRANCH being evaluated there and make sure that we set the BRANCH variable to the same value.

[Ashmita152]

Updated the PR removing action and doing things in workflow itself.

1. Setting BRANCH env as ${{ github.head_ref }} at job level which is set only in pull requests (https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions#github-context)
2. Later setting BRANCH env if BRANCH is not already set (push to master) from GITHUB_REF.

[Ashmita152]

if we already know that we're running in the correct Node version, we do not need nvm use.

I just added this to the GitHub action. Not sure why it was triggering nvm use.

- uses: actions/setup-node@v1
  with:
    node-version: '10'

[Ashmita152]

In the logs the value of BRANCH seems to be merge

Ah sorry I missed it, I should have caught it. I will make sure it's getting populated with the same value as in travis.

[Ashmita152]

What is this error about? https://github.com/jaegertracing/jaeger/runs/1467144626?check_suite_focus=true#step:8:1046

fatal: No names found, cannot describe anything.

Let me look into it more.

[yurishkuro]

I just added this to the GitHub action. Not sure why it was triggering nvm use.

Because in the current script nvm use is unconditional. I am suggesting to replace it with a check for Node version and fail if it does not meet the requirements

[yurishkuro]

btw, I just added a workflow for the UI repo based on setup-node@v2, it's working fine: jaegertracing/jaeger-ui#664

[Ashmita152]

Updated the PR using setup-node@v2 which works now. Also the BRANCH is populating properly now.

Only issue which is remaining is what if the contributor creates PR from their fork's master branch. We can solve this by following either of following:

1. Appending "pr-" infront of the branch name while setting BRANCH env variable in pull request.
2. Updating the current logic of checking whether to push to docker registry with checking whether ${{ github.repository_owner }} variable equal to jaegertracing

[Ashmita152]

Only issue which is remaining is what if the contributor creates PR from their fork's master branch.

This has been resolved by setting an environment variable DOCKERHUB_LOGIN to true iff docker login step succeed (which will not happen in case of PR). And docker push will only happen when DOCKERHUB_LOGIN is true.

[yurishkuro]

almost there

[yurishkuro]

when would this not be null? It's not a std env var in GH actions: https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables

[Ashmita152]

First, we are setting BRANCH env variable in the job level

    env:
      BRANCH: ${{ github.head_ref }}

${{ github.head_ref }} will be only set in case of Pull requests (https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions#github-context)

Hence when we push to master (merge PR), env.BRANCH will be null, in which case we are picking from GITHUB_REF.

[Ashmita152]

Updated the PR to make the steps more self-explanatory.

[Ashmita152]

What is this error about? https://github.com/jaegertracing/jaeger/runs/1467144626?check_suite_focus=true#step:8:1046

fatal: No names found, cannot describe anything.

I think this is what is happening, in Makefile

build-all-in-one looks for BUILD_INFO which inturn looks for GIT_CLOSEST_TAG which isn't getting evaluated properly due to fetch-depth set to 1 while cloning the repo by GitHub action.

$ git clone https://github.com/Ashmita152/jaeger.git --depth 1
Cloning into 'jaeger'...
remote: Enumerating objects: 1243, done.
remote: Counting objects: 100% (1243/1243), done.
remote: Compressing objects: 100% (1050/1050), done.
remote: Total 1243 (delta 323), reused 485 (delta 129), pack-reused 0
Receiving objects: 100% (1243/1243), 1.10 MiB | 1.10 MiB/s, done.
Resolving deltas: 100% (323/323), done.
$ cd jaeger
$ git describe --abbrev=0 --tags
fatal: No names found, cannot describe anything.

[yurishkuro]

build-all-in-one looks for BUILD_INFO which in turn looks for GIT_CLOSEST_TAG

we'll need to fix this because BUILD_INFO is important to include in the binaries.

[Ashmita152]

build-all-in-one looks for BUILD_INFO which in turn looks for GIT_CLOSEST_TAG

we'll need to fix this because BUILD_INFO is important to include in the binaries.

This has been resolved.

[yurishkuro]

it would be better for debugging to print the derived value

Suggested change

	echo "BRANCH=${GITHUB_HEAD_REF}" >> $GITHUB_ENV
	export BRANCH=${GITHUB_HEAD_REF}
	echo "we are on branch=$BRANCH"
	echo "BRANCH=${BRANCH}" >> $GITHUB_ENV

[yurishkuro]

Look great! Minor potential future improvements:

• echo value of BRANCH
• move docker login steps and BRANCH steps into their own local actions (but only if we need to reuse them elsewhere)

[Ashmita152]

Thank you @yurishkuro for the constant help.

After the PR I was expecting one dockerhub image upload after the PR was merged but something looks wrong somewhere in the script.

I can see env variables are populated correctly

2020-11-29T20:00:51.1750375Z env:
2020-11-29T20:00:51.1750881Z   GOROOT: /opt/hostedtoolcache/go/1.15.5/x64
2020-11-29T20:00:51.1751426Z   DOCKERHUB_LOGIN: true
2020-11-29T20:00:51.1751881Z   BRANCH: master

But somehow it still skipped the upload to dockerhub

2020-11-29T20:08:03.2662939Z + upload_to_docker jaegertracing/all-in-one
2020-11-29T20:08:03.2663826Z + [[ master == \m\a\s\t\e\r ]]
2020-11-29T20:08:03.2664409Z + [[ DOCKERHUB_LOGIN == \t\r\u\e ]]
2020-11-29T20:08:03.2665960Z + echo 'skip docker upload for PR'

I will look into it more.