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.

Sign up for GitHub

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

Jump to bottom

Adding changes for external soft delete #30296

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Adding changes for external soft delete #30296

wants to merge 14 commits into from

Conversation

sofiya09
Copy link
Member

@sofiya09 sofiya09 commented Aug 22, 2024 •
edited
Loading

Adding new api version 2024-07-01-preview for external soft delete

@sofiya09 sofiya09 added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required ARMReview resource-manager new-api-version labels Aug 22, 2024
@openapi-pipeline-app openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Aug 22, 2024 •
edited
Loading

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

@openapi-pipeline-app openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Aug 22, 2024 •
edited
Loading

Generated ApiView

Language Package Name ApiView Link
Go sdk/resourcemanager/elasticsan/armelasticsan https://apiview.dev/Assemblies/Review/9bc42e251ceb4725956081c95e6ff3b7?revisionId=27b5a52447274373ac483864b0d3f31a
Java azure-resourcemanager-elasticsan https://apiview.dev/Assemblies/Review/5563a324ee92460db37f8d840c33eeb3?revisionId=299625f334fb4d86a6a2ea7e4bbe0bac
Python azure-mgmt-elasticsan https://apiview.dev/Assemblies/Review/39109b530de34d9bb4fde6838c46077e?revisionId=c0e3b3c8f43c4a519fc6763a50b22600
.Net Azure.ResourceManager.ElasticSan https://apiview.dev/Assemblies/Review/1213701ebe344e698f738277c8b8831c?revisionId=1161de12983242d7b2d77b38ab2a512a
JavaScript @azure/arm-elasticsan https://apiview.dev/Assemblies/Review/b660eca2c7914c6e90b8f7f27c81284f?revisionId=a4e407c9c8304b8998ae33fbce70c4c7

@AzureRestAPISpecReview AzureRestAPISpecReview added the ReadyForApiTest <valid label in PR review process>add this label when swagger and service APIs are ready for test label Aug 22, 2024
This was referenced Aug 22, 2024
Draft
Draft
Draft
Draft
Draft
@sofiya09 sofiya09 added PublishToCustomers Acknowledgement the changes will be published to Azure customers. DoNotMerge <valid label in PR review process> use to hold merge after approval labels Aug 22, 2024
blueww
blueww reviewed Aug 22, 2024
View reviewed changes
.../elasticsan/resource-manager/Microsoft.ElasticSan/preview/2024-07-01-preview/elasticsan.json Outdated
},
{
"name": "purge",
"description": "Optional, used to purge soft deleted volume groups if set to true.",
Copy link
Member

@blueww blueww Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Does this mean it will be permanent deleted?
What's the default value?
Please update the description to include these info and make it more clear.

Besides that, if this is for permanent delete:

  1. Permanent deleted blob parameter is hidden in storage SDK/PSH/CLI. Should we also hide this parameter in PSH/CLI/SDK?
  2. Why not use aligned parameter name/type as blob permanent delete?

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the blob permanent delete parameter name?
For blob, is it just allowed through portal then?

Copy link
Member

@blueww blueww Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the blob permanent delete parameter name?

See the the link in my first comment.

For blob, is it just allowed through portal then?

Blob permanent delete is allowed in rest API, but not SDK/CLI/PSH. I can't find it in Azure Portal, also not blob swagger.
Please confirm with feature PM, if this should be open in SDK/PSH/CLI. If not, as SDK/PSH/CLI are all generated from swagger, we should consider if add this to swagger. (rest API doc is also generated from swagger)

@blueww
Copy link
Member

blueww commented Aug 22, 2024

It looks the SDK check only runs on latest stable API version, which has no change.
The SDK check actually not run on the new added preview API version.

@ArthurMa1978 Any idea how to make SDK validation on run on the new added preview API version in this PR?

@sofiya09 sofiya09 requested a review from PPrahalad August 22, 2024 11:44
@mentat9 mentat9 added the ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review label Aug 23, 2024
@razvanbadea-msft
Copy link
Contributor

Added sign off as the latest commits after label where updated descriptions based on previous comments.

@sofiya09 sofiya09 force-pushed the sofiya09-elasticsan-Microsoft.ElasticSan-2024-07-01-preview branch from ad2eb6c to 675e9dd Compare October 4, 2024 10:05
@openapi-pipeline-app openapi-pipeline-app
Copy link

PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

@sofiya09 sofiya09 enabled auto-merge (squash) October 7, 2024 10:40
@microsoft-github-policy-service microsoft-github-policy-service bot added no-recent-activity There has been no recent activity on this issue. and removed no-recent-activity There has been no recent activity on this issue. labels Nov 11, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity There has been no recent activity on this issue. label Dec 2, 2024
@sofiya09 sofiya09 removed the no-recent-activity There has been no recent activity on this issue. label Dec 3, 2024
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Microsoft.ElasticSan

@sofiya09 sofiya09 force-pushed the sofiya09-elasticsan-Microsoft.ElasticSan-2024-07-01-preview branch from 9abf7b2 to 0a69b34 Compare December 3, 2024 11:46
@openapi-pipeline-app openapi-pipeline-app
Copy link

PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity There has been no recent activity on this issue. label Dec 23, 2024
@sofiya09 sofiya09 removed the no-recent-activity There has been no recent activity on this issue. label Dec 30, 2024
@github-actions github-actions bot added the brownfield Brownfield services will soon be required to convert to TypeSpec. See https://aka.ms/azsdk/typespec. label Jan 16, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity There has been no recent activity on this issue. label Feb 3, 2025
auto-merge was automatically disabled February 17, 2025 13:02

Pull request was closed

@sofiya09 sofiya09 reopened this Feb 18, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the no-recent-activity There has been no recent activity on this issue. label Feb 18, 2025
blueww
blueww reviewed Mar 10, 2025
View reviewed changes
specification/elasticsan/resource-manager/readme.md
@@ -26,7 +26,7 @@ These are the global settings for the storagepool.

``` yaml
openapi-type: arm
tag: package-2024-05
tag: package-2024-07-01-preview
Copy link
Member

Choose a reason for hiding this comment

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

You should add a section for tag "package-2024-07-01-preview" in this file below.

blueww
blueww reviewed Mar 10, 2025
View reviewed changes
...cSan/preview/2024-07-01-preview/examples/ElasticSans_ListByResourceGroup_MinimumSet_Gen.json
],
"nextLink": "axgbgugzotxistmtabpjczrkqbfy"
}
"body": {}
Copy link
Member

Choose a reason for hiding this comment

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

why remove the body?

blueww
blueww reviewed Mar 10, 2025
View reviewed changes
...osoft.ElasticSan/preview/2024-07-01-preview/examples/VolumeGroups_Create_MinimumSet_Gen.json
}
],
"enforceDataIntegrityCheckForIscsi": true,
"deleteRetentionPolicy": {
Copy link
Member

Choose a reason for hiding this comment

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

With this example, as the input parameter for VG create not include deleteRetentionPolicy, Do you mean the "deleteRetentionPolicy" will be enabled with 14 retentionPeriodDays even user not set it in create VG? If not, the result should keep the minium output from server to make sure swagger check will cover the real server behavior with least server output.

blueww
blueww reviewed Mar 10, 2025
View reviewed changes
.../elasticsan/resource-manager/Microsoft.ElasticSan/preview/2024-07-01-preview/elasticsan.json
@@ -1018,6 +1061,69 @@
}
}
},
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ElasticSan/elasticSans/{elasticSanName}/volumegroups/{volumeGroupName}/volumes/{volumeName}/restore": {
Copy link
Member

Choose a reason for hiding this comment

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

Why only volume can be restore, but volumn group can't be restore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blueww blueww blueww left review comments

@mentat9 mentat9 mentat9 left review comments

@yifanz7 yifanz7 yifanz7 left review comments

@PPrahalad PPrahalad Awaiting requested review from PPrahalad

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
ARMReview ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review brownfield Brownfield services will soon be required to convert to TypeSpec. See https://aka.ms/azsdk/typespec. DoNotMerge <valid label in PR review process> use to hold merge after approval new-api-version PublishToCustomers Acknowledgement the changes will be published to Azure customers. ReadyForApiTest <valid label in PR review process>add this label when swagger and service APIs are ready for test resource-manager
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@sofiya09 @blueww @ramoka178 @razvanbadea-msft @azure-sdk @mentat9 @PPrahalad @yifanz7 @AzureRestAPISpecReview