optimize Get-NsxSecurityTagAssignment per #541 #545
Conversation
|
@mtboren, VMware has approved your signed contributor license agreement. |
|
LGTM! Thanks for a quality PR. Any change you could write some simple pester tests for it so we can merge asap? |
|
Super. Welcome, glad to help make things even better! Yes, there is about a 100% chance. I have now done so, and have done a Let me know what else we need in order to wrap up this addition. And, an aside: in creating those tests, I saw a couple of other opportunities for iterating on the module and continuing to make it better still -- the credentials gathering in the |
|
Great work - thanks @mtboren ! I notice the tests expect security tags to pre-exist, can you add creation (and cleanup) to the before all/after all. Tests need to be able to run on a (mostly) vanilla deployment. |
|
Thanks. I certainly can add such things. Though, does that mean, too, that the tests will also need to take care of creating and cleaning up a subject VM and a target security tag assignment, if the tests are to assume that there may be zero tag assignments in the vanilla deployment, as well? |
|
Correct. There are examples in some of the other tests like the DFW ones that you could copy. |
|
Got it, and, done. The PR's "Files changed" view reflects the overall things that I have added for both the function update and the tests. Let me know if there are any other things to address before a merge. Aside: I am not too familiar w/ the effects of the Cheers |
| Import-Module $pnsxmodule | ||
| $script:DefaultNsxConnection = Connect-NsxServer -vCenterServer $PNSXTestVC -NsxServerHint $PNSXTestNSX -Credential $PNSXTestDefViCred -ViWarningAction "Ignore" | ||
|
|
||
| ## GUID to use as suffix for some temporary vSphere/NSX objects, so as to prevent any naming conflict with existing objects | ||
| $strSuffixGuid = [System.Guid]::NewGuid().Guid.Replace("-","") |
There was a problem hiding this comment.
Only use powernsx_ (like other test suite?)
There was a problem hiding this comment.
Howdy- We could, if we want to leave a chance for object naming collision (although, granted, it would still be fairly improbable that a collision would happen even with just the prefix). I added the GUID suffix to drive the possibility of naming collision closer to zero. If a lower chance of collision is better, we could update other test suites at some point to also employ a GUID suffix.
| @@ -28,20 +28,38 @@ Describe -Name "SecurityTagAssignment" -Tag "Get" -Fixture { | |||
| #We load the mod and establish connection to NSX Manager here. | |||
|
|
|||
| #Put any setup tasks in here that are required to perform your tests. Typical defaults: | |||
| Write-Verbose -Verbose "Performing setup tasks for SecurityTag tests" | |||
There was a problem hiding this comment.
really need add verbose ?
There was a problem hiding this comment.
Greets- Need verbose? Possibly not. The reason for Write-Verbose -Verbose here, though, is to deliver a verbose message that had previously been delivered via Write-Warning (a message that was not a warning). And, by "previously delivered", I mean that was in the test code from this module off of which I based this new test file.
So, if we want a verbose message, then, yes, we keep this. If not, of course, let's whack it.
| $script:oVMHostToUse = Get-VMHost -State Connected | Get-Random | ||
| Write-Verbose -Verbose "Using VMHost '$oVMHostToUse' for temporary VM creation" | ||
| ## get the datastore mounted on this VMHost as readWrite (where .ExtensionData.Host.Key is this VMHost's ID, and where .ExtensionData.Host.MountInfo.AccessMode is "readWrite") and with the most FreespaceGB, on which to create some test VMs | ||
| $script:oDStoreToUse = $oVMHostToUse | Get-Datastore | Where-Object {$_.ExtensionData.Host | Where-Object {$_.Key -eq $oVMHostToUse.Id -and ($_.MountInfo.AccessMode -eq "readWrite")}} | Sort-Object -Property FreespaceGB -Descending | Select-Object -First 1 |
There was a problem hiding this comment.
use code from other test about datastore ?
There was a problem hiding this comment.
Howdy- we could. However, I wrote this "get a suitable datastore" code to help ensure success in a greater range of scenarios. For example, the $cl | get-datastore | select -first 1 code from another test suite failed in my testing environment as the first datastore happened to be an NFS datastore that is mounted readOnly (and, so, VM deployments to it failed, of course).
There is also probably more room for improvement here (this code just gets the readWrite datastore with the most free space on the given VMHost, but does not check for, "is the amount of free space sufficient?"). But, maybe that's for another day
There was a problem hiding this comment.
Yes, need some improvement..
like a option to choice the datastore for the first run (and store) like also for cluster...
There was a problem hiding this comment.
$true -- yes, or even parameters to the given test so that one can specify at invocation time the datastore/cluster/VMHost/etc. object(s) to use for the given tests if they want to use some specific resources..
| #We kill the connection to NSX Manager here. | ||
| # AfterAll block runs _once_ at completion of invocation regardless of number of tests/contexts/describes. | ||
| # Clean up anything you create in here. Be forceful - you want to leave the test env as you found it as much as is possible. | ||
| # We kill the connection to NSX Manager here. | ||
|
|
||
| if ($pause) {Read-Host "pausing" } |
There was a problem hiding this comment.
Greets- seems fine to me -- I only kept it here as it was in the "template" test code upon which I based this overall test file. Let me know, and I'll remove it.
|
Thanks @mtboren - I for one certainly have no issue with you making code more robust than our existing!!! At first glance the tests you've added are precisely whats required. I will take a more detailed look later today when I get a free moment. |
|
jenkins test this please |
|
Tests Failed |
|
@mtboren - great work - your tests pass (we have a recurring issue with a few of our security policy tests that caused the failure that are unrelated to your changes). Thanks for a quality PR! |
|
Greets, @nmbradford -- thanks much! I am enjoying the module, and am glad to have to opportunity to contribute. And, thanks for the additional info. See you on the next enhancement |
Updated the
VirtualMachineparameterset code for getting assigned NSX Security Tags by VM:/api/2.0/services/securitytags/vm/{vmId}that came with NSX v6.3.0$nullif no tags assigned to given VM)