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

Improve depends_on docker-compose #4249

Merged
merged 2 commits into from Dec 18, 2023

Conversation

killua99
Copy link
Contributor

Checking the service is up and healthy before start the service to avoid issue first boot.

…t the service that might cause issue first boot
@unixfox
Copy link
Member

unixfox commented Nov 10, 2023

@killua99
Copy link
Contributor Author

Hi, interesting perhaps is something with the compose version.

I'm going to double check.

https://docs.docker.com/compose/compose-file/05-services/#depends_on

@ChunkyProgrammer
Copy link
Contributor

Thank you for the CI is failing: https://github.com/iv-org/invidious/actions/runs/6823141172/job/18558101386?pr=4249

@unixfox I believe this PR fixes the CI issue #4242

@killua99
Copy link
Contributor Author

Oh that's look promising.

But the option restart isn't available on Ubuntu LTS image from the CI, it require a modern docker compose component.

Copy link
Member

@SamantazFox SamantazFox left a comment

Choose a reason for hiding this comment

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

looks good to me! I've that deployed on my private instance, it works like a charm!

@unixfox
Copy link
Member

unixfox commented Nov 23, 2023

I'm still not sure if we should use the new format because then the docker-compose file will not work with the older versions or compose.

@killua99
Copy link
Contributor Author

I didn't alter the version requiere for docker compose to work, so version 3 and above (the current version you support) should be fine and able to run it without issue.

Actually, I should update documentation right ?

@unixfox
Copy link
Member

unixfox commented Nov 24, 2023

But

depends_on:
      invidious-db:
        condition: service_healthy

is invalid in older docker-compose versions. that's what I'm talking about

@killua99
Copy link
Contributor Author

you have set version: "3" that's means should be supported, or am I missing something? gonna double check anyway

@unixfox
Copy link
Member

unixfox commented Nov 27, 2023

I'm not entirely sure, but I think this is only supported by the new Compose V2, the old Compose V1 do not support this depends_on syntax.

@killua99
Copy link
Contributor Author

killua99 commented Nov 27, 2023

then, perhaps you will like to add some tests for compose V1 ? (which BTW is no longer supported)

I can add tests for compose V1

@unixfox
Copy link
Member

unixfox commented Nov 27, 2023

It is not about tests in the CI but about if we should break existing setups that use Compose V1 or not. Just for the sake of a "better start" of Invidious.

Do note though that the docker-compose.yml file is not being used for production, it's only for development. The docker-compose file that is the most used is located at https://github.com/iv-org/documentation/blob/master/docs/installation.md#docker-compose-method-production

@killua99
Copy link
Contributor Author

It is not about tests in the CI but about if we should break existing setups that use Compose V1 or not. Just for the sake of a "better start" of Invidious.

Do note though that the docker-compose.yml file is not being used for production, it's only for development. The docker-compose file that is the most used is located at https://github.com/iv-org/documentation/blob/master/docs/installation.md#docker-compose-method-production

Indeed in a previous comment I mentioned if could I upgrade the documentation (I didn't mention production)

But again docker compose V1 is outdated and without security patches is risky keep /maintained outdated OSS

@unixfox
Copy link
Member

unixfox commented Nov 27, 2023

Indeed in a previous comment I mentioned if could I upgrade the documentation (I didn't mention production)

Yes everyone can!

But again docker compose V1 is outdated and without security patches is risky keep /maintained outdated OSS

We tend to support old things, in a sense invidious allows to use YouTube in old devices. So if the functionality is not a dealbreaker we usually try to not add it if it means dropping support for old devices, old software and so on.

@SamantazFox
Copy link
Member

I agree that we should support old browsers, but on the server side, we should try to incentivize the use of updated software. In addition to that, I've personally never seen a distro shipping compose v1/v2, so I don't see what edge case we are covering here.

@unixfox
Copy link
Member

unixfox commented Dec 18, 2023

I agree that we should support old browsers, but on the server side, we should try to incentivize the use of updated software. In addition to that, I've personally never seen a distro shipping compose v1/v2, so I don't see what edge case we are covering here.

OK that is a good idea. @killua99 could you submit the same change to the documentation please?

@unixfox unixfox merged commit 97c4165 into iv-org:master Dec 18, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants