Closed IgorKhomenko closed 1 day ago
Thanks for your contribution, @IgorKhomenko! Please make sure you sign our CLA, as it's a required step before we can merge this.
I see many problems in this patch:
string_ids_user
from the configuration file, which is what we do for string_ids
; this means people need to set the static value to TRUE
and recompile if they want to use this, which makes little sense;string_ids
to TRUE
, the user counterpart defaults to FALSE
meaning user IDs will not be strings, breaking people's expectations (and possibly applications);As such, this patch needs a lot of fixing IMHO before we can review/test it. While it's ok to use the dedicated booleans for the checks, these should all be configurable via the configuration file, and when the new properties are not provided, the old string_ids
should act as a fallback.
@lminiero
1 - this is addressed and pushed
2 - what you say make total sense. Probably we can check if string_ids_user
is intentionally presented in config (either set to true or false) and if not - then use string_ids
as a fallback.
3 - did not get the point. With this patch there is now a way to separately configure it for rooms and users, e.g. it can be any combination, e.g. true/true
or true/false
or false/true
or false/false
did not get the point. With this patch there is now a way to separately configure it for rooms and users
I don't see this possibility: string_ids
is for both, not only for rooms, which is why I stressed the backwards compatibility part. You can't change the meaning of a property to have it only impact part of what it impacted before, it would break existing applications. There would need to be a new property called, e.g., string_ids_room
specifically for rooms, like you added one specifically for users, and both should default to whatever the value of string_ids
is set to, unless individually changed to something else via config.
@IgorKhomenko did you have a chance to revisit the PR with the changes I suggested?
Hi @IgorKhomenko, did you close the PR to create a different one, or are you not interested in adding the feature anymore? I was waiting for feedback but didn't get any. Thanks!
Issue reference https://github.com/meetecho/janus-gateway/issues/3364#issuecomment-2088184440