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

chore(test-utils): improve handling of running docker containers #1695

Merged

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Sep 21, 2023

  • Convert the output of run commands to a string that is readable.
  • No need to docker rm ... test containers for npm run test:local usage because the docker run --rm ... option will remove them when stopped.

Which problem is this PR solving?

When running npm run test:local in some of the packages (e.g. in "plugins/node/opentelemetry-instrumentation-ioredis") there were a couple problems with the running of docker commands:

  1. If one of the commands failed, the printed output would be a raw array of Buffer content, e.g.:

Before:

...
Failed run command: docker rm otel-redis
[
  null,
  <Buffer >,
  <Buffer 45 72 72 6f 72 20 72 65 73 70 6f 6e 73 65 20 66 72 6f 6d 20 64 61 65 6d 6f 6e 3a 20 4e 6f 20 73 75 63 68 20 63 6f 6e 74 61 69 6e 65 72 3a 20 6f 74 65 ... 8 more bytes>
]

After:

...
Failed run command: docker rm otel-redis
Error response from daemon: No such container: otel-redis
  1. The cleanup steps were running docker stop ... and docker rm .... The latter is unnecessary, given that all the docker run ... commands use the --rm argument. The result was, for example:
Failed run command: docker rm otel-redis
Error response from daemon: No such container: otel-redis

Removing the docker rm ... in the cleanup resolves this issue.

- Convert the output of run commands to a string that is readable.
- No need to `docker rm ...` test containers for `npm run test:local` usage
  because the `docker run --rm ...` option will remove them when stopped.
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #1695 (08cb2c5) into main (5675c49) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1695   +/-   ##
=======================================
  Coverage   91.36%   91.36%           
=======================================
  Files         129      129           
  Lines        6402     6402           
  Branches     1281     1281           
=======================================
  Hits         5849     5849           
  Misses        553      553           

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

🥇

@blumamir blumamir merged commit f64fddf into open-telemetry:main Sep 24, 2023
14 checks passed
@trentm trentm deleted the trentm/test-utils-docker-improvment branch September 25, 2023 15:55
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.

5 participants