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.

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

Add github action for jaeger integration tests #2649

Merged
merged 3 commits into from
Nov 27, 2020
Merged

Add github action for jaeger integration tests #2649

merged 3 commits into from
Nov 27, 2020

Conversation

Ashmita152
Copy link
Contributor

@Ashmita152 Ashmita152 commented Nov 26, 2020

Signed-off-by: Ashmita Bohara ashmita.bohara152@gmail.com

Which problem is this PR solving?

This PR adds GitHub actions for running integration tests for Elasticsearch, Cassandra, Memory, Kafka & Badger.

Part of #2645.

Short description of the changes

Also it removes --interactive and --tty flags from test-compile-es-scripts makefile directive, which shouldn't be used there and was causing elasticsearch integration test to fail on GH actions with this error (not sure how it is working on travis, maybe different docker version)

docker run --rm -it -v /home/runner/work/jaeger/jaeger:/tmp/jaeger python:3-alpine /usr/local/bin/python -m py_compile /tmp/jaeger/plugin/storage/es/esRollover.py
the input device is not a TTY

@Ashmita152 Ashmita152 requested a review from a team as a code owner November 26, 2020 02:32
@mergify mergify bot requested a review from jpkrohling November 26, 2020 02:33
@@ -0,0 +1,93 @@
name: Integration Tests
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to define all our CI jobs in a single file. That way we'll have better control over their orchestration, like using .needs to establish dependencies, define common parameters via env, like the Go version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @yurishkuro that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

The down side is that we can only trigger new runs on a per-workflow basis, meaning that all jobs within the workflow will be restarted.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, if we cannot rerun individual jobs then it's better to have them as separate workflows.

It's annoying that we'd have to repeat a lot of boilerplate. At some point we might want to consider auto-generating the YAML files from smaller pieces. I originally was looking at this approach of using YAML aliases, but that only works in a single file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can indeed have multiple workflows in the same file, but it would be a huge YAML no matter how much we Don't-Repeat-Ourselves. Let's see what's the final result, and we can then iterate and refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. I am little confused if I should merge unit test and integration tests into one yaml. Using matrix, definitely made the config much cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I think our badger integration tests broke due to some recent commit.

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #2649 (b021c83) into master (24fb6bc) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2649      +/-   ##
==========================================
+ 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 Δ
cmd/query/app/static_handler.go 83.03% <0.00%> (-1.79%) ⬇️
plugin/storage/integration/integration.go 77.90% <0.00%> (+0.55%) ⬆️
cmd/query/app/server.go 90.16% <0.00%> (+1.63%) ⬆️

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 24fb6bc...b021c83. Read the comment docs.

@yurishkuro
Copy link
Member

I am little confused if I should merge unit test and integration tests into one yaml. Using matrix, definitely made the config much cleaner.

@Ashmita152 we need to be able to restart individual jobs, not the complete CI run. If we can do it with a single workflow file then I would consolidate all CI into one place (just like it was with .travis.yml), otherwise it's better to have independent workflow files.

@Ashmita152
Copy link
Contributor Author

@yurishkuro Looking at the GitHub community threads, it seems like GH actions don't have support restarting individual job. I would suggest maybe we can go ahead with different workflows for now and merge them into one once GitHub actions support it.

@yurishkuro
Copy link
Member

Let's go with different files. My only other alternative suggestion is to write a shell script that would generate the Yaml files, and execute it as a commit hook. Since the files are only going to be different in one command, the script could be fairly straightforward.

@Ashmita152
Copy link
Contributor Author

Hi @yurishkuro

It makes sense to have a script generate the workflow files. Let me look at that option.

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
@Ashmita152
Copy link
Contributor Author

Hi @yurishkuro

I thought maybe I can do the script version in another PR since I am not quite familiar with git hooks. To have the CI working quickly, we moved each integration test into separate files. Let me know what you think. Thanks.

@yurishkuro
Copy link
Member

yes, let's start with explicit files, auto-generation can be done later

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
@Ashmita152
Copy link
Contributor Author

Thank you @yurishkuro updated the PR with the feedback changes.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro merged commit 34e2f04 into jaegertracing:master Nov 27, 2020
yurishkuro added a commit that referenced this pull request Jan 13, 2021
…ectors (#2657)

* add metrics that show agent connection collector status

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* update comment

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* exec make fmt

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify function and add testing relevant code in the builder_test.go

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* add comment in connect_metrics.go

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* exec make fmt

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix collector panic due to sarama sdk returning nil error (#2654)

Signed-off-by: luhualin <luhualin@bilibili.com>

Co-authored-by: luhualin <luhualin@bilibili.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix flaky tbuffered server test (#2635)

* Fix flaky tbuffered server test

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

* Apply suggestions from code review - more readable comments

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Add github actions for integration tests (#2649)

* Add github action for jaeger integration tests

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Create separate workflow for each integration test

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Clean-up GH action names (#2661)

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix for failures in badger integration tests (#2660)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Add protogen validation test (#2662)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Add github action for jaeger all-in-one image (#2663)

* Add github action for jaeger all-in-one image

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Make steps self-explantory

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Fix git tags issue

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Fix ES integration test

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Update comment that looks confusing during builds

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Use GitHub actions based build badges

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix and minor improvements to all-in-one github action (#2667)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix docker login issue with all-in-one build (#2668)

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix issue with all-in-one build (#2669)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* merage from the lastest master branch and exec make fmt

Signed-off-by: walker.wangxy <walker.wangxy@walkerwangxydeMacBook-Pro.local>

* add comment on ConnectMetrics

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* clear up redundant codes

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

Co-authored-by: WalkerWang731 <walkerwang731@gmail.com>
Co-authored-by: Betula-L <N.Betula.Lu@gmail.com>
Co-authored-by: luhualin <luhualin@bilibili.com>
Co-authored-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Ashmita <ashmita.bohara152@gmail.com>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: walker.wangxy <walker.wangxy@walkerwangxydeMacBook-Pro.local>
bhiravabhatla pushed a commit to bhiravabhatla/jaeger that referenced this pull request Jan 25, 2021
…ectors (jaegertracing#2657)

* add metrics that show agent connection collector status

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* update comment

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* exec make fmt

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify function and add testing relevant code in the builder_test.go

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* add comment in connect_metrics.go

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify code and changed use expvar to show target

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* exec make fmt

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix collector panic due to sarama sdk returning nil error (jaegertracing#2654)

Signed-off-by: luhualin <luhualin@bilibili.com>

Co-authored-by: luhualin <luhualin@bilibili.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix flaky tbuffered server test (jaegertracing#2635)

* Fix flaky tbuffered server test

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

* Apply suggestions from code review - more readable comments

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Add github actions for integration tests (jaegertracing#2649)

* Add github action for jaeger integration tests

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Create separate workflow for each integration test

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Clean-up GH action names (jaegertracing#2661)

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix for failures in badger integration tests (jaegertracing#2660)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Add protogen validation test (jaegertracing#2662)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Add github action for jaeger all-in-one image (jaegertracing#2663)

* Add github action for jaeger all-in-one image

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Feedbacks changes

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Make steps self-explantory

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Fix git tags issue

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Fix ES integration test

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Update comment that looks confusing during builds

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Use GitHub actions based build badges

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix and minor improvements to all-in-one github action (jaegertracing#2667)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix docker login issue with all-in-one build (jaegertracing#2668)

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>

* Fix docker login issue with all-in-one build

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Fix issue with all-in-one build (jaegertracing#2669)

Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* Update cmd/agent/app/reporter/connect_metrics.go

accept suggestions

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* simplify the code that remove ConnectMetricsParams{} and integrate ConnectMetrics{}

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* merage from the lastest master branch and exec make fmt

Signed-off-by: walker.wangxy <walker.wangxy@walkerwangxydeMacBook-Pro.local>

* add comment on ConnectMetrics

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

* clear up redundant codes

Signed-off-by: WalkerWang731 <wxy1990731@hotmail.com>

Co-authored-by: WalkerWang731 <walkerwang731@gmail.com>
Co-authored-by: Betula-L <N.Betula.Lu@gmail.com>
Co-authored-by: luhualin <luhualin@bilibili.com>
Co-authored-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Co-authored-by: Ashmita <ashmita.bohara152@gmail.com>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: walker.wangxy <walker.wangxy@walkerwangxydeMacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants