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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Jun 30, 2017
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d5f2e69
Making -ResourceGroupName an optional parameter for Get-AzureRMSqlSer…
akromm-zz Jun 15, 2017
a0a7986
Recording and fixing tests/code.
akromm-zz Jun 16, 2017
c5e8cf5
Setting some tests to CheckIn type.
akromm-zz Jun 23, 2017
03920c8
Making test more robust and adding some clarification comments.
akromm-zz Jun 23, 2017
5766454
Simplifying test expression
akromm-zz Jun 23, 2017
92e0d2a
Updating/Fixing GetResourceId method to work with both single entity …
akromm-zz Jun 26, 2017
c9f08c3
Fixing broken tests.
akromm-zz Jun 26, 2017
3ad36f5
Changing approach to achieving my goal.
akromm-zz Jun 27, 2017
c6e2cd6
Reverting some package changes.
akromm-zz Jun 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/ResourceManager/Sql/Commands.Sql.Test/Commands.Sql.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,9 @@
<Reference Include="Microsoft.Azure.Test.Framework">
<HintPath>..\..\..\packages\Microsoft.Azure.Test.Framework.1.0.6179.26854-prerelease\lib\net45\Microsoft.Azure.Test.Framework.dll</HintPath>
</Reference>
<Reference Include="Microsoft.Azure.Test.HttpRecorder">
<HintPath>..\..\..\packages\Microsoft.Azure.Test.HttpRecorder.1.6.7-preview\lib\net45\Microsoft.Azure.Test.HttpRecorder.dll</HintPath>
<Reference Include="Microsoft.Azure.Test.HttpRecorder, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\..\..\packages\Microsoft.Azure.Test.HttpRecorder.1.8.1\lib\net452\Microsoft.Azure.Test.HttpRecorder.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Microsoft.Data.Edm">
<SpecificVersion>False</SpecificVersion>
Expand Down Expand Up @@ -123,11 +124,12 @@
<Private>True</Private>
</Reference>
<Reference Include="Microsoft.Rest.ClientRuntime.Azure.Authentication, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\..\..\packages\Microsoft.Rest.ClientRuntime.Azure.Authentication.2.2.9-preview\lib\net45\Microsoft.Rest.ClientRuntime.Azure.Authentication.dll</HintPath>
<HintPath>..\..\..\packages\Microsoft.Rest.ClientRuntime.Azure.Authentication.2.3.0\lib\net452\Microsoft.Rest.ClientRuntime.Azure.Authentication.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Microsoft.Rest.ClientRuntime.Azure.TestFramework">
<HintPath>..\..\..\packages\Microsoft.Rest.ClientRuntime.Azure.TestFramework.1.5.0-preview\lib\net45\Microsoft.Rest.ClientRuntime.Azure.TestFramework.dll</HintPath>
<Reference Include="Microsoft.Rest.ClientRuntime.Azure.TestFramework, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\..\..\packages\Microsoft.Rest.ClientRuntime.Azure.TestFramework.1.7.2\lib\net452\Microsoft.Rest.ClientRuntime.Azure.TestFramework.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Microsoft.Threading.Tasks, Version=1.0.12.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
<SpecificVersion>False</SpecificVersion>
Expand Down
4 changes: 2 additions & 2 deletions src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/ImportExportTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ public ImportExportTests(ITestOutputHelper output) : base(output)
}

[Fact]
[Trait(Category.AcceptanceType, Category.Sql)]
[Trait(Category.AcceptanceType, Category.CheckIn)]
public void TestExportDatabase()
{
RunPowerShellTest("Test-ExportDatabase");
}

[Fact]
[Trait(Category.AcceptanceType, Category.Sql)]
[Trait(Category.AcceptanceType, Category.CheckIn)]
public void TestImportDatabase()
{
RunPowerShellTest("Test-ImportDatabase");
Expand Down
16 changes: 12 additions & 4 deletions src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/ServerCrudTests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,29 +85,37 @@ function Test-GetServer
{
# Setup
$rg = Create-ResourceGroupForTest
$rg1 = Create-ResourceGroupForTest
$server1 = Create-ServerForTest $rg
$server2 = Create-ServerForTest $rg
$server3 = Create-ServerForTest $rg1

try
{
# Test using parameters
$resp1 = Get-AzureRmSqlServer -ResourceGroupName $rg.ResourceGroupName -ServerName $server1.ServerName
Assert-AreEqual $server1.ServerName $resp1.ServerName
Assert-AreEqual $server1.ServerVersion $resp1.ServerVersion
Assert-AreEqual $server1.SqlAdministratorLogin $resp1.SqlAdministratorLogin

# Test piping
$resp2 = $server2 | Get-AzureRmSqlServer
Assert-AreEqual $server2.ServerName $resp2.ServerName
Assert-AreEqual $server2.ServerVersion $resp2.ServerVersion
Assert-AreEqual $server2.SqlAdministratorLogin $resp2.SqlAdministratorLogin

$all = Get-AzureRmSqlServer -ResourceGroupName $rg.ResourceGroupName
Assert-AreEqual $all.Count 2
Assert-AreEqual 2 $all.Count

# Test getting all servers in all resource groups
$all2 = Get-AzureRmSqlServer

# 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.
Copy link
Member

Choose a reason for hiding this comment

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

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 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

($server1, $server2, $server3) | ForEach-Object { Assert-True {$_.ServerName -in $all2.ServerName} }
}
finally
{
Remove-ResourceGroupForTest $rg
Remove-ResourceGroupForTest $rg1
}
}

Expand Down
18,521 changes: 7,582 additions & 10,939 deletions ...ecords/Microsoft.Azure.Commands.Sql.Test.ScenarioTests.ServerCrudTests/TestServerGet.json
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions src/ResourceManager/Sql/Commands.Sql.Test/UnitTests/AzureSqlCmdletBaseAttributeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using Microsoft.Azure.Commands.Sql.Common;
using Microsoft.Azure.Commands.Sql.Test.Utilities;
using Microsoft.Azure.ServiceManagemenet.Common.Models;
using Microsoft.WindowsAzure.Commands.ScenarioTest;
using Xunit;
using Xunit.Abstractions;

Expand All @@ -28,12 +29,14 @@ public AzureSqlCmdletBaseAttributeTests(ITestOutputHelper output)
}

[Fact]
[Trait(Category.AcceptanceType, Category.CheckIn)]
public void TestAzureSqlCmdletBaseAttributes()
{
UnitTestHelper.CheckCmdletParameterAttributes(typeof(AzureSqlCmdletBase<object, object>), "ResourceGroupName", true, true);
}

[Fact]
[Trait(Category.AcceptanceType, Category.CheckIn)]
public void TestAzureSqlDatabaseCmdletBaseAttributes()
{
UnitTestHelper.CheckCmdletParameterAttributes(typeof(AzureSqlDatabaseCmdletBase<object, object>), "DatabaseName", true, true);
Expand Down
4 changes: 2 additions & 2 deletions src/ResourceManager/Sql/Commands.Sql.Test/packages.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<package id="Microsoft.Azure.Management.Sql" version="1.5.0-preview" targetFramework="net452" />
<package id="Microsoft.Azure.Management.Storage" version="2.4.0-preview" targetFramework="net45" />
<package id="Microsoft.Azure.Test.Framework" version="1.0.6179.26854-prerelease" targetFramework="net45" />
<package id="Microsoft.Azure.Test.HttpRecorder" version="1.6.7-preview" targetFramework="net45" />
<package id="Microsoft.Azure.Test.HttpRecorder" version="1.8.1" targetFramework="net452" />
<package id="Microsoft.Bcl" version="1.1.9" targetFramework="net45" />
<package id="Microsoft.Bcl.Async" version="1.0.168" targetFramework="net45" />
<package id="Microsoft.Bcl.Build" version="1.0.14" targetFramework="net45" />
Expand All @@ -27,7 +27,7 @@
<package id="Microsoft.WindowsAzure.Management" version="4.1.1" targetFramework="net45" />
<package id="Microsoft.WindowsAzure.Management.Storage" version="5.1.1" targetFramework="net45" />
<package id="Moq" version="4.2.1510.2205" targetFramework="net45" />
<package id="Newtonsoft.Json" version="6.0.8" targetFramework="net45" />
<package id="Newtonsoft.Json" version="9.0.1" targetFramework="net452" />
<package id="System.Spatial" version="5.6.4" targetFramework="net45" />
<package id="WindowsAzure.Storage" version="5.0.0" targetFramework="net45" />
<package id="xunit" version="2.1.0" targetFramework="net45" />
Expand Down
2 changes: 1 addition & 1 deletion src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ protected virtual string GetResourceId(M model)
Position = 0,
HelpMessage = "The name of the resource group.")]
[ValidateNotNullOrEmpty]
public string ResourceGroupName { get; set; }
public virtual string ResourceGroupName { get; set; }

/// <summary>
/// The ModelAdapter object used by this cmdlet
Expand Down
2 changes: 1 addition & 1 deletion ...sourceManager/Sql/Commands.Sql/Replication/Services/AzureSqlDatabaseReplicationAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ internal AzureReplicationLinkModel CreateLink(string resourceGroupName, string s
private void CheckPartnerResourceGroupValid(string partnerResourceGroupName)
{
// checking if the resource group is valid as a partner resource group
ServerCommunicator.List(partnerResourceGroupName);
ServerCommunicator.ListByResourceGroup(partnerResourceGroupName);
}

/// <summary>
Expand Down
24 changes: 21 additions & 3 deletions src/ResourceManager/Sql/Commands.Sql/Server/Cmdlet/GetAzureSqlServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ namespace Microsoft.Azure.Commands.Sql.Server.Cmdlet
[Cmdlet(VerbsCommon.Get, "AzureRmSqlServer", ConfirmImpact = ConfirmImpact.None, SupportsShouldProcess = true)]
public class GetAzureSqlServer : AzureSqlServerCmdletBase, IModuleAssemblyInitializer
{
/// <summary>
/// Gets or sets the name of the resource group to use.
/// </summary>
[Parameter(Mandatory = false,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment.

ValueFromPipelineByPropertyName = true,
Position = 0,
HelpMessage = "The name of the resource group.")]
[ValidateNotNullOrEmpty]
public override string ResourceGroupName { get; set; }

/// <summary>
/// Gets or sets the name of the database server to use.
/// </summary>
Expand All @@ -38,7 +48,7 @@ public class GetAzureSqlServer : AzureSqlServerCmdletBase, IModuleAssemblyInitia
[Alias("Name")]
[ValidateNotNullOrEmpty]
public string ServerName { get; set; }

/// <summary>
/// Gets a server from the service.
/// </summary>
Expand All @@ -47,14 +57,22 @@ protected override IEnumerable<AzureSqlServerModel> GetEntity()
{
ICollection<AzureSqlServerModel> results = null;

if (this.MyInvocation.BoundParameters.ContainsKey("ServerName"))
if (MyInvocation.BoundParameters.ContainsKey("ServerName") && MyInvocation.BoundParameters.ContainsKey("ResourceGroupName"))
{
results = new List<AzureSqlServerModel>();
results.Add(ModelAdapter.GetServer(this.ResourceGroupName, this.ServerName));
}
else if (MyInvocation.BoundParameters.ContainsKey("ResourceGroupName"))
{
results = ModelAdapter.ListServersByResourceGroup(this.ResourceGroupName);
}
else if (!MyInvocation.BoundParameters.ContainsKey("ServerName"))
{
results = ModelAdapter.ListServers();
}
else
{
results = ModelAdapter.GetServers(this.ResourceGroupName);
throw new PSArgumentException("When specifying the serverName parameter the ResourceGroup parameter must also be used");
Copy link
Member

Choose a reason for hiding this comment

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

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]

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

return results;
Expand Down
33 changes: 26 additions & 7 deletions src/ResourceManager/Sql/Commands.Sql/Server/Services/AzureSqlServerAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,34 @@ public AzureSqlServerAdapter(IAzureContext context)
public AzureSqlServerModel GetServer(string resourceGroupName, string serverName)
{
var resp = Communicator.Get(resourceGroupName, serverName);
return CreateServerModelFromResponse(resourceGroupName, resp);
return CreateServerModelFromResponse(resp);
}

/// <summary>
/// Gets a list of all the servers in a subscription
/// </summary>
/// <param name="resourceGroupName">The name of the resource group</param>
/// <returns>A list of all the servers</returns>
public List<AzureSqlServerModel> ListServers()
{
var resp = Communicator.List();
return resp.Select((s) =>
{
return CreateServerModelFromResponse(s);
}).ToList();
}

/// <summary>
/// Gets a list of all the servers in a resource group
/// </summary>
/// <param name="resourceGroupName">The name of the resource group</param>
/// <returns>A list of all the servers</returns>
public List<AzureSqlServerModel> GetServers(string resourceGroupName)
public List<AzureSqlServerModel> ListServersByResourceGroup(string resourceGroupName)
{
var resp = Communicator.List(resourceGroupName);
var resp = Communicator.ListByResourceGroup(resourceGroupName);
return resp.Select((s) =>
{
return CreateServerModelFromResponse(resourceGroupName, s);
return CreateServerModelFromResponse(s);
}).ToList();
}

Expand All @@ -94,7 +108,7 @@ public AzureSqlServerModel UpsertServer(AzureSqlServerModel model)
Version = model.ServerVersion,
});

return CreateServerModelFromResponse(model.ResourceGroupName, resp);
return CreateServerModelFromResponse(resp);
}

/// <summary>
Expand All @@ -113,11 +127,16 @@ public void RemoveServer(string resourceGroupName, string serverName)
/// <param name="resourceGroupName">The resource group the server is in</param>
/// <param name="resp">The management client server response to convert</param>
/// <returns>The converted server model</returns>
private static AzureSqlServerModel CreateServerModelFromResponse(string resourceGroupName, Management.Sql.Models.Server resp)
private static AzureSqlServerModel CreateServerModelFromResponse(Management.Sql.Models.Server resp)
{
AzureSqlServerModel server = new AzureSqlServerModel();

server.ResourceGroupName = resourceGroupName;
// Extract the resource group name from the ID.
// ID is in the form:
// /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rgName/providers/Microsoft.Sql/servers/serverName
string[] segments = resp.Id.Split('/');
server.ResourceGroupName = segments[4];

server.ServerName = resp.Name;
server.ServerVersion = resp.Version;
server.SqlAdministratorLogin = resp.AdministratorLogin;
Expand Down
12 changes: 10 additions & 2 deletions src/ResourceManager/Sql/Commands.Sql/Server/Services/AzureSqlServerCommunicator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,21 @@ public Management.Sql.Models.Server Get(string resourceGroupName, string serverN
}

/// <summary>
/// Lists Azure Sql Database Servers
/// Lists Azure Sql Servers in a resource group
/// </summary>
public IList<Management.Sql.Models.Server> List(string resourceGroupName)
public IList<Management.Sql.Models.Server> ListByResourceGroup(string resourceGroupName)
{
return GetCurrentSqlClient().Servers.ListByResourceGroup(resourceGroupName).ToList();
}

/// <summary>
/// Lists Azure Sql Servers
/// </summary>
public IList<Management.Sql.Models.Server> List()
{
return GetCurrentSqlClient().Servers.List().ToList();
}

/// <summary>
/// Creates or updates a Azure Sql Database SErver
/// </summary>
Expand Down