Make Remove-TestResource invoke custom script prior to deletion of the resource group

[azabbasi]

Since there are certain actions that might need to be taken prior to deletion of test resources, we will invoke custom scripts in the service directory named : remove-test-resources-pre.ps1

[weshaggard]

PTAL @danieljurek @heaths

[heaths]

This check is unnecessary. Put [ValidateIsNotNullOrEmpty()] on the parameter as well and just use line 123. IT's better to do most validation in PowerShell parameters because it will validate and return correct errors.

[heaths]

I noticed you pushed another commit, but please make this change as well. We want PowerShell to validate as much as possible - not the script.

[danieljurek]

The advantage of allowing an empty value here is that invocation would be more simple for the contributor... but only if they coincidentally didn't have one of these pre- scripts (in the majority of cases today you don't need to run a pre- script for resource removal).

If we do validation here we'll need to update references to remove-test-resources.yml in each repo before the PRs that this merge opens get merged or we'll be broken.

[heaths]

If they don't want to specify it, don't specify the parameter. ValidateNullOrEmpty only applies if they specify the parameter. That's the norm with well-written PowerShell scripts.

[danieljurek]

Thanks for clarifying... I think the correct syntax would take the form of:

...

[ValidateNotNullOrEmpty()]
[string] $ServiceDirectory,

...

if ($ServiceDirectory) { 
  $root = [System.IO.Path]::Combine("$PSScriptRoot/../../../sdk", $ServiceDirectory) | Resolve-Path
  ...
}

I'll start updating the template invocations ahead of this change.

[heaths]

You still need the [Parameter()] attribute.

[weshaggard]

I actually think we should not put ValidateNotNullOrEmpty as that will be a breaking change for all the consumers of our eng\common. So at least initially I'd stick with what we is here right now. We can later decide to fix up all the places and then we can add this if we feel it is interesting.

The more and more dependencies we get on the stuff under eng/common the more we are going to have to be careful about making breaking changes as it will become very difficult to keep things working in all the places if we don't.

[heaths]

It's not a breaking change. It only applies if you specify the -ServiceDirectory. If they didn't (it didn't exist), there's no harm. This is how PowerShell works.

[weshaggard]

Combining it with the change to the template which pass empty by default will break pipelines until we can get all the users updated to pass something not-empty. We can come back and fix this later once we have updated all the usages.

[danieljurek]

PRs to add ServiceDirectory to usages of remove-test-resources.yml:

• Supply ServiceDirectory to uses of remove-test-resources.yml azure-sdk-for-java#10591
• Add ServiceDirectory parameter to usages of remove-tests-resources.yml azure-sdk-for-js#8597
• add ServiceDirectory to usages of remove-test-resources.yml azure-sdk-for-net#11666
• Add ServiceDirectory to usage of remove-test-resources.yml azure-sdk-for-python#11117

[danieljurek]

This should also include an update to the "In CI" section of https://github.com/Azure/azure-sdk-tools/blob/master/eng/common/TestResources/README.md

It should add the ServiceDirectory parameter to that documentation

...
# Run tests

- template: /eng/common/TestResources/remove-test-resources.yml 
  parameters:
    ServiceDirectory: '${{ parameters.ServiceDirectory }}'

[weshaggard]

Thanks @azabbasi for adding this extension point.

[weshaggard]

@azabbasi when this change makes it into your working branch you may need to handle the merge carefully, especially if there are any differences between what you have here and what you committed in your branch.