Added support for deleting Shares that have leased snapshots #15889
Conversation
seanmcc-msft
requested review from
KrzysztofCwalina,
amishra-dev,
amnguye,
kasobol-msft and
tg-msft
as code owners
ghost
added
the
| else if ((includeSnapshots == null || includeSnapshots == true) && shareSnapshotsDeleteOption == null) | ||
| { | ||
| deleteSnapshotsOption = includeSnapshots ? DeleteSnapshotsOptionType.Include : (DeleteSnapshotsOptionType?)null; | ||
| shareSnapshotsDeleteOption = ShareSnapshotsDeleteOption.IncludeLeased; |
There was a problem hiding this comment.
Previously, we defaulted to deleting Snapshots with the Share, unless the user specified otherwise. If a share had snapshots, the call would fail if x-ms-delete-snapshots wasn't set to "include".
Now, it's possible to take a lease on a Share Snapshot. The delete call will fail if a Share has a Snapshot with a lease, unless x-ms-delete-snapshots = "include-leased"
There was a problem hiding this comment.
hmm. is leasing snapshot introduced in the same service release or earlier ?
There was a problem hiding this comment.
It was introduced in the newest service release.
There was a problem hiding this comment.
so what happens today with leased share snapshots on default parameters? does it fail?
taking that aside, I think that taking lease on snapshot means "please don't touch it" so if someone else is attempting to delete it they should get a warning and if they still want to break/override lease that should be explicit action on their side (i.e. they should specify explicit option to do that.).
There was a problem hiding this comment.
Previously, you couldn't lease shares or share snapshots. If you attempted to delete a share that had snapshots, the request would fail with a 400. The previous behavior in the SDK was to delete snapshots by default.
Today, the previous behavior stands, but has been extended. Now, you can take a lease on a share snapshot. If you don't specify "IncludeLeased", the request will fail with a 400. To keep the previous SDK behavior, I think we need to switch to sending "IncludeLeased" by default. Passing "Include" by default would also make sense.
There was a problem hiding this comment.
Passing "Include" by default as we previously did might be the best option.
There was a problem hiding this comment.
So people who never took lease on snapshot won't tell a difference between default "Include" and "IncludeLeased.
But if we default to "IncludeLeased" we might get some unhappy users who's data got deleted unintentionally.
Afaik for other API/use cases where lease is involved user is required to call some specific action/parameter to break the lease - I think that's more important than making sure that this api never throws 400 on defaults.
I'd default to "Include"
There was a problem hiding this comment.
I think that's more important than making sure that this api never throws 400 on defaults.
The one downside of defaulting to "include" is we will throw a 400 with defaults if there are any leased share snapshots associated with the share.
There was a problem hiding this comment.
I think that's desired. Otherwise why someone would take lease?
There was a problem hiding this comment.
Changed back to "include"
| /// <summary> | ||
| /// Specifies the option include to delete the base share and all of its snapshots. | ||
| /// </summary> | ||
| public enum ShareSnapshotsDeleteOption | ||
| { | ||
| /// <summary> | ||
| /// include | ||
| /// </summary> | ||
| Include, | ||
|
|
||
| /// <summary> | ||
| /// include-leased | ||
| /// </summary> | ||
| IncludeLeased | ||
| } |
There was a problem hiding this comment.
Is "IncludeLeased" a super set of "Include" and "Leased" or is it just leased. I.e. should I say IncludeLeased or Include | IncludeLeased to delete everything? I find this naming little bit confusing, at least we should explain in the doc string what does it do.
There was a problem hiding this comment.
IncludeLeased is a super set, see the new unit test.
There was a problem hiding this comment.
maybe IncludeWithLeased then or something like that.
There was a problem hiding this comment.
We will have to create a hand-written enum of we want to change this value.
There was a problem hiding this comment.
If we can add verbose docs strings without creating hand-written type then let's leave it. Otherwise (if adding docs would force type creation) let's rename.
There was a problem hiding this comment.
We can't update the xml comments for the individual values, but I am able to change the xml comment for the enum itself. Would that work?
There was a problem hiding this comment.
:/ I'd go with hand crafted type in that case. So that we can provide experience like this:
We can also file feature request for generator to be able to customize enum docs.
| } | ||
| else | ||
| // This is for backwards compatibility. Perviously, ShareClient.Delete() took a bool includSnapshots parameter.s | ||
| else if ((includeSnapshots == null || includeSnapshots == true) && shareSnapshotsDeleteOption == null) |
There was a problem hiding this comment.
Should we throw if both boolean and enum are provided?
There was a problem hiding this comment.
nvm I guess this is not possible
seanmcc-msft
deleted the
feature/storage/shareDeleteIncludeLeasedShapshots
branch
3 participants
No description provided.