deprecate hosted_domain in favor of restrict_hosted_domains, always apply strip_domain if True, clarify docstrings - #830
Conversation
manics
left a comment
There was a problem hiding this comment.
I think this is fine, and deprecating is also fine if you want since the configuration is pretty complex.
Is it intentional that strip_domain is ignored for everything other than hosted_domain
oauthenticator/oauthenticator/google.py
Lines 250 to 251 in 5ffbbe9
i.e. domains are never stripped if you're using allowed_domains?
|
+1 for rename & deprecate, I think it makes this much clearer. |
I think that's a valid question, and I think the answer is "yes" that it's intentional, but that doesn't mean it has to be that way. It seems confusing that |
Co-authored-by: Yuvi <yuvipanda@gmail.com>
|
What should the behavior of Here's what I think is the simplest:
2 and 3 keep consistent current behavior and offer a smooth deprecation path, but 1 could be considered a breaking change, because if anyone had explicit |
and deprecate implicit strip_domain=True for single hosted_domain strip_domain now ignores hosted_domain config and applies to all accounts
|
This PR now reflects the above proposal - |
docstrings Co-authored-by: Simon Li <orpheus+devel@gmail.com>
related to #828 and the confusion that led to it
In discussing this, I also considered deprecating
hosted_domainin favor ofrestrict_hosted_domainsto be even clearer and disambiguate from the newallow_hosted_domains, but didn't take that step here, this is just the docstring.