-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 e2e tests to remaining services #242
Conversation
hey @mic-max do we really need to expose the localhost ports? For testing instead of opening the localhost, we could spin up a container within the same docker network and execute the test. Another solution would be to keep the internal ports the apps are already using, and expose a different one within the localhost.
In that case we could have multiple container running on port 5050 (for instance), and just change the localhost exposed port. I'd rather go for the 1st solution. |
@julianocosta89 Is there any downside to opening to ports to the localhost? I think it is the simplest way to do it. The test container option sounds doable, does anyone know if that's a common architecture for testing docker containers? |
No actual downside in a demo perspective, just in a real life app. Another point is that we need to ensure all services all communicating with each other with the new ports. But that is fine, considering that it will be done once. |
@fatsheep9146, @julianocosta89 any further comments / suggestions ? |
LGTM |
@mic-max please resolve the conflicts then we'll merge |
Resolved 😊 |
This is a follow-up to #248, which in turn was resolving: #242 (comment) The ruby app is relying on convention, and by convention a `PORT` env varable will determine which port the `puma` webserver actually listens on. So - we don't need to set it explicitly in the compose file, but that means we need to set it elsewhere somehow. I tried a few hacks with the dockerfile: - Trying to pass `PORT=$EMAIL_SERVICE_PORT` - Trying to pass `-p $EMAIL_SERVICE_PORT` in the command But those didn't work and I gave up. So instead we just set it directly in the sinatra app ourselves. (Please don't ask me how that actually gets set down all the way through sinatra -> rack -> puma, because truthfully I do not know: rack-based servers are basically magic). We can see that it works: ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ git grep EMAIL_SERVICE_P ORT .env:EMAIL_SERVICE_PORT=6060 ``` ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ docker-compose up --build emailservice email-service | == Sinatra (v2.2.0) has taken the stage on 6060 for production with backup from Puma email-service | I, [2022-08-03T13:08:27.643291 #1] INFO -- : Instrumentation: OpenTelemetry::Instrumentation::Sinatra was successfully installed with the following options {} email-service | Puma starting in single mode... email-service | * Puma version: 5.6.4 (ruby 3.1.2-p20) ("Birdie's Version") email-service | * Min threads: 0 email-service | * Max threads: 5 email-service | * Environment: production email-service | * PID: 1 email-service | * Listening on http://0.0.0.0:6060 email-service | Use Ctrl-C to stop ``` Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
This is a follow-up to open-telemetry#248, which in turn was resolving: open-telemetry#242 (comment) The ruby app is relying on convention, and by convention a `PORT` env varable will determine which port the `puma` webserver actually listens on. So - we don't need to set it explicitly in the compose file, but that means we need to set it elsewhere somehow. I tried a few hacks with the dockerfile: - Trying to pass `PORT=$EMAIL_SERVICE_PORT` - Trying to pass `-p $EMAIL_SERVICE_PORT` in the command But those didn't work and I gave up. So instead we just set it directly in the sinatra app ourselves. (Please don't ask me how that actually gets set down all the way through sinatra -> rack -> puma, because truthfully I do not know: rack-based servers are basically magic). We can see that it works: ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ git grep EMAIL_SERVICE_P ORT .env:EMAIL_SERVICE_PORT=6060 ``` ``` @ahayworth ➜ /workspaces/opentelemetry-demo-webstore (ahayworth/emailservice-port-things ✗) $ docker-compose up --build emailservice email-service | == Sinatra (v2.2.0) has taken the stage on 6060 for production with backup from Puma email-service | I, [2022-08-03T13:08:27.643291 open-telemetry#1] INFO -- : Instrumentation: OpenTelemetry::Instrumentation::Sinatra was successfully installed with the following options {} email-service | Puma starting in single mode... email-service | * Puma version: 5.6.4 (ruby 3.1.2-p20) ("Birdie's Version") email-service | * Min threads: 0 email-service | * Max threads: 5 email-service | * Environment: production email-service | * PID: 1 email-service | * Listening on http://0.0.0.0:6060 email-service | Use Ctrl-C to stop ``` Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
Fixes #195
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes