Skip to content

[Storage] Fix #22679: az storage account file-service-properties update: Fix AttributeError: 'NoneType' object has no attribute 'smb'#22691

Merged
evelyn-ys merged 1 commit intoAzure:devfrom
evelyn-ys:fix_update_storage_file_properties
Jun 7, 2022
Merged

[Storage] Fix #22679: az storage account file-service-properties update: Fix AttributeError: 'NoneType' object has no attribute 'smb'#22691
evelyn-ys merged 1 commit intoAzure:devfrom
evelyn-ys:fix_update_storage_file_properties

Conversation

@evelyn-ys
Copy link
Copy Markdown
Member

@evelyn-ys evelyn-ys commented Jun 1, 2022 •
edited
Loading

Related command

az storage account file-service-properties update

Description

Fix #22679 , IcM 311511171
update_file_service_properties can't handle properly when instance.protocol_settings is None.

Previously get_file_service_properties would get response "protocolSettings":{"smb":{}} from server.
But recently server won't return "protocalSettings" property any more.

Then CLI would crash with AttributeError: 'NoneType' object has no attribute 'smb'

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

@evelyn-ys
az storage account file-service-properties update: Fix `AttributeEr…
6478891 
…ror: 'NoneType' object has no attribute 'smb'`
@ghost ghost added the Auto-Assign Auto assign by bot label Jun 1, 2022
@ghost ghost requested a review from yonzhan June 1, 2022 03:52
@ghost ghost assigned evelyn-ys Jun 1, 2022
@ghost ghost added this to the Jun 2022 (2022-07-05) milestone Jun 1, 2022
@ghost ghost added the Storage az storage label Jun 1, 2022
@ghost ghost requested a review from jiasli June 1, 2022 03:52
@ghost ghost assigned jiasli Jun 1, 2022
@ghost ghost added the Account az login/account label Jun 1, 2022
@ghost ghost requested a review from wangzelin007 June 1, 2022 03:53
@ghost ghost assigned jsntcy Jun 1, 2022
@ghost ghost added the Portal az portal label Jun 1, 2022
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Jun 1, 2022

Storage

jiasli
jiasli requested changes Jun 1, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jiasli jiasli left a comment

Choose a reason for hiding this comment

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

I don't think this PR is a good idea.

If we implement this check, we should implement checks for all object properties, like this protocolSettings or shareDeleteRetentionPolicy, just in case they are suddenly missing from the API's response.

> az storage account file-service-properties show -n clitest23zhehg2ug7pzcmmt
{
  "cors": {
    "corsRules": []
  },
  "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/clitest47xrljy3nijo3qd5vzsdilpqy5gmhc6vhrxdt4iznh6uaopskftgp4scam2w7drpot4l/providers/Microsoft.Storage/storageAccounts/clitest23zhehg2ug7pzcmmt/fileServices/default",
  "name": "default",
  "protocolSettings": null,
  "resourceGroup": "clitest47xrljy3nijo3qd5vzsdilpqy5gmhc6vhrxdt4iznh6uaopskftgp4scam2w7drpot4l",
  "shareDeleteRetentionPolicy": {
    "allowPermanentDelete": null,
    "days": 0,
    "enabled": false
  },
  "sku": {
    "name": "Standard_LRS",
    "tier": "Standard"
  },
  "type": "Microsoft.Storage/storageAccounts/fileServices"
}

Also, after API fixes this, the check for protocolSettings will be rendered useless.

@jiasli
Copy link
Copy Markdown
Member

jiasli commented Jun 1, 2022 •
edited
Loading

This PR reminds me of #17526 where managedByTenants suddenly became missing.

@evelyn-ys
Copy link
Copy Markdown
Member Author

evelyn-ys commented Jun 2, 2022 •
edited
Loading

If we implement this check, we should implement checks for all object properties, like this protocolSettings or shareDeleteRetentionPolicy, just in case they are suddenly missing from the API's response.

If we want to update sub property of shareDeleteRetentionPolicy, then we should check if it's None. And this is what we already do:

azure-cli/src/azure-cli/azure/cli/command_modules/storage/operations/account.py

Lines 737 to 738 in 05c00f9

if instance.share_delete_retention_policy is not None and instance.share_delete_retention_policy.enabled:
instance.share_delete_retention_policy.days = delete_retention_days

Also, after API fixes this, the check for protocolSettings will be rendered useless.

Service won't "fix" API. They think return None is the fix. Here're words from service team:

The code change is a right behavior from SRP perspective. The call needs to exactly send the response which data plane sends and that's what the fix does

We should do essential checks to make sure our product doesn't break frequently. That's quality requirements for our own product. We can't rely on the quality of external input.

@jiasli
Copy link
Copy Markdown
Member

jiasli commented Jun 2, 2022

We should do essential checks to make sure our product doesn't break frequently. That's quality requirements for our own product. We can't rely on the quality of external input.

Then we need to be prepared that any object property can be None all of a sudden. This is not external input, but Azure's own API. Keeping a consistent interface is both beneficial to us and external customers.

I approved the PR, but I keep my personal opinion.

@evelyn-ys evelyn-ys changed the base branch from dev to release June 2, 2022 08:03
@evelyn-ys evelyn-ys changed the base branch from release to dev June 2, 2022 08:03
@evelyn-ys
Copy link
Copy Markdown
Member Author

evelyn-ys commented Jun 2, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 2, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@evelyn-ys evelyn-ys mentioned this pull request Jun 7, 2022
Closed
@evelyn-ys
Copy link
Copy Markdown
Member Author

evelyn-ys commented Jun 7, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 7, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@wangzelin007
Copy link
Copy Markdown
Member

wangzelin007 commented Jun 7, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 7, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@wangzelin007 wangzelin007 reopened this Jun 7, 2022
@wangzelin007
Copy link
Copy Markdown
Member

wangzelin007 commented Jun 7, 2022

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 7, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@evelyn-ys evelyn-ys merged commit fa8b6ed into Azure:dev Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jiasli jiasli jiasli approved these changes

@calvinhzy calvinhzy calvinhzy approved these changes

@jsntcy jsntcy Awaiting requested review from jsntcy jsntcy is a code owner

@zhoxing-ms zhoxing-ms Awaiting requested review from zhoxing-ms

@yonzhan yonzhan Awaiting requested review from yonzhan

@wangzelin007 wangzelin007 Awaiting requested review from wangzelin007

@kairu-ms kairu-ms Awaiting requested review from kairu-ms

@necusjz necusjz Awaiting requested review from necusjz

@andyliuliming andyliuliming Awaiting requested review from andyliuliming

@dbradish-microsoft dbradish-microsoft Awaiting requested review from dbradish-microsoft

Assignees

@jiasli jiasli

@evelyn-ys evelyn-ys

@jsntcy jsntcy

Labels

Account az login/account Auto-Assign Auto assign by bot Portal az portal Storage az storage

Projects

None yet

Milestone

Jun 2022 (2022-07-05)

Development

Successfully merging this pull request may close these issues.

AZ CLI az storage account file-service-properties update returns - Non-Type SMB Object error

6 participants

@evelyn-ys @yonzhan @jiasli @wangzelin007 @calvinhzy @jsntcy