[SRP] Add delete retention policy for blob container and file share #7626

zfchen95 · 2019-10-24T21:58:51Z

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

I have reviewed the documentation for the workflow.
Validation tools were run on swagger spec(s) and have all been fixed in this PR.
The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
adding/removing APIs.
adding/removing properties.
adding/removing API-version.
adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Please follow the link to find more details on API review process.

openapi-sdkautomation · 2019-10-24T21:58:55Z

In Testing, Please Ignore

[Logs] (Generated from eb302b3, Iteration 2)

.NET: test-repo-billy/azure-sdk-for-net [Logs]

Package generation in progress.

Python: test-repo-billy/azure-sdk-for-python

Package generation pending.

Java: test-repo-billy/azure-sdk-for-java [Logs]

Package generation in progress.

Go: test-repo-billy/azure-sdk-for-go [Logs]

Package generation in progress.

JavaScript: test-repo-billy/azure-sdk-for-js

Package generation pending.

Ruby: test-repo-billy/azure-sdk-for-ruby

Package generation pending.

azuresdkci · 2019-10-24T22:01:13Z

Can one of the admins verify this patch?

AutorestCI · 2019-10-24T22:02:01Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#6200

yungezz · 2019-10-25T01:43:57Z

specification/storage/resource-manager/Microsoft.Storage/stable/2019-06-01/blob.json

@@ -1142,6 +1142,10 @@
            "changeFeed": {
              "$ref": "#/definitions/ChangeFeed",
              "description": "The blob service properties for change feed events."
+            },
+            "containerDeleteRetentionPolicy": {


the neww property will be think as breaking change in stable version. suggest to add it in new version

@yungezz
I don't see any reason why add new properties are breaking change, would you please give some reason.
And we have add new properties in same API verison before, but not take as breaking.
Per my understanding, REmove/modify a property will breaking, but add new properties are not breaking.

https://github.com/Azure/adx-documentation-pr/wiki/Breaking-changes-guidelines#new-required-property-added-to-request,
https://github.com/Azure/adx-documentation-pr/wiki/Breaking-changes-guidelines#new-property-added-to-response

Yunge, I understand your concern. Adding a new property can cause data lost during update. June19 is the version we added last week. We can't afford to add a new version for every new feature. As Wei mentioned, we have been adding properties in the same API version before, which does not cause any issue.

@yungezz
Thanks for the sharing, from the doc I see : "This will overwrite the change made by the first customer from the portal."

Per my understanding, in current storage object update request, if the request send to server does not contain a property (since not get from server), the property will keep the origin value and won't be updated. @zfchen95 , would you please confirm?
In this case, the GET-PUT pipeline won't be broken, so there are no breaking change.

Correct. If the property is not shown in the request, it will not clear the property.

@yungezz
Do you agree adding the new property in this PR is not a breaking? since it won't break the GET-PUT pipeline. Let us know if you still have concern on this.

@blueww as long as service team can make sure no user breaking experience, we're ok to it.

yungezz · 2019-10-25T01:44:19Z

specification/storage/resource-manager/Microsoft.Storage/stable/2019-06-01/file.json

@@ -432,6 +432,10 @@
            "cors": {
              "$ref": "./common.json#/definitions/CorsRules",
              "description": "Specifies CORS rules for the File service. You can include up to five CorsRule elements in the request. If no CorsRule elements are included in the request body, all CORS rules will be deleted, and CORS will be disabled for the File service."
+            },
+            "shareDeleteRetentionPolicy": {


same as above.

blueww · 2019-10-25T01:50:37Z

@zfchen95
We also have the delete retention policy on Blob service properties. Why we still need it on blob container? Will the container policy overwrite the service policy?
Would you please give the feature doc link in the PR, so we can get more info for the feature, and can help the review.

zfchen95 · 2019-10-25T04:04:43Z

@zfchen95
We also have the delete retention policy on Blob service properties. Why we still need it on blob container? Will the container policy overwrite the service policy?
Would you please give the feature doc link in the PR, so we can get more info for the feature, and can help the review.

The existing delete retention policy is for blob soft delete. The new container delete retention policy is for container soft delete. @priyapansrp may have the documentation.

priyapansrp · 2019-10-28T18:15:11Z

@zfchen95
We also have the delete retention policy on Blob service properties. Why we still need it on blob container? Will the container policy overwrite the service policy?
Would you please give the feature doc link in the PR, so we can get more info for the feature, and can help the review.

The existing delete retention policy is for blob soft delete. The new container delete retention policy is for container soft delete. @priyapansrp may have the documentation.

These are for two different feature. The first one is for blob soft delete which is different than container soft delete.

zfchen95 · 2019-10-29T21:35:25Z

@yungezz Could you remove the DoNotMerge tag. Thanks!

yungezz · 2019-10-31T05:10:38Z

hi @zfchen95 is the service change public? is the PR ready to merge?

zfchen95 · 2019-10-31T06:02:45Z

hi @zfchen95 is the service change public? is the PR ready to merge?

Hi @yungezz , you can merge this PR

AutorestCI · 2019-10-31T06:05:50Z

Automation for azure-sdk-for-python

Encountered a Subprocess error: (azure-sdk-for-python)

Command: ['/usr/local/bin/autorest', '/tmp/tmp9398nq5i/rest/specification/storage/resource-manager/readme.md', '--perform-load=false', '--swagger-to-sdk', '--output-artifact=configuration.json', '--input-file=foo', '--output-folder=/tmp/tmp95pf5trk']
Finished with return code 7
and output:

AutoRest code generation utility [version: 2.0.4283; node: v8.12.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Failure:
Error: Unable to start AutoRest Core from /root/.autorest/@microsoft.azure_autorest-core@2.0.4405/node_modules/@microsoft.azure/autorest-core
Error: Unable to start AutoRest Core from /root/.autorest/@microsoft.azure_autorest-core@2.0.4405/node_modules/@microsoft.azure/autorest-core
    at main (/opt/node_modules/autorest/dist/app.js:232:19)
    at <anonymous>

/root/.autorest/@microsoft.azure_autorest-core@2.0.4405/node_modules/@microsoft.azure/autorest-core/dist/app.js:33
    autorest_core_1.Shutdown();
    ^
ReferenceError: autorest_core_1 is not defined
    at process.on (/root/.autorest/@microsoft.azure_autorest-core@2.0.4405/node_modules/@microsoft.azure/autorest-core/dist/app.js:33:5)
    at emitOne (events.js:121:20)
    at process.emit (events.js:211:7)
    at process.emit (/node_modules/source-map-support/source-map-support.js:439:21)
fs.js:612
  return binding.close(fd);
                 ^

Error: EBADF: bad file descriptor, close
    at Object.fs.closeSync (fs.js:612:18)
    at StaticVolumeFile.shutdown (/opt/node_modules/autorest/dist/static-loader.js:352:10)
    at StaticFilesystem.shutdown (/opt/node_modules/autorest/dist/static-loader.js:406:17)
    at process.exit.n [as exit] (/opt/node_modules/autorest/dist/static-loader.js:169:11)
    at printErrorAndExit (/node_modules/source-map-support/source-map-support.js:423:11)
    at process.emit (/node_modules/source-map-support/source-map-support.js:435:16)
    at process._fatalException (bootstrap_node.js:391:26)

[SRP] Add delete retention policy for blob container and file share

eb302b3

zfchen95 changed the title ~~[SRP] Add delete retention policy for blob container and file share~~ [Do not merge][SRP] Add delete retention policy for blob container and file share Oct 24, 2019

azuresdkci assigned yungezz Oct 24, 2019

azuresdkci requested a review from yungezz October 24, 2019 21:59

AutorestCI mentioned this pull request Oct 24, 2019

[AutoPR storage/resource-manager] [SRP] Add delete retention policy for blob container and file share Azure/azure-sdk-for-go#6148

Merged

yungezz added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Oct 25, 2019

yungezz reviewed Oct 25, 2019

View reviewed changes

yungezz added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Oct 25, 2019

zfchen95 changed the title ~~[Do not merge][SRP] Add delete retention policy for blob container and file share~~ [SRP] Add delete retention policy for blob container and file share Oct 29, 2019

zfchen95 requested a review from yungezz October 29, 2019 21:37

pilor added ARMReviewInProgress and removed DoNotMerge <valid label in PR review process> use to hold merge after approval labels Oct 30, 2019

pilor approved these changes Oct 30, 2019

View reviewed changes

pilor added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMReviewInProgress WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Oct 30, 2019

yungezz merged commit 0e99ef8 into Azure:master Oct 31, 2019

zfchen95 mentioned this pull request Nov 5, 2019

[SRP] Remove a recent added property for container soft delete #7706

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SRP] Add delete retention policy for blob container and file share #7626

[SRP] Add delete retention policy for blob container and file share #7626

zfchen95 commented Oct 24, 2019

openapi-sdkautomation bot commented Oct 24, 2019 •

edited

Loading

azuresdkci commented Oct 24, 2019

AutorestCI commented Oct 24, 2019 •

edited

Loading

yungezz Oct 25, 2019

blueww Oct 25, 2019

yungezz Oct 25, 2019

zfchen95 Oct 25, 2019

blueww Oct 25, 2019 •

edited

Loading

zfchen95 Oct 25, 2019

blueww Oct 29, 2019

yungezz Oct 29, 2019

yungezz Oct 25, 2019

blueww Oct 25, 2019

blueww commented Oct 25, 2019

zfchen95 commented Oct 25, 2019

priyapansrp commented Oct 28, 2019

zfchen95 commented Oct 29, 2019

yungezz commented Oct 31, 2019

zfchen95 commented Oct 31, 2019

AutorestCI commented Oct 31, 2019 •

edited

Loading

[SRP] Add delete retention policy for blob container and file share #7626

[SRP] Add delete retention policy for blob container and file share #7626

Conversation

zfchen95 commented Oct 24, 2019

Latest improvements:

Contribution checklist:

ARM API Review Checklist

openapi-sdkautomation bot commented Oct 24, 2019 • edited Loading

In Testing, Please Ignore

[Logs] (Generated from eb302b3, Iteration 2)

azuresdkci commented Oct 24, 2019

AutorestCI commented Oct 24, 2019 • edited Loading

Automation for azure-sdk-for-go

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueww Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueww commented Oct 25, 2019

zfchen95 commented Oct 25, 2019

priyapansrp commented Oct 28, 2019

zfchen95 commented Oct 29, 2019

yungezz commented Oct 31, 2019

zfchen95 commented Oct 31, 2019

AutorestCI commented Oct 31, 2019 • edited Loading

Automation for azure-sdk-for-python

openapi-sdkautomation bot commented Oct 24, 2019 •

edited

Loading

AutorestCI commented Oct 24, 2019 •

edited

Loading

blueww Oct 25, 2019 •

edited

Loading

AutorestCI commented Oct 31, 2019 •

edited

Loading