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.

Sign up for GitHub

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

Jump to bottom

Using docker compose instead of docker-compose #1791

Closed
wants to merge 1 commit into from
Closed

Using docker compose instead of docker-compose #1791

wants to merge 1 commit into from

Conversation

highghlow
Copy link
Contributor

@highghlow highghlow commented Apr 11, 2024 •
edited

docker-compose is the V1 syntax, which is now deprecated. The V2 syntax is docker compose, since it's now a plugin and not a separate command.

This pull request should fix #1785

@highghlow
Copy link
Contributor Author

So, it turns out docker compose now names containers <app>-<container>-<no> and not <app>_<container>_<no>, which will break all apps using app_proxy. Don't merge this yet, I'll figure out a solution

@highghlow highghlow marked this pull request as draft April 11, 2024 18:46
@highghlow
Copy link
Contributor Author

Okay, I don't know what to do. docker dash compose is clearly broken, but space compose has a different naming schema. Hmmm....

@highghlow
Copy link
Contributor Author

highghlow commented Apr 11, 2024 •
edited

It turns out docker compose has a compatibility flag, but that is not a good long term solution. I don't want to implement this right now since temporary is the most permanent.

The issue can be dealt with by running docker compose down after every test.

In docker compose guides I've seen inter-container networking being done by just using the container name (without app_id and _1). @lukechilds Why didn't you use this here?

Toromambo
Toromambo approved these changes Apr 28, 2024
View reviewed changes
scripts/start
@@ -147,7 +147,7 @@ fi
echo
echo "Starting Docker services..."
echo
docker-compose "${compose_files[@]}" up --detach --build --remove-orphans || {
Copy link

Choose a reason for hiding this comment

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

build new version

@highghlow
Copy link
Contributor Author

Ok, so Umbrel v1.0 already uses the new compose. I'm just on 0.5

@highghlow highghlow closed this Apr 29, 2024
@lukechilds
Copy link
Member

In docker compose guides I've seen inter-container networking being done by just using the container name (without app_id and _1). @lukechilds Why didn't you use this here?

We don't rely on that because it can have collisions between apps, e.g if two apps have a db container and you reference it via my-service --connnect db:1234 it can randomly connect to either db container from the different apps on each request.

Using the full container name guarantees uniqueness and avoids collisions.

So, it turns out docker compose now names containers -- and not , which will break all apps using app_proxy. Don't merge this yet, I'll figure out a solution

Yeah that was a pain, we handled this in 1.0 by manually patching all compose files to force the container name to the old pattern:

umbrel/packages/umbreld/source/modules/apps/app.ts

Lines 93 to 109 in ace3d9d

async patchComposeServices() {
// Temporary patch to fix contianer names for modern docker-compose installs.
// The contianer name scheme used to be <project-name>_<service-name>_1 but
// recent versions of docker-compose use <project-name>-<service-name>-1
// swapping underscores for dashes. This breaks Umbrel in places where the
// containers are referenced via name and it also breaks referring to other
// containers via DNS since the hostnames are derived with the same method.
// We manually force all container names to the old scheme to maintain compatibility.
const compose = await this.readCompose()
for (const serviceName of Object.keys(compose.services!)) {
if (!compose.services![serviceName].container_name) {
compose.services![serviceName].container_name = `${this.id}_${serviceName}_1`
}
}
await this.writeCompose(compose)
}

@highghlow
Copy link
Contributor Author

So, it turns out docker compose now names containers -- and not , which will break all apps using app_proxy. Don't merge this yet, I'll figure out a solution

Yeah that was a pain, we handled this in 1.0 by manually patching all compose files to force the container name to the old pattern

Got it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Toromambo Toromambo Toromambo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failing to install on ubuntu 20.04
3 participants
@highghlow @lukechilds @Toromambo