Resubmitting AnalysisServices Servicemanagement APIs #3214

athipp · 2016-11-21T16:22:20Z

-Closed the old pr targeting 3.2.

submitting a new PR targeting dev brancha and with suggested naming conventions for the API.

- Added ServerFullName property to powershell type. - Modified tests to validate.

athipp · 2016-11-21T16:31:40Z

abandoned the old PR targeting 3.2.0 (#3192) and created a new one with appropriate naming. @shahabhijeet please take a look when you get a chance.

markcowl · 2016-11-22T01:36:35Z

src/ResourceManager/AnalysisServices/ChangeLog.md

+## Current Release
+
+## Version 3.3.0
+* Fixed bug in Get-AzureRMAnalysisServicesServer


This should go under current release

markcowl · 2016-11-22T01:37:04Z

.../Commands.AnalysisServices.ServiceManagement/AzureRM.AnalysisServices.ServiceManagement.psd1

@@ -0,0 +1,106 @@
+#  


Remove this file

@markcowl ? Dont get it. This is a separate project and separate module. how will the dll be packaged if I remove this.

markcowl · 2016-11-22T01:38:20Z

...mmands.AnalysisServices.ServiceManagement/Commands.AnalysisServices.ServiceManagement.csproj

+    <Reference Include="System.Xml.Linq" />
+    <Reference Include="System.Data" />
+    <Reference Include="System.Xml" />
+    <Reference Include="System.Management.Automation, Version=3.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL" />


A simple reference, without evidence should do

markcowl · 2016-11-22T01:38:31Z

...mmands.AnalysisServices.ServiceManagement/Commands.AnalysisServices.ServiceManagement.csproj

+      <SpecificVersion>False</SpecificVersion>
+      <HintPath>..\..\..\packages\Microsoft.Net.Http.2.2.28\lib\net45\System.Net.Http.Extensions.dll</HintPath>
+    </Reference>
+    <Reference Include="System.Net.Http.Primitives, Version=4.2.28.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">


Is this necessary?

markcowl · 2016-11-22T01:38:38Z

...mmands.AnalysisServices.ServiceManagement/Commands.AnalysisServices.ServiceManagement.csproj

+    <Reference Include="System.Core" />
+    <Reference Include="System.Net" />
+    <Reference Include="System.Net.Http" />
+    <Reference Include="System.Net.Http.Extensions, Version=2.2.28.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">


Is this necessary?

trimmed it. System.Net.Http is necessary.

markcowl · 2016-11-22T01:49:06Z

.../AnalysisServices/Commands.AnalysisServices.ServiceManagement/Models/AsAzureClientSession.cs

+        /// </summary>
+        public static string GetAadAuthenticatedToken(AsAzureContext asAzureContext, SecureString password, PromptBehavior promptBehavior, string clientId, string resourceUri, Uri resourceRedirectUri)
+        {
+            var authUriBuilder = new UriBuilder(asAzureContext.Environment.Endpoints[AsAzureEnvironment.AsRolloutEndpoints.AdAuthorityBaseUrl]);


No, you should not use ADAL directly in the cmdlets - this places a restriction on the ADAL version. Instead, you shoudl use the AuthenticationFactory, or you shoudl use Microsoft.Rest.Azure.Authentication to acquire credentials.

Both of those options require the user to do Login-AzureRmAccount. For this dataplane api that does not work because the token audience is different. We discussed about this and agreed that I'll have to get token on my own.

markcowl · 2016-11-22T01:50:23Z

...er/AnalysisServices/Commands.AnalysisServices.ServiceManagement/Models/AsAzureEnvironment.cs

+
+        public string Name { get; set; }
+
+        public Dictionary<AsRolloutEndpoints, string> Endpoints { get; set; }


Dictionary is not particularly serializable in powershell, consider using a Hashtable instead.

Makes sense.

markcowl · 2016-11-22T01:50:33Z

...anager/AnalysisServices/Commands.AnalysisServices.ServiceManagement/Models/AsAzureProfile.cs

+        /// <summary>
+        /// Gets or sets AS Azure environments.
+        /// </summary>
+        public Dictionary<string, AsAzureEnvironment> Environments { get; set; }


Same comment

Makes sense.

markcowl · 2016-11-22T01:52:19Z

src/ResourceManager/AnalysisServices/AzureRM.AnalysisServices.ServiceManagement.psd1

@@ -0,0 +1,108 @@
+#  


Why is ServiceManagement necessary in this manifest name. Consider a shorter name

@markcowl do you have any suggestions. Given that the assembly name is that I was keeping the .psd1 file name consistent. OR by manifest do you mean something else?

This is a different manifest than the AzureRM.AnalysisServices.psd1. So we need some differentiation...I renamed AzureRM.AnalysisServices.ServiceManagement.psd1 to AzureRM.AnalysisServices.Dataplane .psd1 which is shorter

We talked about using Azure.AnalysisServices as this module name

This is "fixed"

How is it fixed? The file has exactly the same name.

this time for real :) sorry for the double work.

markcowl · 2016-11-22T01:52:35Z

src/ResourceManager/AnalysisServices/Commands.AnalysisServices/Properties/AssemblyInfo.cs

@@ -35,5 +35,5 @@
 // by using the '*' as shown below:


-[assembly: AssemblyVersion("0.0.1")]
-[assembly: AssemblyFileVersion("0.0.1")]
+[assembly: AssemblyVersion("0.0.2")]


This should match the psd1

@markcowl I was told the .psd1 i donot need to update and will be done by powershell team. I can update them both to be 0.0.2

They should match.

this is "fixed"

markcowl · 2016-11-22T01:53:49Z

src/ResourceManager/AnalysisServices/Commands.AnalysisServices.Test/ScenarioTests/AsTests.ps1

+{
+    param
+	(
+		$environment = "aspaaswestusloop1.asazure-int.windows.net",


do not encode internal endpoints

Not sure what you mean here. This is just a default value to the test here. The user will provide this an input to the cmdlet.

markcowl · 2016-11-22T01:54:33Z

src/ResourceManager/AnalysisServices/Commands.AnalysisServices.Test/ScenarioTests/AsTests.cs

+        [Fact]
+        public void TestAnalysisServicesServerRestart()
+        {
+            NewInstance.RunPsTest(string.Format("Test-AnalysisServicesServerRestart -environment '{0}'", "aspaaswestusloop1.asazure-int.windows.net"));


Is there a recording for this test? Also, do not encode internal endppoints, or particular regiosn in test code.

Renaming EnivronmentName parameter to RolloutEnvironment Fixing other review comments.

markcowl · 2016-12-02T07:27:53Z

src/ResourceManager/AnalysisServices/AzureRM.AnalysisServices.ServiceManagement.psd1

@@ -0,0 +1,108 @@
+#  


We talked about using Azure.AnalysisServices as this module name

markcowl · 2016-12-02T07:29:25Z

...urceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Azure.AnalysisServices.psd1

@@ -0,0 +1,106 @@
+#  


This is a duplicate of the previous - the dlls and formats are the same.

markcowl · 2016-12-02T07:29:47Z

...lysisServices/Commands.AnalysisServices.Dataplane/Commands.AnalysisServices.Dataplane.csproj

+    <DelaySign>true</DelaySign>
+    <DebugType>pdbonly</DebugType>
+    <Optimize>true</Optimize>
+    <OutputPath>..\..\..\Package\Release\ResourceManager\AzureResourceManager\AzureRM.AnalysisServices.ServiceManagement</OutputPath>


What is this path? Again, why use AzureRM.AnalysisServices.ServiceManagement

We talked about using

AzureRM.AnalysisServices for the ARM module and
Azure.AnalysisServices for the data plane module. Please use these.

This is not fixed, the path is the same

Fixed for release as well

markcowl · 2016-12-02T07:31:56Z

...ceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Commands/AddAzureASAccount.cs

+        {
+            base.BeginProcessing();
+#pragma warning disable 0618
+            if (RolloutEnvironment == null)


This is redundant, it is taken care of by the ValidateNotNullOrEmpty attribute on the parameter

markcowl · 2016-12-02T07:32:57Z

...ceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Commands/AddAzureASAccount.cs

+        protected override void BeginProcessing()
+        {
+            base.BeginProcessing();
+#pragma warning disable 0618


What are you accessing that is marked as obsolete?

markcowl · 2016-12-02T07:42:26Z

...ager/AnalysisServices/Commands.AnalysisServices.Test/InMemoryTests/AddAzureASAccountTests.cs

+
+        private const string testPassword = "testpassword";
+
+        private AddAzureASAccountCommand cmdlet;


You should create this in the test - it should not be a private member

same for command runtime mock. These might be different for each test

markcowl · 2016-12-02T07:42:54Z

...nager/AnalysisServices/Commands.AnalysisServices.Test/InMemoryTests/DataPlaneCommandTests.cs

+using System.Net;
+using System.Net.Http;
+using System.Reflection;
+using System.Threading;


Please do not have two sets of usings

markcowl · 2016-12-02T07:43:24Z

...nager/AnalysisServices/Commands.AnalysisServices.Test/InMemoryTests/DataPlaneCommandTests.cs

+                                        + "65kxhZWVUbTHaPuEvg03ZQ3esDb6wxQewJPAL-GARg6S9wIN776Esw8-53AWhzFu0fIut-9FXGma6jV7"
+                                        + "MYPoUUcFuQzLZgphecPyMPXSVhummVCdBwX9sizxnmFA";
+
+        private Mock<ICommandRuntime> commandRuntimeMock;


same comment about member variables for values that might change from test to test

markcowl · 2016-12-02T07:45:09Z

src/ResourceManager/AnalysisServices/Commands.AnalysisServices/Properties/AssemblyInfo.cs

@@ -35,5 +35,5 @@
 // by using the '*' as shown below:


The dll-Help.psd1 file is not needed, please remove it.

markcowl · 2016-12-02T07:45:46Z

src/ResourceManager/AnalysisServices/Commands.AnalysisServices/Properties/AssemblyInfo.cs

@@ -35,5 +35,5 @@
 // by using the '*' as shown below:


-[assembly: AssemblyVersion("0.0.1")]
-[assembly: AssemblyFileVersion("0.0.1")]
+[assembly: AssemblyVersion("0.0.2")]


They should match.

athipp · 2016-12-02T18:30:41Z

@markcowl Thanks for the comments. I've fixed all of them now.

athipp · 2016-12-06T00:07:21Z

@markcowl , @cormacpayne, I've addressed all of the comments. thanks!

markcowl

A few issues not fixed. A few comments.

markcowl · 2016-12-07T22:26:30Z

src/ResourceManager/AnalysisServices/AzureRM.AnalysisServices.ServiceManagement.psd1

@@ -0,0 +1,108 @@
+#  


How is it fixed? The file has exactly the same name.

markcowl · 2016-12-07T22:27:59Z

...lysisServices/Commands.AnalysisServices.Dataplane/Commands.AnalysisServices.Dataplane.csproj

+    <DelaySign>true</DelaySign>
+    <DebugType>pdbonly</DebugType>
+    <Optimize>true</Optimize>
+    <OutputPath>..\..\..\Package\Release\ResourceManager\AzureResourceManager\AzureRM.AnalysisServices.ServiceManagement</OutputPath>


The Release path does not match the Debug path

Fixed for release as well

markcowl · 2016-12-07T22:29:06Z

...ceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Commands/AddAzureASAccount.cs

+                password = Credential.Password;
+            }
+
+#pragma warning disable 0618


This is not fixed. The pragma is till there

markcowl · 2016-12-07T22:30:38Z

...anager/AnalysisServices/Commands.AnalysisServices.Test/Commands.AnalysisServices.Test.csproj

    <ProjectReference Include="..\Commands.AnalysisServices\Commands.AnalysisServices.csproj">
      <Project>{8aab43e6-e8f6-4f91-af8a-6a63cd78ef1e}</Project>
      <Name>Commands.AnalysisServices</Name>
    </ProjectReference>
  </ItemGroup>
  <ItemGroup>
+    <None Include="app.config" />


Remove app.config

markcowl · 2016-12-07T22:31:16Z

src/ResourceManager/AnalysisServices/Commands.AnalysisServices.Test/app.config

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="utf-8"?>


Remove this file

markcowl · 2016-12-07T22:33:33Z

signing run here:http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-sign/740/

shahabhijeet · 2016-12-15T01:42:43Z

@athipp can you address the feedback on this PR. This PR will be closed by EOW. Please reopen the PR or submit a new PR.

cormacpayne · 2016-12-21T17:33:33Z

@athipp closing this PR since there has been no activity for a couple of weeks

athipp · 2017-01-09T00:23:20Z

...lysisServices/Commands.AnalysisServices.Dataplane/Commands.AnalysisServices.Dataplane.csproj

+    <DelaySign>true</DelaySign>
+    <DebugType>pdbonly</DebugType>
+    <Optimize>true</Optimize>
+    <OutputPath>..\..\..\Package\Release\ResourceManager\AzureResourceManager\AzureRM.AnalysisServices.ServiceManagement</OutputPath>


Fixed for release as well

athipp · 2017-01-09T00:25:42Z

...ceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Commands/AddAzureASAccount.cs

+                password = Credential.Password;
+            }
+
+#pragma warning disable 0618


really fixed. :(

athipp · 2017-01-09T00:26:10Z

...anager/AnalysisServices/Commands.AnalysisServices.Test/Commands.AnalysisServices.Test.csproj

    <ProjectReference Include="..\Commands.AnalysisServices\Commands.AnalysisServices.csproj">
      <Project>{8aab43e6-e8f6-4f91-af8a-6a63cd78ef1e}</Project>
      <Name>Commands.AnalysisServices</Name>
    </ProjectReference>
  </ItemGroup>
  <ItemGroup>
+    <None Include="app.config" />


athipp · 2017-01-09T00:28:36Z

src/ResourceManager/AnalysisServices/Commands.AnalysisServices/Properties/AssemblyInfo.cs

@@ -35,5 +35,5 @@
 // by using the '*' as shown below:


-[assembly: AssemblyVersion("0.0.1")]
-[assembly: AssemblyFileVersion("0.0.1")]
+[assembly: AssemblyVersion("0.0.2")]


Fixed all the current set of comments. Reopening this review for submission.

This reverts commit 27c95dc.

This reverts commit f886901.

shahabhijeet · 2017-01-09T23:18:15Z

@athipp the setup is failing as it cannot find the below file
'D:\workspace\powershell\setup..\src\Package\Debug\ResourceManager\AzureResourceManager\AzureRM.AnalysisServices\Microsoft.Azure.Commands.AnalysisServices.dll-help.xml'

athipp · 2017-01-10T01:16:28Z

@markcowl , @cormacpayne . I've addressed the previous comments and reopened the PR. Also addded platyps based help. please take a look. thanks!

markcowl · 2017-01-11T01:16:00Z

...ceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Commands/AddAzureASAccount.cs

+
+        public void OnImport()
+        {
+            // Nothing to do on assembly initialize


You should be defining your aliases here

I thought aliases are defined at the beginning of the cmdlet. Not sure what you mean here

This is Fixed.

markcowl · 2017-01-11T01:19:34Z

...eManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Models/AsAzureClientSession.cs

+
+        public static Dictionary<string, AsAzureAuthInfo> AsAzureRolloutEnvironmentMapping = new Dictionary<string, AsAzureAuthInfo>()
+            {
+             { "asazure-int.windows.net", new AsAzureAuthInfo()


You should not include internal endpoints here. No customers will ever use this. If using the internal endpoints is needed, you should have a mechanism for adding a new environment

I will add a work item to externalize this endpoint into the environment. But i suspect I will not have time for that this release. I'll take a bug and do it in the next milestone.

markcowl · 2017-01-11T01:29:15Z

...ceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Commands/AddAzureASAccount.cs

+            base.BeginProcessing();
+            if (string.IsNullOrEmpty(RolloutEnvironment))
+            {
+                RolloutEnvironment = AsAzureClientSession.GetDefaultEnvironmentName();


I strongly encourage you from moving away from a static Session structure - this causes problems when running cmdlets in parallel. For a future release, you should consider changing this to a Session variable.,

I will take this as a bug and fix it in the next release.

I've opened an issue - to track this. #3380

markcowl · 2017-01-11T01:31:34Z

...urceManager/AnalysisServices/Commands.AnalysisServices.Dataplane/Azure.AnalysisServices.psd1

+# Module manifest for module 'Microsoft.Azure.Commands.AnalysisServices.Dataplane'  
+#  
+# Generated by: Microsoft Corporation  
+#  


You will need to include this module in the AzureRM module

Not sure how I go about doing that. Any pointers?

@athipp I will take care of adding this module in AzureRM, so you can ignore this.

shahabhijeet · 2017-01-11T22:55:17Z

Running on demand
http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1373/

athipp added 7 commits November 1, 2016 14:14

Need to check for null when creating Tags.

d84e0bc

Merge remote-tracking branch 'Azure/release-3.1.0' into dev

166292e

Fixing bugs and modifying tests for analysis services commands

9e727f2

- Added ServerFullName property to powershell type. - Modified tests to validate.

Merge remote-tracking branch 'Azure/dev' into dev

37f7df4

Add changelog.md and bumpup version

bb3f93c

Merge remote-tracking branch 'Azure/dev' into dev

6fb8d7a

Service management apis for ananlysis services.

9e9f1eb

azurecla added the cla-not-required label Nov 21, 2016

Updating target version in changelog.md

b8a2180

markcowl requested changes Nov 22, 2016

View reviewed changes

athipp added 10 commits November 28, 2016 08:43

Incorporating review comments

5a79985

fixing help file

504c2d6

Adding help

c39ebfc

Remove unused .csproj file

f506280

Changing ServiceManagement to Dataplane

bf0aeb2

Changing parameter name for login command

77cc0a5

Updating xml help file

5abad7d

Removing old help files

1137bed

Adding Inmemory tests for Dataplane API

66f85fa

Renaming EnivronmentName parameter to RolloutEnvironment Fixing other review comments.

Fixing a build issue

fa9b99e

markcowl requested changes Dec 2, 2016

View reviewed changes

athipp added 2 commits December 2, 2016 09:40

Fixing bugs

0431b8f

Review comments and bug fix

c12c0dc

athipp added 4 commits December 2, 2016 11:50

Bug fixes

f3bfd84

Merge remote-tracking branch 'Azure/dev' into dev

43f80e3

Fixing build break

dc2e3ff

adding ignored file.

913b2c7

cormacpayne added Changes Requested and removed Changes Requested labels Dec 5, 2016

cormacpayne added the Sign Off Required label Dec 6, 2016

markcowl requested changes Dec 7, 2016

View reviewed changes

markcowl added Changes Requested and removed Sign Off Required labels Dec 7, 2016

cormacpayne closed this Dec 21, 2016

Merge remote-tracking branch 'Azure/dev' into dev

c8d7c9e

athipp commented Jan 9, 2017

View reviewed changes

athipp reopened this Jan 9, 2017

azurecla added the cla-not-required label Jan 9, 2017

athipp added 6 commits January 8, 2017 17:16

fixing review comments

984cc29

Remove dll-help file from wxi project

f886901

Removing invalid ref

27c95dc

Revert "Removing invalid ref"

74817f3

This reverts commit 27c95dc.

Revert "Remove dll-help file from wxi project"

ecb20b5

This reverts commit f886901.

Bringing back the dll-help files

0fbc7af

athipp added 2 commits January 9, 2017 15:31

adding back reference to dll-help file

721374c

Platyps based help files for analysis services

5634b44

markcowl requested changes Jan 11, 2017

View reviewed changes

Resolving comments

cabdab0

shahabhijeet merged commit 2526f28 into Azure:dev Jan 12, 2017


		public string Name { get; set; }

		public Dictionary<AsRolloutEndpoints, string> Endpoints { get; set; }


		private const string testPassword = "testpassword";

		private AddAzureASAccountCommand cmdlet;

Resubmitting AnalysisServices Servicemanagement APIs #3214

Resubmitting AnalysisServices Servicemanagement APIs #3214

Uh oh!

Conversation

athipp commented Nov 21, 2016

Uh oh!

athipp commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athipp Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athipp Nov 24, 2016 •

edited

Loading