Split create_basic_client into two methods #14673

lmazuel · 2020-10-21T18:41:33Z

Should help with

azure-sdk-for-python/sdk/schemaregistry/azure-schemaregistry/tests/async_tests/test_schema_registry_async.py

Lines 44 to 101 in 83ec2c4

    
           def create_basic_client(self, client_class, **kwargs): 
        
               # This is the patch for creating client using aio identity 
        
               tenant_id = os.environ.get("AZURE_TENANT_ID", None) 
        
               client_id = os.environ.get("AZURE_CLIENT_ID", None) 
        
               secret = os.environ.get("AZURE_CLIENT_SECRET", None) 
        
               if tenant_id and client_id and secret and self.is_live: 
        
                   if _is_autorest_v3(client_class): 
        
                       # Create azure-identity class 
        
                       from azure.identity.aio import ClientSecretCredential 
        
                       credentials = ClientSecretCredential( 
        
                           tenant_id=tenant_id, 
        
                           client_id=client_id, 
        
                           client_secret=secret 
        
                       ) 
        
                   else: 
        
                       # Create msrestazure class 
        
                       from msrestazure.azure_active_directory import ServicePrincipalCredentials 
        
                       credentials = ServicePrincipalCredentials( 
        
                           tenant=tenant_id, 
        
                           client_id=client_id, 
        
                           secret=secret 
        
                       ) 
        
               else: 
        
                   if _is_autorest_v3(client_class): 
        
                       credentials = self.AsyncFakeCredential() 
        
                       #credentials = self.settings.get_azure_core_credentials() 
        
                   else: 
        
                       credentials = self.settings.get_credentials() 
        
               # Real client creation 
        
               # FIXME decide what is the final argument for that 
        
               # if self.is_playback(): 
        
               #     kwargs.setdefault("polling_interval", 0) 
        
               if _is_autorest_v3(client_class): 
        
                   kwargs.setdefault("logging_enable", True) 
        
                   client = client_class( 
        
                       credential=credentials, 
        
                       **kwargs 
        
                   ) 
        
               else: 
        
                   client = client_class( 
        
                       credentials=credentials, 
        
                       **kwargs 
        
                   ) 
        
               if self.is_playback(): 
        
                   try: 
        
                       client._config.polling_interval = 0  # FIXME in azure-mgmt-core, make this a kwargs 
        
                   except AttributeError: 
        
                       pass 
        
               if hasattr(client, "config"):  # Autorest v2 
        
                   if self.is_playback(): 
        
                       client.config.long_running_operation_timeout = 0 
        
                   client.config.enable_http_logger = True 
        
               return client

…ow, added a positional arg for create_client_from_credential for services that might have required positional args

tools/azure-sdk-tools/devtools_testutils/azure_testcase.py

… and removed extra code from schema registry to verify works

… when in Python 3

yunhaoling · 2020-10-21T23:51:23Z

tools/azure-sdk-tools/devtools_testutils/azure_testcase.py

+                return ServicePrincipalCredentials(
                    tenant=tenant_id,
                    client_id=client_id,
                    secret=secret
                )


as now you're approaching the _get_real_credential method for returning real credential, I'm wondering we could do further unification here for this ServicePrincipalCredentials as well.

maybe we could just tweak the _get_real_credential signature a bit, making it

def _get_real_credential(self, is_async=false, crendetial_type=None): # if crendetial_type is provided, just use the given type if crendetial_type: return crendetial_type(client_id=...) # otherwise we fall back to some default paths if is_async: from ..

@yunhaoling that would have to be a string, since we can't import aio code in 2.7 (right now it works because the aio code is smartly wrapped in the right if).

So I'm not against, but I'm not sure we need in the first place (are we trying to solve an actual problem, or just do something cool for fun? :)) and this string challenge make the API less than ideal.

I'm happy to be pushed back if you strongly believe in it

@lmazuel emmm, I see the reverse, it's because we're in the py2.7 so that we won't be importing and passing aio credential_type into the method -- it's the caller's duty to check which env/is async/etc. and make the right import.
the callee _get_credential simply takes what has been passed.

as you said, it's mostly "something cool for fun" -- the place where I come from is for the long term maintainability/scalability/readability -- that's what a unified approach could potentially provide.

but I might be bikeshedding and overengineering, let's waste no more time on this and get it work in our repo first😊

(btw: I didn't mean to block the PR, I'm good with these changes)

tools/azure-sdk-tools/devtools_testutils/azure_testcase.py

seankane-msft · 2020-10-22T16:28:55Z

Addresses #13594

…hich does the same thing

seankane-msft · 2020-10-27T18:19:15Z

/azp run python - tables - tests

azure-pipelines · 2020-10-27T18:19:31Z

Azure Pipelines successfully started running 1 pipeline(s).

seankane-msft · 2020-10-27T18:20:44Z

/azp run python - schemaregistry - tests

azure-pipelines · 2020-10-27T18:20:54Z

Azure Pipelines successfully started running 1 pipeline(s).

seankane-msft · 2020-10-27T18:57:31Z

/azp run python - tables - tests

azure-pipelines · 2020-10-27T18:57:41Z

Azure Pipelines successfully started running 1 pipeline(s).

… correctly

…ow, added a positional arg for create_client_from_credential for services that might have required positional args

iscai-msft · 2020-11-03T15:52:49Z

tools/azure-sdk-tools/devtools_testutils/azure_testcase.py

@@ -241,6 +256,11 @@ def create_basic_client(self, client_class, **kwargs):
            client.config.enable_http_logger = True
        return client

+    def create_basic_client(self, client_class, **kwargs):
+        """ DO NOT USE ME ANYMORE."""


tiniest nit: do we want to log a warning instead of having a comment, more discoverable

…t_cred/create from cred

…into add_invoice_multipage * 'master' of https://github.com/Azure/azure-sdk-for-python: (31 commits) add more samples (Azure#15007) Address feedbacks: Rename type, improve readme (Azure#14905) Correct Visual Studio Code default environment name (Azure#14648) [ServiceBus] Adjust AutoLockRenewer to only allow registration of intended types (ReceivedMessage and ServiceBusSession) (Azure#14600) Validate tenant IDs used in URLs (Azure#14955) [ServiceBus] Make sub-client initializers internal (add '_' to from_conn_str for receiver/sender) (Azure#14502) Raise minimum msal version to 1.6.0 (Azure#14992) update monitor to target '*' instead of azure-* (Azure#14985) Split create_basic_client into two methods (Azure#14673) [ServiceBus] receive related methods/constructor raise error if given max_wait_time <= 0 (Azure#14925) String unicode deserial (Azure#14958) Clarified exceptions docs (Azure#14947) search-documents: correct method calls in sample (Azure#14980) Catch exceptions thrown by async download (Azure#14946) Increment package version after release of azure_eventgrid (Azure#14266) [ServiceBus] Add additional stress test coverage to ensure parity with cross-language priorities (Azure#14437) Service Bus Connection String Parser (Azure#14772) Remove AmendCommit parameter (Azure#14965) revert creation takes full object change (Azure#14957) [ServiceBus] Settlement move from Message to Receiver (Azure#14681) ...

* Update azure_testcase.py * updated test_table(_async) to verify changes to azure_testcase work now, added a positional arg for create_client_from_credential for services that might have required positional args * removed the positional *args in favor of pushing towards kwargs * added private _get_real_credential with kwarg for is_async * added the async fake credential that was copied from schema registry, and removed extra code from schema registry to verify works * breaking the fake async credential into its own file and importing it when in Python 3 * fixing up comments based on laurent/adams review * removing the second boolean for is_real, using self.is_live instead which does the same thing * updating credential to be self.xxx_cred, caught in the tables live tests * fixed a create_credential in test table async that did not use kwargs correctly * Update azure_testcase.py * updated test_table(_async) to verify changes to azure_testcase work now, added a positional arg for create_client_from_credential for services that might have required positional args * removed the positional *args in favor of pushing towards kwargs * added private _get_real_credential with kwarg for is_async * added the async fake credential that was copied from schema registry, and removed extra code from schema registry to verify works * breaking the fake async credential into its own file and importing it when in Python 3 * fixing up comments based on laurent/adams review * removing the second boolean for is_real, using self.is_live instead which does the same thing * updating credential to be self.xxx_cred, caught in the tables live tests * fixed a create_credential in test table async that did not use kwargs correctly * changing a word to force check-enforcer to re-evaulate CI * Clarify async protocol * adding a logging message to warn about create_basic_client and use get_cred/create from cred * fixing up the syntax of the logger Co-authored-by: seankane-msft <[email protected]>

…into add_invoice_multipage_tests * 'master' of https://github.com/Azure/azure-sdk-for-python: (31 commits) add more samples (Azure#15007) Address feedbacks: Rename type, improve readme (Azure#14905) Correct Visual Studio Code default environment name (Azure#14648) [ServiceBus] Adjust AutoLockRenewer to only allow registration of intended types (ReceivedMessage and ServiceBusSession) (Azure#14600) Validate tenant IDs used in URLs (Azure#14955) [ServiceBus] Make sub-client initializers internal (add '_' to from_conn_str for receiver/sender) (Azure#14502) Raise minimum msal version to 1.6.0 (Azure#14992) update monitor to target '*' instead of azure-* (Azure#14985) Split create_basic_client into two methods (Azure#14673) [ServiceBus] receive related methods/constructor raise error if given max_wait_time <= 0 (Azure#14925) String unicode deserial (Azure#14958) Clarified exceptions docs (Azure#14947) search-documents: correct method calls in sample (Azure#14980) Catch exceptions thrown by async download (Azure#14946) Increment package version after release of azure_eventgrid (Azure#14266) [ServiceBus] Add additional stress test coverage to ensure parity with cross-language priorities (Azure#14437) Service Bus Connection String Parser (Azure#14772) Remove AmendCommit parameter (Azure#14965) revert creation takes full object change (Azure#14957) [ServiceBus] Settlement move from Message to Receiver (Azure#14681) ...

Update azure_testcase.py

a8a078f

lmazuel requested a review from seankane-msft October 21, 2020 18:41

updated test_table(_async) to verify changes to azure_testcase work n…

52c9b1c

…ow, added a positional arg for create_client_from_credential for services that might have required positional args

seankane-msft self-assigned this Oct 21, 2020

seankane-msft requested a review from KieranBrantnerMagee October 21, 2020 19:28

lmazuel commented Oct 21, 2020

View reviewed changes

tools/azure-sdk-tools/devtools_testutils/azure_testcase.py Outdated Show resolved Hide resolved

seankane-msft added 3 commits October 21, 2020 13:27

removed the positional *args in favor of pushing towards kwargs

2ba457e

added private _get_real_credential with kwarg for is_async

4ab4cd3

added the async fake credential that was copied from schema registry,…

cd97f09

… and removed extra code from schema registry to verify works

lmazuel requested a review from yunhaoling as a code owner October 21, 2020 21:37

breaking the fake async credential into its own file and importing it…

07eb9de

… when in Python 3

yunhaoling reviewed Oct 21, 2020

View reviewed changes

tools/azure-sdk-tools/devtools_testutils/azure_testcase.py Outdated Show resolved Hide resolved

yunhaoling reviewed Oct 22, 2020

View reviewed changes

tools/azure-sdk-tools/devtools_testutils/azure_testcase.py Outdated Show resolved Hide resolved

fixing up comments based on laurent/adams review

7e110aa

seankane-msft requested a review from yunhaoling October 22, 2020 23:46

removing the second boolean for is_real, using self.is_live instead w…

65bea77

…hich does the same thing

updating credential to be self.xxx_cred, caught in the tables live tests

4360a14

seankane-msft and others added 5 commits October 27, 2020 13:57

fixed a create_credential in test table async that did not use kwargs…

dd71477

… correctly

Update azure_testcase.py

f8e9a4f

updated test_table(_async) to verify changes to azure_testcase work n…

a2e467a

…ow, added a positional arg for create_client_from_credential for services that might have required positional args

removed the positional *args in favor of pushing towards kwargs

b7bd954

added private _get_real_credential with kwarg for is_async

5cfc599

lmazuel requested a review from KieranBrantnerMagee as a code owner November 3, 2020 00:17

lmazuel force-pushed the async_cred_test branch 2 times, most recently from 318cdf0 to 9b68e71 Compare November 3, 2020 00:20

Merge remote-tracking branch 'origin/master' into async_cred_test

69c5dee

lmazuel requested review from yunhaoling and removed request for amishra-dev, annatisch, chlowell, danieljurek, lzchen, mayurid, mitchdenny, rakshith91, RezaJooyandeh, schaabs, southpolesteve, turalf, weshaggard and zfoster November 3, 2020 00:22

Clarify async protocol

208b6a3

iscai-msft previously approved these changes Nov 3, 2020

View reviewed changes

adding a logging message to warn about create_basic_client and use ge…

ac5ce2f

…t_cred/create from cred

seankane-msft dismissed iscai-msft’s stale review via ac5ce2f November 3, 2020 19:49

fixing up the syntax of the logger

ec9b9f0

iscai-msft approved these changes Nov 3, 2020

View reviewed changes

lmazuel merged commit 4762e58 into master Nov 3, 2020

lmazuel deleted the async_cred_test branch November 3, 2020 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split create_basic_client into two methods #14673

Split create_basic_client into two methods #14673

lmazuel commented Oct 21, 2020

yunhaoling Oct 21, 2020

lmazuel Oct 22, 2020

yunhaoling Oct 27, 2020 •

edited

Loading

seankane-msft commented Oct 22, 2020

seankane-msft commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

seankane-msft commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

seankane-msft commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

iscai-msft Nov 3, 2020

	def create_basic_client(self, client_class, **kwargs):
	# This is the patch for creating client using aio identity

	tenant_id = os.environ.get("AZURE_TENANT_ID", None)
	client_id = os.environ.get("AZURE_CLIENT_ID", None)
	secret = os.environ.get("AZURE_CLIENT_SECRET", None)

	if tenant_id and client_id and secret and self.is_live:
	if _is_autorest_v3(client_class):
	# Create azure-identity class
	from azure.identity.aio import ClientSecretCredential
	credentials = ClientSecretCredential(
	tenant_id=tenant_id,
	client_id=client_id,
	client_secret=secret
	)
	else:
	# Create msrestazure class
	from msrestazure.azure_active_directory import ServicePrincipalCredentials
	credentials = ServicePrincipalCredentials(
	tenant=tenant_id,
	client_id=client_id,
	secret=secret
	)
	else:
	if _is_autorest_v3(client_class):
	credentials = self.AsyncFakeCredential()
	#credentials = self.settings.get_azure_core_credentials()
	else:
	credentials = self.settings.get_credentials()

	# Real client creation
	# FIXME decide what is the final argument for that
	# if self.is_playback():
	# kwargs.setdefault("polling_interval", 0)
	if _is_autorest_v3(client_class):
	kwargs.setdefault("logging_enable", True)
	client = client_class(
	credential=credentials,
	**kwargs
	)
	else:
	client = client_class(
	credentials=credentials,
	**kwargs
	)

	if self.is_playback():
	try:
	client._config.polling_interval = 0 # FIXME in azure-mgmt-core, make this a kwargs
	except AttributeError:
	pass

	if hasattr(client, "config"): # Autorest v2
	if self.is_playback():
	client.config.long_running_operation_timeout = 0
	client.config.enable_http_logger = True
	return client

Split create_basic_client into two methods #14673

Split create_basic_client into two methods #14673

Conversation

lmazuel commented Oct 21, 2020

yunhaoling Oct 21, 2020

Choose a reason for hiding this comment

lmazuel Oct 22, 2020

Choose a reason for hiding this comment

yunhaoling Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

seankane-msft commented Oct 22, 2020

seankane-msft commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

seankane-msft commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

seankane-msft commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

iscai-msft Nov 3, 2020

Choose a reason for hiding this comment

yunhaoling Oct 27, 2020 •

edited

Loading