Implement instance_discovery=False #496
Conversation
rayluo
force-pushed
the
instance-discovery-endpoint
branch
from
d314b11 to
027e820
Compare
rayluo
force-pushed
the
known_authorities
branch
from
35e00a3 to
20c0da3
Compare
rayluo
force-pushed
the
instance-discovery-endpoint
branch
from
027e820 to
03fe109
Compare
rayluo
force-pushed
the
instance-discovery-endpoint
branch
from
03fe109 to
de8d4b2
Compare
| @@ -174,6 +173,7 @@ def __init__( | |||
| # when we would eventually want to add this feature to PCA in future. | |||
| exclude_scopes=None, | |||
| http_cache=None, | |||
| instance_discovery=None, | |||
There was a problem hiding this comment.
Is instance_discovery=None here means default to False?
I guess we want the default value to be true?
There was a problem hiding this comment.
Basically, I think I am moving away from using True or False as the default value, because they would force downstream yet mid-tier libraries (such as Azure Identity) to also duplicate the same default value in their corresponding API surface.
I would instead use None as default value, and it just means default. It does NOT imply a boolean evaluation to make it a False.
And the actual default behavior will and only will be available via documentation. See line 419 in this PR, or read the rendered docs in the staging environment.
There was a problem hiding this comment.
Do you mean the caller (me :)) can specify None as the input value of instance_discovery to say "use default"?
Driven by this conversation, this PR implements
instance_discovery=Falsebased on internal design 6504. (Note: This PR does NOT containknown_authority_hosts, which was implemented in a different PR #492. The two features would co-exist when both PRs are merged in, but they can also work independently. Here they are implemented independently.)As usual, the unit tests in this PR also serve as acceptance test for the aforementioned design.