Skip to content
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

Fix batch ai cluster auto storage account creation issue #7132

Merged
merged 4 commits into from
Aug 24, 2018
Merged

Fix batch ai cluster auto storage account creation issue #7132

merged 4 commits into from
Aug 24, 2018

Conversation

llidev
Copy link
Contributor

@llidev llidev commented Aug 23, 2018

Fix batch ai cluster auto storage account creation issue


This checklist is used to make sure that common guidelines for a pull request are followed.

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

@williexu williexu requested a review from adewaleo August 23, 2018 23:41
@williexu williexu added the BatchAI az batchai label Aug 23, 2018
@tjprescott
Copy link
Member

Fixes #7129.

Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing a bug like this is a customer-facing change and should be called out in the release history.

@@ -3,6 +3,10 @@
Release History
===============

0.4.3
+++++
* Minor fixes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should call out that it fixes the bug since that's a customer-facing change (anyone who's run into that will want to know it's fixed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

@AlexanderYukhanov
Copy link
Contributor

hi, we have test_batchai_cluster_with_auto_storage unit test which is supposed to cover this logic. Do we still run live tests nightly?

@AlexanderYukhanov
Copy link
Contributor

Please fix the following lines as well (if storage switched to named parameters in one place, most probably it switched in other places as well):
549: file_service = FileService(account, key)
551: blob_service = BlockBlobService(account, key)

@@ -575,7 +575,7 @@ def _create_auto_storage_account(storage_client, resource_group, location):
name = _generate_auto_storage_account_name()
check = storage_client.storage_accounts.check_name_availability(name).name_available
storage_client.storage_accounts.create(resource_group, name, {
'sku': Sku(SkuName.standard_lrs),
'sku': Sku(name=SkuName.standard_lrs),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check if 549 and 551 should be changed as well - i suppose the storage sdk switched to named parameters everywhere

@tjprescott tjprescott merged commit 81a9a16 into Azure:dev Aug 24, 2018
@haroldrandom haroldrandom added the BatchAI az batchai label Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BatchAI az batchai
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants