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

Migrate BlobInventoryPolicy related cmdelts and Invoke-AzStorageAccountFailover to Track2 SDK #17

Merged
merged 3 commits into from
Jun 29, 2022

Conversation

yifanz0
Copy link

@yifanz0 yifanz0 commented Jun 20, 2022

Description

This PR includes migration of BlobInventoryPolicy related cmdlets and Invoke-AzStorageAccountFailover to Track2 SDK.
BlobInventoryPolicy related cmdlets include Get-AzStorageBlobInventoryPolicy, New-AzStorageBlobInventoryPolicyRule, Remove-AzStorageBlobInventoryPolicy, Set-AzStorageBlobInventoryPolicy

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@yifanz0 yifanz0 requested a review from blueww June 20, 2022 12:01
this.ResourceGroupName,
this.StorageAccountName);
Track2.BlobInventoryPolicyResource policy =
this.StorageClientTrack2.GetBlobInventoryPolicyResource(this.ResourceGroupName, this.StorageAccountName, "default").Get();
Copy link
Member

@yaxia yaxia Jun 21, 2022

Choose a reason for hiding this comment

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

"default"

It is recommended to define a static string and name it something like "DefaultInventoryPolicyName".

It would be easier for everyone to understand, review and maintain this parameter better.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Done

Copy link
Member

Choose a reason for hiding this comment

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

It'd be better if the static string in a place that can be shared by other files also use the it (Set/RemoveAzureStorageBlobInventoryPolicy.cs).

Copy link
Author

Choose a reason for hiding this comment

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

Sorry totally missed this comment. Will include this change in the next PR. Thanks a lot!


this.Enabled = policy.Policy.Enabled;
this.Destination = policy.Policy.Destination;
Copy link
Member

Choose a reason for hiding this comment

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

Might need to add a comments to add the destination which is on a later API version.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Enabled = this.Enabled,
//Destination = this.Destination,
Rules = invRules
Policy = policySchema,
Copy link
Member

Choose a reason for hiding this comment

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

It looks still still missing ID, Name, LastModified...

Copy link
Author

Choose a reason for hiding this comment

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

Seems like we cannot assign those values, including id, name, and lastModifiedOn, as the constructor is internal. I'll record this issue.

image

Copy link
Member

Choose a reason for hiding this comment

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

This might not be an issue, since this function is only used in set-AzstorageStorageBlobInventoryPolicy, to parse the input the policy (PSH object) to the policy (SDK object) to set.

And I can understand SDK not allow to set them, since they should only be read from server, but not set by customer.

In a word, if PSH code works with it, I believe this will not be an issue.

@yifanz0 yifanz0 merged commit cc4a708 into wastoresh:track2review Jun 29, 2022
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.

4 participants