-
-
Notifications
You must be signed in to change notification settings - Fork 68
Add Jitsi #23
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
Conversation
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 ?
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
CONFIGenv var instead of. /jisti/conf?
Because CONFIG is used very often, but I can also copy copy paste ./jisti/conf to all occurrences.
8a806f9 to
9ea882a
Compare
I've removed the exposed ports, need to fix the tests, would it then meet your needs?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ?
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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
Co-authored-by: Tom Moulard <tom@moulard.org>
9d1bc71 to
8135136
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Mhhh still have to fix the tests, I'll do it tomorrow.