-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
NFSAAS-2235 update to RP V3.5 (#7) #5660
Conversation
Again. With clean master.
Again. With clean master.
@leonardbf props for the clean commit history 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great for the most part
Version 0.9.1-preview relates to NetApp Files (ANF) RP R3 | ||
- Addition of export policy for volumes | ||
- Addition of active directory for accounts | ||
Version 0.9.0-preview relates to NetApp Files (ANF) RP R2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You don't need to explain what previous versions of the package do.,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll keep this in mind but would like to leave it here this time.
@@ -35,5 +35,5 @@ | |||
// You can specify all the values or you can default the Build and Revision Numbers | |||
// by using the '*' as shown below: | |||
// [assembly: AssemblyVersion("1.0.*")] | |||
[assembly: AssemblyVersion("1.0.0.0")] | |||
[assembly: AssemblyFileVersion("1.0.0.0")] | |||
[assembly: AssemblyVersion("1.1.0.0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test project versions are always set to 1.0.0.0
and never modified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Updated.
@@ -143,8 +143,15 @@ public void PatchSnapshot() | |||
Tags = dict | |||
}; | |||
|
|||
var resource = netAppMgmtClient.Snapshots.Update(snapshotPatch, ResourceUtils.resourceGroup, ResourceUtils.accountName1, ResourceUtils.poolName1, ResourceUtils.volumeName1, ResourceUtils.snapshotName1); | |||
Assert.True(resource.Tags.ToString().Contains("Tag1") && resource.Tags.ToString().Contains("Value1")); | |||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Consider using Assert.Throws
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. If it's acceptable I will look at this for the next version which is gated GA in the next few weeks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM will merge when CI builds pass
@azuresdkci test this please |
@leonardbf please join the azure github org here in order to kick off this build |
* NFSAAS-2235 update to RP V3.5 Again. With clean master. * NFSAAS-2235 update to RP V3.5 (Azure#7) Again. With clean master. * NFSAAS-2235 update from review comments
Again. With clean master.
Update SDK to version R3.5 of RP.
Includes active directory and export policy.
Still in preview - breaking changes are not an issue
Swagger:
https://github.com/leonardbf/azure-rest-api-specs/tree/master/specification/netapp/resource-manager
Last swagger PR:
Azure/azure-rest-api-specs#5446
This replaces #5646
(NetApp internal ticket NFSAAS-2235)