Page MenuHomePhabricator

[services] Remove `networks` from services `docker-compose` file.
ClosedPublic

Authored by max on May 30 2022, 6:00 AM.
Tags
None
Referenced Files
F3178020: D4149.diff
Fri, Nov 8, 12:42 AM
Unknown Object (File)
Wed, Nov 6, 11:41 AM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM
Unknown Object (File)
Sat, Nov 2, 3:41 PM

Details

Summary

This diff removes networks from the services docker-compose file to use the default shared network and omit complex network setup.

Linear task: ENG-774

Test Plan

Run yarn run-tunnelbroker-service-dev-mode and connect to the localstack and rabbitmq service by the hostnames localstack and rabbitmq respectively from inside of the tunnelbroker container.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max held this revision as a draft.
max edited the test plan for this revision. (Show Details)
max added reviewers: karol, tomek.
max published this revision for review.May 30 2022, 6:20 AM
ashoat requested changes to this revision.May 30 2022, 12:19 PM

What's the point of defining a network and then just connecting every single container to it? Isn't that the default behavior of Docker Compose?

I think we can simply remove all of the networks: sections (in the root and for each service) and everything should just work the same. Do we have any plans for a more complex network topology that justifies the extra boilerplate?

This revision now requires changes to proceed.May 30 2022, 12:19 PM

What's the point of defining a network and then just connecting every single container to it? Isn't that the default behavior of Docker Compose?

I think we can simply remove all of the networks: sections (in the root and for each service) and everything should just work the same.

Yes, I think we can remove the networks: sections. It was added to follow the existing configuration only.

I can change this diff to remove all the networks: sections.

Do we have any plans for a more complex network topology that justifies the extra boilerplate?

From the tunnelbroker perspective, we don't have such plans for now. Will any other services need it? cc @karol-bisztyga , @varun.

max added a reviewer: varun.

Resigning to remove this from my queue, while leaving it on others' queue. My perspective is that networks: should be removed for now since it is doing nothing, and we can reintroduce it later if/when we decide we need it

Do we have any plans for a more complex network topology that justifies the extra boilerplate?

From the tunnelbroker perspective, we don't have such plans for now. Will any other services need it? cc @karol-bisztyga , @varun.

I think the Identity service can just use the default network

tomek requested changes to this revision.Jun 2 2022, 12:56 AM

Requesting changes because it looks like we don't need to specify networks. We're using it in a couple of place, so it might be a good idea to delete all of the usages.

This revision now requires changes to proceed.Jun 2 2022, 12:56 AM
max retitled this revision from [services] Tunnelbroker - Add `networks` to the services `docker-compose` file. to [services] Remove `networks` from services `docker-compose` file..Jun 2 2022, 5:04 AM
max edited the summary of this revision. (Show Details)
In D4149#118158, @palys-swm wrote:

Requesting changes because it looks like we don't need to specify networks. We're using it in a couple of place, so it might be a good idea to delete all of the usages.

I've changed the diff to delete the networks section. Requesting re-reviewing it.

This revision is now accepted and ready to land.Jun 3 2022, 1:38 AM