Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Add Jitsi #23

SailReal wants to merge 4 commits into tomMoulard:master from SailReal:master

Conversation

@SailReal
Copy link
Contributor

@SailReal SailReal commented Apr 17, 2022
edited
Loading

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

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 ?

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.

Copy link
Contributor Author

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

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.

Comment on lines +129 to +130
aliases:
- ${XMPP_DOMAIN}
Copy link
Owner

@tomMoulard tomMoulard May 19, 2022

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

@tomMoulard tomMoulard May 23, 2022

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>
@tomMoulard tomMoulard added the kind/enhancement New feature or request label Jul 26, 2022
Copilot

This comment was marked as off-topic.

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

Reviewers

@tomMoulard tomMoulard tomMoulard left review comments

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

kind/enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /