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
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
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
Branch
add-networks-to-tunnelbroker
Lint
No Lint Coverage
Unit
No Test Coverage

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