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

Revert "[SRP] Add default response status code" #8074

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

blueww
Copy link
Member

@blueww blueww commented Jan 7, 2020

Reverts #8065

We need to revert #8065, since it will cause the display issue of the error responds, and is a breaking change to SDK.
We should not make it in until the display issue is fixed?
Or PSH upgrade to new SRP SDK will be blocked.

Such as for an error responds with StatusCode as Conflict (409), and following content:

{
  "error": {
    "code": "StorageDomainNameCouldNotVerify",
    "message": "The custom domain name could not be verified. CNAME mapping from foo.example.com to any of sto1511.blob.core.windows.net,sto1511.z1.web.core.windows.net does not exist."
  }
}

Before the change, the exception will have the Error code and message in Exception.Body.code, and Exception.Message
• Exception.Response.StatusCode: Conflict
• Exception.Body.code: StorageDomainNameCouldNotVerify
• Exception.Message: The custom domain name could not be verified. CNAME mapping from foo.example.com to any of sto1511.blob.core.windows.net,sto1511.z1.web.core.windows.net does not exist.

But after the change, no properties of the exception has the Error Code and Message, but only has the status code (a property has whole responds content, which is not friendly for user to read):
• Exception.Response.StatusCode: Conflict
• Exception.Body.code: null
• Exception.Message: Operation returned an invalid status code 'Conflict'

So it’s difficult for user to know the failure reason, especially for Powershell (depends on .NET SRP SDK generated from swagger) since it will display the Exception.Message to console output.
After the change the Exception.Message can’t show the detail reason, so it’s difficult for powershell user to handle the failures.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@blueww blueww requested a review from zfchen95 January 7, 2020 06:49
@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jan 7, 2020

azure-sdk-for-js - Release

️✔️ succeeded [Logs] [Expand Details]
  • ️✔️ Generate from c24cd81 with merge commit 68831c6. SDK Automation 13.0.17.20191226.1
  • ️✔️@azure/arm-storage [Logs]  [Release SDK Changes]
    [npmPack] npm WARN deprecated [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-node-resolve.
    [npmPack] loaded rollup.config.js with warnings
    [npmPack] (!) Unused external imports
    [npmPack] default imported from external module 'rollup' but never used
    [npmPack] 
    [npmPack] ./esm/storageManagementClient.js → ./dist/arm-storage.js...
    [npmPack] created ./dist/arm-storage.js in 569ms

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jan 7, 2020

azure-sdk-for-python - Release

⚠️ warning [Logs] [Expand Details]
  • ⚠️ Generate from c24cd81 with merge commit 68831c6. SDK Automation 13.0.17.20191226.1
    Failed to find any diff after autorest so no changed packages was found.

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jan 7, 2020

azure-sdk-for-go - Release

️✔️ succeeded [Logs] [Expand Details]

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jan 7, 2020

azure-sdk-for-net - Release

️✔️ succeeded [Logs] [Expand Details]

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jan 7, 2020

azure-sdk-for-java - Release

⚠️ warning [Logs] [Expand Details]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants