Making -ResourceGroupName parameter optional for Get-AzureRmSqlServer cmdlet #4188

akromm-zz · 2017-06-23T17:26:07Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

akromm-zz · 2017-06-23T17:26:37Z

#635

jaredmoo

Test issue is the only blocker for me, other comments are just suggestions

jaredmoo · 2017-06-23T17:36:06Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/ServerCrudTests.ps1

+
+		# Test getting all servers in all resource groups
+		$all2 = Get-AzureRmSqlServer
+		Assert-AreEqual 3 $all2.Count


This will fail if I run it in my subscription that already has some sql servers unrelated to PS test. Can you change this so that it checks that the list contains the 3 previous servers?

Sure, I can do that.

jaredmoo · 2017-06-23T17:37:29Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+    /// <summary>
+    /// The base class for all Azure Sql cmdlets
+    /// </summary>
+    public abstract class AzureSqlCmdletBaseBase<M, A> : AzureRMCmdlet


haha okay ;)

My creativity shining through :)

jaredmoo · 2017-06-23T17:38:52Z

src/ResourceManager/Sql/Commands.Sql/Server/Cmdlet/GetAzureSqlServer.cs

    {
        /// <summary>
+        /// Gets or sets the name of the resource group to use.
+        /// </summary>
+        [Parameter(Mandatory = false,


can you or some comment explaining why BaseBase when you have ResourceGroup param? I understand that's it's for Mandatory=false when looking at this change in isolation, but it won't be obvious when reading this code later

added a comment.

jaredmoo · 2017-06-23T17:42:08Z

src/ResourceManager/Sql/Commands.Sql/Server/Cmdlet/GetAzureSqlServer.cs

            else
            {
-                results = ModelAdapter.GetServers(this.ResourceGroupName);
+                throw new PSArgumentException("When specifying the serverName parameter the ResourceGroup parameter must also be used");


Can you split this into 2 ParamGroups so that it's enforced by powershell logic instead of custom code?
Group 1: Get-AzureRmSqlServer
Group 2: Get-AzureRmSqlServer -ResourceGroupName [- ServerName]

I might be wrong here, not sure if "no parameters" is possible to specify as a parameter set.

I thought a bit about this, but wasn't sure how to achieve it. According to MSDN-Parameter Set Requirements a parameter set must have at least 1 parameter.

Another option is to allow specifying the ServerName without the resourceGroupName and then just retrieve the list of all servers and filter it down to just the one with the matching name (potentially bad perf)? The underlying API doesn't support retrieving a specific server without the resource group name.

jaredmoo · 2017-06-23T23:26:33Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/ServerCrudTests.ps1

-		Assert-AreEqual 3 $all2.Count
+
+		# It is possible that there were existing servers in the subscription when the test was recorded, so make sure
+		# that the servers that we created are retrieved and ignore the other ones.


This works, but it's a bit confusing to read. Does something like this work?
($server1, $server2, $server3) | foreach { Assert-True $_.ServerName -in $all2.ServerName }

Interestingly that does work. I wasn't aware that trying to access a property on an array would return the property value for each object in the array.

shahabhijeet · 2017-06-25T02:59:34Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBase.cs

@@ -59,87 +36,5 @@ protected virtual string GetResourceId(M model)
            HelpMessage = "The name of the resource group.")]
        [ValidateNotNullOrEmpty]
        public string ResourceGroupName { get; set; }


@akromm changing ResourceGroupName from mandatory to non-mandatory is a breaking change?
Any reason you want to do this now?
What scenarios are you trying to fix? or is it just ease for user?

@shahabhijeet is relaxing the requirements of parameters a breaking change? I'm doing this change because there have been multiple github issues requesting it. eg: #635

@akromm @shahabhijeet changing a parameter from mandatory to non-mandatory is not a breaking change; however, the other way around is a breaking change

@cormacpayne thanks for correcting. I wrote that comment and then a day later realized that is not right what I wrote, but by that time I was not able to come back to GitHub to correct it.

shahabhijeet · 2017-06-25T03:07:33Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+    /// <summary>
+    /// The base class for all Azure Sql cmdlets
+    /// </summary>
+    public abstract class AzureSqlCmdletBaseBase<M, A> : AzureRMCmdlet


@akromm any reason there aren't any constraints for M, A?

shahabhijeet · 2017-06-25T03:12:13Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+
+        protected virtual string GetResourceId(M model)
+        {
+            var serverProperty = model.GetType().GetProperty("ServerName");


@akromm maybe I am missing to see how this will work when model is of type IEnumerable

@shahabhijeet I have updated the method to work with IEnumerable and other single entity types. I have also added a corresponding unittest to ensure it works.

…ver cmdlet.

…model and IEnumerable entity lists.

shahabhijeet · 2017-06-27T16:54:25Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+
+        protected virtual string GetResourceId(M model)
+        {
+            object m = null;


@akromm why m is not of type M? Why object?

So that overriding methods in subclasses can use the specific type

@shahabhijeet I think the right thing to do here is make the method Abstract and force each cmdlet to implement it.

To answer your question. It is of type object because M can be IEnumerable and I want an object of type T not M.

shahabhijeet · 2017-06-27T17:02:40Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+
+            if (m != null)
+            {
+                var serverProperty = m.GetType().GetProperty("ServerName");


@akromm if you make M to some constraint class, you don't have to deal with late bound objects.

shahabhijeet · 2017-06-27T17:02:41Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+        {
+            object m = null;
+
+            if (typeof(IEnumerable).IsAssignableFrom(model.GetType()))


@akromm you have now covered two scenarios, IEnumerable and M.
Are you sure those are the only two types your consumers are going to pass in?
That is the reason you need to define constraints on M.
And then you should have however many overloads GetResourceId e.g.
GetResourceId(IEumerable listOfModels)
GetResourceId(ICollection modelCollection) just to make my point, but overloads can handle IEnumerable.
Having generic class with no constraint is setting yourself for runtime errors.

shahabhijeet · 2017-06-27T17:05:23Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+        {
+        }
+
+        protected virtual string GetResourceId(M model)


@akromm plus anyone who overrides and have their own implementation of this method will have to make sure they have safety code for all possible types of M (similar to what you have)

@shahabhijeet This would be overridden per cmdlet type (Database, Server, DataMasking, ...) and as such the type M will be well known and there shouldn't be the complex logic I have.

shahabhijeet · 2017-06-27T17:06:30Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+        /// </summary>
+        /// <param name="model">The about to be written model object</param>
+        /// <returns>The prepared object to be written out</returns>
+        protected virtual object TransformModelToOutputObject(M model)


@akromm any generic class with late bound return types defeats the purpose of your generic class.

@shahabhijeet
This is only used in a couple places.
Get-AzureRmSqlDatabaseDataMaskingRule cmdlet uses it to filter the results before showing the user.
SqlDatabaseDataMaskingRuleCmdletBase also uses it to do client side filtering of the results.
New-AzureRmSqlDatabase Actually uses it to modify the response slightly.

That being said, all this code is pre-existing code that already existed. I'm just moving it to a new file. I might have a different way to achieve my main goal without moving this stuff around.

shahabhijeet · 2017-06-27T17:08:34Z

src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBaseBase.cs

+        /// </summary>
+        public override void ExecuteCmdlet()
+        {
+            ModelAdapter = InitModelAdapter(DefaultProfile.DefaultContext.Subscription);


@akromm can you point me where is InitModelAdapter implemented?

@shahabhijeet InitModelAdapter is implemented in each individual cmdlet. Database Cmdlet base example

Making ResourceGroupName parameter virtual rather than creating a new base class.

akromm-zz · 2017-06-29T20:16:51Z

@cormacpayne @shahabhijeet @jaredmoo Finally got the tests to pass (issues with dependencies and NuGet package versions). Can I please get this reviewed and merged now :)

Thanks.

restructured how the changes are being done.

shahabhijeet · 2017-06-30T17:44:20Z

On-Demand
http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1724/

cormacpayne · 2017-06-30T18:58:55Z

@azuresdkci test this please

shahabhijeet · 2017-06-30T20:33:14Z

@azuresdkci test this please

msftclas added the cla-not-required label Jun 23, 2017

akromm-zz requested a review from jaredmoo June 23, 2017 17:26

azuresdkci added the needs-review label Jun 23, 2017

azuresdkci assigned shahabhijeet Jun 23, 2017

jaredmoo suggested changes Jun 23, 2017

View reviewed changes

jaredmoo previously approved these changes Jun 23, 2017

View reviewed changes

akromm-zz dismissed jaredmoo’s stale review via 5c163d1 June 23, 2017 23:39

shahabhijeet suggested changes Jun 25, 2017

View reviewed changes

akromm-zz added 7 commits June 26, 2017 16:23

Making -ResourceGroupName an optional parameter for Get-AzureRMSqlSer…

d5f2e69

…ver cmdlet.

Recording and fixing tests/code.

a0a7986

Setting some tests to CheckIn type.

c5e8cf5

Making test more robust and adding some clarification comments.

03920c8

Simplifying test expression

5766454

Updating/Fixing GetResourceId method to work with both single entity …

92e0d2a

…model and IEnumerable entity lists.

Fixing broken tests.

c9f08c3

akromm-zz force-pushed the preview branch from c2d84a2 to c9f08c3 Compare June 26, 2017 23:24

shahabhijeet previously requested changes Jun 27, 2017

View reviewed changes

Changing approach to achieving my goal.

3ad36f5

Making ResourceGroupName parameter virtual rather than creating a new base class.

jaredmoo previously approved these changes Jun 28, 2017

View reviewed changes

Reverting some package changes.

c6e2cd6

akromm-zz dismissed jaredmoo’s stale review via c6e2cd6 June 29, 2017 18:07

jaredmoo approved these changes Jun 30, 2017

View reviewed changes

shahabhijeet changed the base branch from preview to release-4.2.0 June 30, 2017 17:41

shahabhijeet merged commit a58a237 into Azure:release-4.2.0 Jun 30, 2017

akromm-zz mentioned this pull request Jun 30, 2017

ARM 0.9.5 :: Get-AzureSqlServer requires -ResourceGroupName parameter #635

Closed

Making -ResourceGroupName parameter optional for Get-AzureRmSqlServer cmdlet #4188

Making -ResourceGroupName parameter optional for Get-AzureRmSqlServer cmdlet #4188

Conversation

akromm-zz commented Jun 23, 2017 • edited by azuresdkci

Description

akromm-zz commented Jun 23, 2017

jaredmoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akromm-zz Jun 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akromm-zz commented Jun 29, 2017

shahabhijeet commented Jun 30, 2017

cormacpayne commented Jun 30, 2017

shahabhijeet commented Jun 30, 2017

akromm-zz commented Jun 23, 2017 •

edited by azuresdkci

akromm-zz Jun 27, 2017 •

edited