-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Expose --socket-timeout
for blob uploads and downloads
#6046
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/6046 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test.
logger.debug('Getting data service client service_type=%s', service_type.__name__) | ||
try: | ||
client_kwargs = {'account_name': account_name, | ||
'account_key': account_key, | ||
'connection_string': connection_string, | ||
'sas_token': sas_token} | ||
'sas_token': sas_token, | ||
'socket_timeout': socket_timeout} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferable:
if socket_timeout:
client_kwargs['socket_timeout'] = socket_timeout
Better not to assume default values.
@@ -57,6 +57,8 @@ def load_arguments(self, _): # pylint: disable=too-many-locals, too-many-statem | |||
completer=get_storage_name_completion_list(t_queue_service, 'list_queues')) | |||
progress_type = CLIArgumentType(help='Include this flag to disable progress reporting for the command.', | |||
action='store_true', validator=add_progress_callback) | |||
socket_timeout_type = CLIArgumentType(help='The socket timeout used by the service to regulate data flow.', | |||
type=int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the unit? Second or millisecond?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seconds, will add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One test issue, otherwise looks fine.
|
||
container = self.create_container(account_info) | ||
|
||
with self.assertRaises(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLIException
instead of Exception
. Narrow down the assertion to make sure real issue is exposed.
Addresses #5896
This checklist is used to make sure that common guidelines for a pull request are followed.