ci: reuse docker job from actions #752
Conversation
Codecov Report
@@ Coverage Diff @@
## main #752 +/- ##
=======================================
Coverage 75.31% 75.31%
=======================================
Files 40 40
Lines 3622 3622
=======================================
Hits 2728 2728
Misses 812 812
Partials 82 82 |
Signed-off-by: Artsiom Koltun <artsiom.koltun@intel.com>
| run: docker-compose up --build --force-recreate --detach | ||
|
|
||
| - name: Run Tests | ||
| run: ./scripts/tests.sh |
There was a problem hiding this comment.
Good catch!
WDYT about opiproject/actions#22
There was a problem hiding this comment.
I believe this is just duplication, since we have both success of the opi-test container and then now test.sh...
maybe we can leave just shell script and do the wait inside... WDYT ? just brainstorming...
There was a problem hiding this comment.
What's the main difference between test.sh and godpu? In tests.sh we run some checks for nvme tcp controller by means of spdk_nvme_perf/spdk_nvme_identify.
Are they really important?
Can we add similar checks to other parts like Nvme PCIe, virtio-blk, backend api?
| @@ -69,6 +69,8 @@ services: | |||
| condition: service_healthy | |||
| redis: | |||
| condition: service_healthy | |||
| jaeger: | |||
There was a problem hiding this comment.
this is not must, those services can be independent... you can leave it or remove it...
There was a problem hiding this comment.
I'd prefer to keep this as a dependency. If a jaeger service is not running, it takes too long to get a cmd executed (jaeger requests are timeouted), so if we execute docker-compose up opi-spdk-server jaeger will be brought up as well
| @@ -77,6 +79,8 @@ services: | |||
| image: redis:7.2.2-alpine3.18 | |||
| networks: | |||
| - opi | |||
| ports: | |||
There was a problem hiding this comment.
If I want to run a server without a container, I need to provide an environment as well. I can type docker-compose up redis and get redis listening on the port. The same with jaeger. But if you want, I can remove this part.
glimchb
added
the
Merge Candidate
Additionally, we need to change required actions in the repo list