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

Make ports exposed in docker compose configurable #113

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Conversation

relud
Copy link
Member

@relud relud commented Dec 18, 2023

docker-compose is exposing ports on the host that I believe are only needed internally within the docker-compose network.

This caused me issues with trying to use a vscode devcontainer, as seen in mozilla-services/tecken#2857, because both projects expose these ports, and the environments are not automatically shut down when vscode is closed: https://github.com/mozilla-services/tecken/pull/2857/files#diff-24ad71c8613ddcf6fd23818cb3bb477a1fb6d83af4550b0bad43099813088686R10

It would make sense to also do this in tecken, but that exposes more ports that I am less sure are unnecessary

see also mozilla-services/tecken#2858

per https://github.com/mozilla-services/tecken/pull/2857/files#r1430382049

 "shutdownAction": "none",

[biancadanforth](https://github.com/biancadanforth) [Dec 18, 2023](https://github.com/mozilla-services/tecken/pull/2857/files#r1430382049):
This is what prevents the devcontainer from shutting down when we close out of VS Code IIUC.

with devcontainer and its docker-compose dependencies running even when vscode isn't open, ports conflict between tecken, eliot, and socorro, so this PR makes the exposed ports configurable via environment variables, which can be set in .env in each project to avoid overlap.

@relud relud requested a review from a team December 18, 2023 20:09
@willkg
Copy link
Contributor

willkg commented Dec 18, 2023

We use a lot of the same services between Antenna, Eliot, Socorro, and Tecken. I can't remember if I did a good job of making them consistent. If they're not consistent, we should fix that--but we could do that in the future.

and make exposed ports configurable
@relud relud force-pushed the expose-less-docker branch from 1e7f3fb to 0d7a59f Compare December 18, 2023 21:08
@relud relud requested a review from willkg December 18, 2023 21:09
@relud relud changed the title Don't expose unnecessary ports in docker-compose Make ports exposed in docker compose configurable Dec 18, 2023
docker-compose.yml Outdated Show resolved Hide resolved
@@ -17,7 +17,7 @@ services:
extends:
service: base
ports:
- "8000:8000"
- "${EXPOSE_ELIOT_PORT:-8000}:8000"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you set this in .env, then make run doesn't really work. Is it possible to set this in .env for the local dev environment, but not the devcontainer?

Copy link
Member Author

Choose a reason for hiding this comment

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

then make run doesn't really work

how so? it still runs eliot, just exposed on a different port

Co-authored-by: Will Kahn-Greene <willkg@users.noreply.github.com>
Copy link
Contributor

@willkg willkg left a comment

Choose a reason for hiding this comment

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

Once we add the .devcontainer stuff, we should adjust the stop rule in the Makefile to something like:

.PHONY: stop
stop: .env  ## | Stop docker containers.
	${DC} -f docker-compose.yml -f .devcontainer/docker-compose.yml stop

We might also want to write up a page in Confluence covering using VSCode with all the crash ingestion services.

This looks good!

@smarnach
Copy link
Contributor

We could put the configuration for the dev container in the main docker-compose.yml file. I chose to put it in a different file to keep things separate, but I don't know how much value there is to keeping things separate. If we merge the configuration files, managing the dev container from the command line will become easier.

@relud relud merged commit 11aee08 into main Dec 19, 2023
1 check passed
@relud relud deleted the expose-less-docker branch December 19, 2023 17:09
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.

3 participants