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

Updated the BBB plugin to accept any API URL #1914

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

Conversation

ianespana
Copy link

Some BBB service providers have configured their API URLs differently than the default https://bigbluebutton.mycompany.com/bigbluebutton/api/.

For instance, a service provider called MynaParrot utilizes https://api.mynaparrot.com/bigbluebutton/username/api/ as their API URL. This would not work with Canvas as it was expecting the URLs to be formatted like the default one, and thus limited the amount of services it'd work with quite severly.

Now we just make sure said URL ends with /api/ instead of assuming the default format is being used.

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2021

CLA assistant check
All committers have signed the CLA.

@maths22
Copy link
Member

maths22 commented Jul 10, 2021

Thank you for your contribution. While I like the idea of this change, as currently implemented this will break any existing plugin configurations. I would suggest either adding a migration to automatically transform existing plugin settings to use the new format (if you do that you'll need to migrate into a new field name in a predeploy migration (e.g. :address instead of :host) so that conferences still work after the migration has run before the new code is deployed) or I would suggest detecting if the url starts with http(s):// and using the new logic if it does and the old logic if it doesn't.

Also please don't update the language .yml files, those will be automatically updated separately through an automatic process.

@ianespana
Copy link
Author

Oh, right. I'll implement the second way (check if https:// exists). As for the translations I did not know, so I will revert those

This reverts commit d7ff213, reversing
changes made to 78be0ec.
If the domain contains http(s), treat it as a custom URL,
else treat it as a hostname/ip (default behavior)
@ianespana
Copy link
Author

ianespana commented Jul 10, 2021

Changes done, it now uses the old logic if http(s):// is not set in the domain configuration

@dustin-cowles dustin-cowles self-assigned this Aug 30, 2022
@dustin-cowles
Copy link
Contributor

Hi @ianespana , sorry this PR got missed. If you can verify it is still valid and resolve the conflicts, I can get it reviewed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants