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

fix: use localhost for services in CI #816

Merged
merged 4 commits into from
Jan 4, 2022

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Jan 3, 2022

Which problem is this PR solving?

The services in CI are not currently accessible.
The hostnames do not resolve because the step execution was recently moved from containers(which belong to the bridge network where the service names resolve) to the runner machine.
See c69c202 - @dyladan

The docs:

If you configure the job to run directly on the runner machine and your step doesn't use a container action, you must map any required Docker service container ports to the Docker host (the runner machine). You can access the service container using localhost and the mapped port.

The above section from the docs makes total sense, however, I'm personally not clear on why is actions/setup-node@v2 the recommended way to run node - it pollutes the runner machine and comes with an additional layer of caching.

Short description of the changes

Since just swapping back to running in a container, results in some FS permission issues, I'm just changing the hostnames to use to localhost.

That means the TAV setup is broken as well. I'd make the same change to it, but seeing that it doesn't set up node, I cannot see how it could work even with the services being accessible. I'll disable it for now.

Sorted the keywords in the packages mainly to trigger a change for Lerna for all the packages.

@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #816 (081e069) into main (359fee8) will decrease coverage by 0.20%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #816      +/-   ##
==========================================
- Coverage   94.91%   94.71%   -0.21%     
==========================================
  Files          14      154     +140     
  Lines         708    12175   +11467     
  Branches      142     1176    +1034     
==========================================
+ Hits          672    11531   +10859     
- Misses         36      644     +608     
Impacted Files Coverage Δ
...etry-instrumentation-bunyan/src/instrumentation.ts 97.82% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 96.77% <0.00%> (ø)
...e/opentelemetry-instrumentation-redis/src/utils.ts 92.98% <0.00%> (ø)
...pentelemetry-instrumentation-mongodb/test/utils.ts 91.42% <0.00%> (ø)
...emetry-instrumentation-aws-sdk/src/services/sns.ts 93.33% <0.00%> (ø)
...on-dns/test/integrations/dnspromise-lookup.test.ts 91.15% <0.00%> (ø)
...telemetry-instrumentation-redis/test/redis.test.ts 95.65% <0.00%> (ø)
...de/opentelemetry-instrumentation-net/test/utils.ts 97.67% <0.00%> (ø)
...e/opentelemetry-instrumentation-pg/test/pg.test.ts 94.13% <0.00%> (ø)
...entation-aws-sdk/src/services/MessageAttributes.ts 87.09% <0.00%> (ø)
... and 130 more

@rauno56 rauno56 marked this pull request as ready for review January 3, 2022 17:26
@rauno56 rauno56 requested a review from a team January 3, 2022 17:26
@dyladan
Copy link
Member

dyladan commented Jan 3, 2022

I switched from containers to node when I updated actions/checkout from v1 to v2. The v2 checkout was running into filesystem permission issues you mentioned in this PR description. At first I tried to fix ownership/permissions of the directories, but switching to setup-node was just easier. At that time the tests passed so I didn't realize it would cause a networking issue.

We can go back to running in node containers, but we will need to solve the permission issue.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Approving this because it solves our immediate problem of tests failing but we should look into container tests again

@vmarchaud vmarchaud merged commit f497313 into open-telemetry:main Jan 4, 2022
@rauno56 rauno56 deleted the fix/runner-machine-services branch January 4, 2022 09:06
This was referenced Feb 25, 2022
@chingor13 chingor13 mentioned this pull request Feb 28, 2022
@dyladan dyladan mentioned this pull request Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants