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

Chore/docker-documentation-improvements #2494

Merged

Conversation

kimsible
Copy link
Contributor

@kimsible kimsible commented Feb 14, 2020

This an update /support/docker/production/.env file example since the recent changes of environment variables (#2247) in the release v2.1.

Also, more details and some improvements for the docker documentation /support/doc/docker.md to simplify the installation, some parts were unclear for non-familiar with docker-compose and traefik : https://github.com/kimsible/PeerTube/blob/chore/improve-docker-documentation/support/doc/docker.md

Plus, it moves ACME email and domain list from /support/docker/production/config/traefik.toml to /support/docker/production/.env and /support/docker/production/docker-compose.yml as a command of traefik service.

@kimsible kimsible mentioned this pull request Feb 14, 2020
@rigelk rigelk added Component: Docker 🐳 Deals with containerisation, a hellish nightmare for Chocobos Component: Documentation 📚 labels Feb 14, 2020
@kimsible
Copy link
Contributor Author

kimsible commented Feb 14, 2020

Thanks for adding the labels @rigelk.

I also wonder if ./docker-volume/traefik/traefik.toml could be placed in ./docker-volume/config/traefik.toml as it is provided by the peertube docker image chocobozzz/peertube:production-buster and as the [acme] email and [[acme.domains]] configurations lines have been dropped and now defined as a traefik command in docker-compose.yml

services:
  reverse-proxy:
    volumes:
      [...]
      - ./docker-volume/traefik/acme.json:/etc/acme.json
      - ./docker-volume/traefik/traefik.toml:/traefik.toml

Also, for ./docker-volume/traefik/acme.json, is it really usefull to link it as a volume in docker-compose.yml ? I mean, docker will not erase it inside the traefik container (/etc/acme.json) until it is recreated.

If I don't miss anything it could simplify more the configuration.

@kimsible kimsible force-pushed the chore/improve-docker-documentation branch from 9e0a94d to f4111ab Compare February 14, 2020 22:48
@Chocobozzz
Copy link
Owner

Thanks for the PR. Could you fix the conflict please?

@kimsible kimsible force-pushed the chore/improve-docker-documentation branch from f4111ab to d5c227c Compare February 17, 2020 12:44
@kimsible
Copy link
Contributor Author

Ok thanks @Chocobozzz for replying to my first comment in the issue #2304.

Here .env has minor changes to make documentation clearer, as it is the main file to configure.
I see it as an example file.

For the extrating of [acme] email and [[acme.domains]] of ./docker-volume/traefik/traefik.toml to .env / docker-compose.yml, it is more to follow the recent changes of the v2.1, to unify main env var in the same file .env.

In my opinion, some environement variables should have been left in docker-compose.yml (POSTGRES and POSTFIX) but I suppose they've been extracted to make docker-compose.yml simpler to maintain.

@LecygneNoir
Copy link
Contributor

Hello,

I have just noticed (already present in the old doc it seems) that the files used for "preview" are different from the files downloaded.

Indeed, to download with curl, you use /master/, but for the link to the previews, you use /develop/

I guess we should use /master/ for all?

Could you fix it in the PR before merging? (Else I may create a new PR once this one is merged ^^)

Anyway, thanks for the improvements! 👍

@rigelk
Copy link
Collaborator

rigelk commented Feb 23, 2020

Indeed, to download with curl, you use /master/, but for the link to the previews, you use /develop/ […] I guess we should use /master/ for all?

I personally don't understand why you don't use relative references, since you are already in the peertube directory. You don't need to curl these files, you can just edit them locally. The same goes for links: you replace local references with references to the github.

@LecygneNoir
Copy link
Contributor

LecygneNoir commented Feb 24, 2020

I personally don't understand why you don't use relative references, since you are already in the peertube directory. You don't need to curl these files, you can just edit them locally. The same goes for links: you replace local references with references to the github.

Nope, do not forgot it's a docker usage, so in this documentation you curl the file. You cannot assume the entire git repo has been cloned, as ideally, you just need the docker-compose and some config files and the docker image do all the "app" job for you.

The main advantage of using docker is to avoid cloning and building the app manually.

So, you need to curl the files as you do not have them locally if you have not git cloned the repo :)
Or I am totally misunderstanding your answer sorry 😅

The previous version of the doc also link to the github (and to the /develop/ instead of /master/):

View the source of the files you're about to download: [docker-compose.yml](https://github.com/Chocobozzz/PeerTube/blob/develop/support/docker/production/docker-compose.yml) and the [traefik.toml](https://github.com/Chocobozzz/PeerTube/blob/develop/support/docker/production/config/traefik.toml) and the [.env](https://github.com/Chocobozzz/PeerTube/blob/develop/support/docker/production/.env)

My point was, for the curl part and the View the source of the file you're about to download: we should use same URL, as long as we continue to provide these 2 lines in doc.

Valid only if we keep both lines, which I think we should (one should always look at what he download)

@rigelk
Copy link
Collaborator

rigelk commented Feb 24, 2020

ah, I see. Thanks for putting that into context @LecygneNoir.

@kimsible kimsible force-pushed the chore/improve-docker-documentation branch from d5c227c to 4d7225b Compare February 24, 2020 14:09
@kimsible
Copy link
Contributor Author

I have just noticed (already present in the old doc it seems) that the files used for "preview" are different from the files downloaded.
Indeed, to download with curl, you use /master/, but for the link to the previews, you use /develop/

Thanks for the review @LecygneNoir and @rigelk, it's fixed.

@Chocobozzz Chocobozzz merged commit e962e1c into Chocobozzz:develop Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Docker 🐳 Deals with containerisation, a hellish nightmare for Chocobos Component: Documentation 📚
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants