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

Add Jitsi #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add Jitsi #23

wants to merge 4 commits into from

Conversation

SailReal
Copy link
Contributor

@SailReal SailReal commented Apr 17, 2022

Mhhh still have to fix the tests, I'll do it tomorrow.

@tomMoulard
Copy link
Owner

Hey @SailReal,

I've got a few remarks for you:
Why do you expose ports, instead of routing them through Traefik?
Why do you define the middle ware and do not use it?
Why do you add so much environment variable, and do not check if some can be interpreted with some other ones (like TZ)?
Why do you use the CONFIG env var instead of . /jisti/conf ?

@SailReal
Copy link
Contributor Author

Thank you for the review!

Why do you expose ports, instead of routing them through Traefik?

A good and valid point, I will take another look at it.

Why do you define the middle ware and do not use it?

It should be used in prosody in https://github.com/SailReal/make-my-server/blob/8a806f933f4ed5879e3f3b35975cf0830cb0571c/jitsi/docker-compose.jitsi.yml#L219

Why do you add so much environment variable, and do not check if some can be interpreted with some other ones (like TZ)?

Will fix that, thanks for pointing out.

Why do you use the CONFIG env var instead of . /jisti/conf?

Because CONFIG is used very often, but I can also copy copy paste ./jisti/conf to all occurrences.

@SailReal
Copy link
Contributor Author

I've removed the exposed ports, need to fix the tests, would it then meet your needs?

@tomMoulard
Copy link
Owner

Awesome !

For the middleware, I see that you define it as middlewares-secure-headers-jitsi but use it as chain-jitsi-auth. I'll go with middlewares-secure-headers-jitsi everywhere, and define it on the prosody service itself. Doing so will create the middleware only when this service is activated.

jitsi/docker-compose.jitsi.yml Outdated Show resolved Hide resolved
jitsi/docker-compose.jitsi.yml Outdated Show resolved Hide resolved
jitsi/docker-compose.jitsi.yml Outdated Show resolved Hide resolved
jitsi/docker-compose.jitsi.yml Outdated Show resolved Hide resolved
Comment on lines +129 to +130
aliases:
- ${XMPP_DOMAIN}
Copy link
Owner

Choose a reason for hiding this comment

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

Why ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'm not quiet sure, but if I remove it I get "Bad gateway" errors and took it from here: https://github.com/jitsi-contrib/jitsi-traefik/blob/main/traefik-v2/docker-compose.yml#L101-L103 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

OK, I'll have to check this out 🤔

But if you can do otherwise it could be better to have unified docker-compose files in the project

traefik/dynamic_conf/chain-jitsi-auth.yml Outdated Show resolved Hide resolved
Co-authored-by: Tom Moulard <tom@moulard.org>
@tomMoulard tomMoulard added the kind/enhancement New feature or request label Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants