Websites: Updating websites module to use the latest released .NET SDK version of websites #7127

panchagnula · 2018-09-04T22:10:04Z

Description

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

panchagnula · 2018-09-04T22:10:50Z

CC: @Nking92 as well

maddieclayton · 2018-09-04T23:32:55Z

@panchagnula If you are bumping the API version, you will need to rerecord all the tests.

Edit: see here for the two you need to rerecord: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/7143/artifact/src/Publish/TestResults/FailingTests/Microsoft.Azure.Commands.Websites.Test.dll.html

maddieclayton · 2018-09-04T23:33:15Z

src/ResourceManager/Websites/Commands.Websites.Test/Commands.Websites.Test.csproj

    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Please remove these new references as they are found in the common targets.

@maddieclayton - we had to include this because during our SDK generation we found a bug with provisioning code polling code & we were asked to update the Microsoft.Rest.ClientRuntime version you can see the details here Azure/azure-sdk-for-net#4688

I am going to be adding this back since the common targets version is lower & is causing our Set AppServicePlan, test case to fail.

maddieclayton · 2018-09-04T23:34:07Z

src/ResourceManager/Websites/Commands.Websites.Test/packages.config

  <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.Bcl" version="1.1.10" targetFramework="net45" />
  <package id="Microsoft.Bcl.Async" version="1.0.168" targetFramework="net45" />
  <package id="Microsoft.Bcl.Build" version="1.0.14" targetFramework="net45" />
  <package id="Microsoft.IdentityModel.Clients.ActiveDirectory" version="2.28.3" targetFramework="net45" />
  <package id="Microsoft.Net.Http" version="2.2.29" targetFramework="net45" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.12" targetFramework="net452" />


Remove these two references

maddieclayton · 2018-09-04T23:34:59Z

src/ResourceManager/Websites/Commands.Websites/packages.config

@@ -1,5 +1,8 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
  <package id="Microsoft.Azure.Management.Storage" version="3.0.0" targetFramework="net45" />
-  <package id="Microsoft.Azure.Management.Websites" version="1.7.1-preview" targetFramework="net452" />
+  <package id="Microsoft.Azure.Management.Websites" version="2.0.0" targetFramework="net452" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.12" targetFramework="net452" />


Remove the new references

maddieclayton · 2018-09-04T23:35:35Z

src/ResourceManager/Websites/Commands.Websites/Models.WebApp/PSSite.cs

@@ -55,7 +55,6 @@ public PSSite(Site other)
                  possibleOutboundIpAddresses: other.PossibleOutboundIpAddresses,
                  dailyMemoryTimeQuota: other.DailyMemoryTimeQuota,
                  suspendedTill:other.SuspendedTill,
-                  snapshotInfo: other.SnapshotInfo,


Why is this being removed? Should the property be deprecated?

This property was defined in our APIs but never used for anything. It was removed in the newest API version. There's almost no chance that a customer had a dependency on this property, but we can deprecate it in whatever way is needed.

After reviewing this further I think the real issue is this PSSite wrapper class is extending the SDK model. We could rewrite this class so it no longer extends the SDK model but still exposes all the same properties. We could also keep the SnapshotInfo property that way, although it'd always be empty.

@maddieclayton wanted to make sure you agree with our plan to change the PSSite.cs wrapper to not extend the Site SDK model, we believe this will also fix our breaking changes comment you had raised as well. Let me know if you have other suggestions here.

Sounds good! You could also extend the SDK model but simply add back in the removed properties. Up to you.

Edit: Add back the removed properties as properties of specifically PSSite.

maddieclayton · 2018-09-04T23:37:44Z

...sourceManager/Websites/Commands.Websites/Cmdlets/BackupRestore/RestoreAzureWebAppSnapshot.cs

@@ -48,23 +48,23 @@ public class RestoreAzureWebAppSnapshot : WebAppOptionalSlotBaseCmdlet
        public override void ExecuteCmdlet()
        {
            base.ExecuteCmdlet();
-            Site targetApp = WebsitesClient.GetWebApp(ResourceGroupName, Name, Slot);
-            SnapshotRecoveryTarget target = new SnapshotRecoveryTarget()
+            Site sourceApp = WebsitesClient.GetWebApp(InputObject.ResourceGroupName, InputObject.Name, InputObject.Slot);


Why are you changing these to InputObject.<>? Shouldn't this be using the parameters for retrieving the web app rather than the snapshot?

This is because the API is different now. In previous API versions you would make a recovery request to the app where the snapshot was taken (the source app) and specify which app to restore the snapshot to (the target app) in the request body. The semantics have been switched around. Now you send a RestoreSnapshot request to the target app, and specify the source app inside the request body. The API was changed to fix some scenarios in ARM.

InputObject is the source snapshot, and the *Name parameters are for the target site name. I swapped the code around so the cmdlet will still work the same way it used to, but internally the API flow will be fixed. I hope this won't be considered a breaking change since as far as the end user is concerned, the cmdlet works the same way.

That makes sense to me, thanks for the clarification. That will not be considered a breaking change.

...sourceManager/Websites/Commands.Websites/Cmdlets/BackupRestore/RestoreAzureWebAppSnapshot.cs

maddieclayton · 2018-09-04T23:40:44Z

src/ResourceManager/Websites/Commands.Websites/Commands.Websites.csproj

+    <Reference Include="Microsoft.Azure.Management.Websites, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
+      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Websites.2.0.0\lib\net452\Microsoft.Azure.Management.Websites.dll</HintPath>
+    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime.Azure, Version=3.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Please remove new references, they will be found in common targets.

maddieclayton · 2018-09-04T23:45:44Z

You also have a lot of breaking changes here: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/7143/artifact/src/Package/BreakingChangeIssues.csv/*view*/. You will need to mitigate that in PowerShell. You can do this by adding a PowerShell model wrapper that has all the original properties, and copies over the properties from the returned SDK type. You should add a deprecation message to any of these properties that you want to remove at the breaking change release in November.

panchagnula · 2018-09-06T02:19:02Z

@maddieclayton regarding the breaking changes looking at our commandlets none of these properties are actually being used or referenced anywhere i tried to add the SnapshotInfo property to PSSite.cs, & also added PSAppServicePlan.cs for the AppServicePlanName, however these are not referenced anywhere. I think we can safely supress all these, however, i will defer to your folks judgement, though

cormacpayne · 2018-09-11T16:40:29Z

@panchagnula Hey Sisira, the build is failing after CredScan flagged a few files for containing potential passwords, secrets, keys, etc. You can find the results here: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/7363/artifact/src/Package/CredentialScannerOutput/CredScanIssues.csv

Would you mind taking a look at the files that were flagged and ensure that these are values that anyone else can use? (i.e., they are dummy values or have been deleted since the recording) If it turns out this is the case, we can suppress these exceptions (using the method outlined here).

panchagnula · 2018-09-11T16:44:33Z

@Nking92 the cred scan alerts are from the backup & snapshot tests recordings - can you take a look & answer the question Cormac has here "Would you mind taking a look at the files that were flagged and ensure that these are values that anyone else can use? (i.e., they are dummy values or have been deleted since the recording) If it turns out this is the case, we can suppress these exceptions"

if these can be safely added to the list of exceptions to supress - please let me know & i can take care of it. If you cannot get to it today?

maddieclayton · 2018-09-11T22:24:19Z

Marking as do not merge until the client runtime is bumped in preview. Will take a look at the PR soon.

maddieclayton · 2018-09-12T19:21:33Z

...ResourceManager/Websites/Commands.Websites/Cmdlets/Certificates/GetAzureWebAppCertificate.cs

@@ -19,7 +19,7 @@ namespace Microsoft.Azure.Commands.WebApps.Cmdlets.WebApps
    /// this commandlet will let you get existing web app certificates using ARM APIs
    /// </summary>
    [Cmdlet("Get", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "WebAppCertificate")]
-    [OutputType(typeof(Certificate))]
+    [OutputType(typeof(PSCertificate))]


Looks like you didn't change the output below - unless CmdletHelpers.GetCertificates now returns a PSCertificate

maddieclayton · 2018-09-12T19:22:36Z

src/ResourceManager/Websites/Commands.Websites/Cmdlets/DeploymentSlots/GetAzureWebAppSlot.cs

@@ -26,7 +26,7 @@ namespace Microsoft.Azure.Commands.WebApps.Cmdlets.DeploymentSlots
    /// this commandlet will let you get a new Azure Web app slot using ARM APIs
    /// </summary>
    [Cmdlet("Get", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "WebAppSlot")]
-    [OutputType(typeof(Site))]
+    [OutputType(typeof(PSSite))]


Looks like you didn't change the actual output here either - WebsitesClient.ListWebApps and WebsitesClient.GetWebApp should be wrapped in PSSite

maddieclayton · 2018-09-12T19:23:33Z

src/ResourceManager/Websites/Commands.Websites/Cmdlets/DeploymentSlots/StartAzureWebAppSlot.cs

 using System.Management.Automation;

 namespace Microsoft.Azure.Commands.WebApps.Cmdlets.DeploymentSlots
 {
    /// <summary>
    /// this commandlet will let you Start an Azure Web app slot
    /// </summary>
-    [Cmdlet("Start", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "WebAppSlot"), OutputType(typeof(Site))]
+    [Cmdlet("Start", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "WebAppSlot"), OutputType(typeof(PSSite))]


same comment on actual output

maddieclayton · 2018-09-12T19:24:34Z

src/ResourceManager/Websites/Commands.Websites/Cmdlets/WebApps/NewAzureWebApp.cs

@@ -44,7 +44,7 @@ namespace Microsoft.Azure.Commands.WebApps.Cmdlets.WebApps
    /// <summary>
    /// this commandlet will let you create a new Azure Web app using ARM APIs
    /// </summary>
-    [Cmdlet("New", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "WebApp", DefaultParameterSetName = SimpleParameterSet, SupportsShouldProcess = true), OutputType(typeof(Site))]
+    [Cmdlet("New", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "WebApp", DefaultParameterSetName = SimpleParameterSet, SupportsShouldProcess = true), OutputType(typeof(PSSite))]


same comment about actual output

maddieclayton · 2018-09-12T19:29:53Z

src/ResourceManager/Websites/documentation/upcoming-breaking-changes.md


-**Get-AzureRmAppServicePlan**
- Output type changed from ServerFarmWithRichSku to AppServicePlan
+The following cmdlets were affected by this release:


Looks like you didn't add the upcoming breaking changes?

what is the recommendation here? Give we added the upcoming breaking change attribute to the property do we add the same list here as well? Or do we specify changing the return type to PSSite etc..?

maddieclayton · 2018-09-12T19:30:10Z

src/ResourceManager/Websites/documentation/upcoming-breaking-changes.md


-**Set-AzureRmAppServicePlan**
- Output type changed from ServerFarmWithRichSku to AppServicePlan
+## Release 6.0.0 - May 2018

 **New-AzureRmAppServicePlan**
 - Output type changed from ServerFarmWithRichSku to AppServicePlan


Please remove changes to tools/AzureRM/AzureRM/psm1 below.

maddieclayton · 2018-09-13T01:20:00Z

@panchagnula Looks good! I'll let you know when the clientruntime has been merged. Looks like all that needs to be done is to reenable the skipped test.

maddieclayton · 2018-09-13T20:05:31Z

@panchagnula ClientRuntime PR has been merged! Please unskip the test now and fix the merge conflict.

…uild errors from breaking changes. Re-record backup tests. Recording tests with the new SDK Updating ChangeLog Updating reference to latest websites version Updating WebsitesClientExtension errors, when build using .NetCore Adding recording for 2 missed tests & removing unneeded references as per PR feedback Adding recording of test FW: Adding recording, updating PSSite & adding some breaking changes to the list of supressed changes Re-adding test recording Adding client Runtime back so that the latest version is always used Adding wrapper Models & adding support to handle breaking changes with new SDK Adding the new packages to the package.config to get all tests to pass Bug fixes, updating the return to match the output type, failing tests re-recorded Adding DRTEndpoint parameter to PublishingProfile Commandlet Updating to use the new Breaking change Attribute Disabling one test for the time-being until common targets are updated to use the required client Runtime Fixing all breaking changes & updating the CSV Updating help for the newly added Switch Parameter to PublishingProfile commandlets Fixing Singnature Issues Help update Supressing legitimate test recording cred scan issues for backup&restore tests

…ile to the upstream version

… available via common targets

…hould now work

azuresdkci1x added the needs-review label Sep 4, 2018

maddieclayton suggested changes Sep 4, 2018

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels Sep 4, 2018

maddieclayton assigned panchagnula Sep 4, 2018

panchagnula force-pushed the sisirap-previewSDK branch 2 times, most recently from aee0caf to a47c558 Compare September 8, 2018 00:14

maddieclayton added needs-review and removed needs-revision labels Sep 10, 2018

maddieclayton assigned maddieclayton and unassigned panchagnula Sep 10, 2018

maddieclayton mentioned this pull request Sep 10, 2018

Webapps undelete commands #7165

Merged

8 tasks

panchagnula force-pushed the sisirap-previewSDK branch from 638bb8e to 66f0cfc Compare September 11, 2018 00:50

cormacpayne added needs-revision and removed needs-review labels Sep 11, 2018

cormacpayne assigned panchagnula and unassigned maddieclayton Sep 11, 2018

maddieclayton added the Ignite label Sep 11, 2018

panchagnula force-pushed the sisirap-previewSDK branch 2 times, most recently from 02a947c to 696e77e Compare September 11, 2018 20:18

panchagnula added needs-review and removed needs-revision labels Sep 11, 2018

maddieclayton self-assigned this Sep 11, 2018

maddieclayton added the Do Not Merge 🚫 label Sep 11, 2018

exc3eed pushed a commit to exc3eed/azure-powershell that referenced this pull request Sep 12, 2018

Syncing Azure#7127

376ef03

maddieclayton suggested changes Sep 12, 2018

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels Sep 12, 2018

exc3eed mentioned this pull request Sep 12, 2018

cmdlet changes for creating and managing windows container web app #7191

Merged

8 tasks

panchagnula force-pushed the sisirap-previewSDK branch from f396d46 to 4e0976c Compare September 12, 2018 23:19

maddieclayton removed the needs-revision label Sep 13, 2018

panchagnula force-pushed the sisirap-previewSDK branch from 4e0976c to 75f9e64 Compare September 13, 2018 05:24

maddieclayton added needs-revision and removed Do Not Merge 🚫 labels Sep 13, 2018

Nicholas King and others added 6 commits September 13, 2018 13:52

Adding missing file to cred scan supress list

6a715bd

Updating the output object to match the new Model, & resetting psm1 f…

d21a70d

…ile to the upstream version

Removing the newly added non websites references, since these will be…

3c5df18

… available via common targets

Updating doc with upcoming breaking changes

d77e2fc

Rebased to latest changes on upstream preview& reenabling test that s…

85bd2c9

…hould now work

panchagnula force-pushed the sisirap-previewSDK branch from 75f9e64 to 85bd2c9 Compare September 13, 2018 21:08

maddieclayton changed the base branch from preview to release-2018-09-17 September 13, 2018 21:37

maddieclayton approved these changes Sep 13, 2018

View reviewed changes

maddieclayton added Waiting for CI and removed needs-revision labels Sep 13, 2018

maddieclayton merged commit 1f07dbd into Azure:release-2018-09-17 Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Websites: Updating websites module to use the latest released .NET SDK version of websites #7127

Websites: Updating websites module to use the latest released .NET SDK version of websites #7127

panchagnula commented Sep 4, 2018

panchagnula commented Sep 4, 2018

maddieclayton commented Sep 4, 2018 •

edited

Loading

maddieclayton Sep 4, 2018

panchagnula Sep 6, 2018

maddieclayton Sep 4, 2018

maddieclayton Sep 4, 2018

maddieclayton Sep 4, 2018

nking-1 Sep 5, 2018

nking-1 Sep 5, 2018

panchagnula Sep 5, 2018

maddieclayton Sep 5, 2018 •

edited

Loading

maddieclayton Sep 4, 2018

nking-1 Sep 5, 2018

maddieclayton Sep 5, 2018

maddieclayton Sep 4, 2018

maddieclayton commented Sep 4, 2018

panchagnula commented Sep 6, 2018

cormacpayne commented Sep 11, 2018

panchagnula commented Sep 11, 2018

maddieclayton commented Sep 11, 2018

maddieclayton Sep 12, 2018

panchagnula Sep 12, 2018

maddieclayton Sep 12, 2018

panchagnula Sep 12, 2018

maddieclayton Sep 12, 2018

panchagnula Sep 12, 2018

maddieclayton Sep 12, 2018

panchagnula Sep 12, 2018

maddieclayton Sep 12, 2018

panchagnula Sep 12, 2018

maddieclayton Sep 12, 2018

maddieclayton commented Sep 13, 2018

maddieclayton commented Sep 13, 2018

		</Reference>
		<Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">

Websites: Updating websites module to use the latest released .NET SDK version of websites #7127

Websites: Updating websites module to use the latest released .NET SDK version of websites #7127

Conversation

panchagnula commented Sep 4, 2018

Description

Checklist

panchagnula commented Sep 4, 2018

maddieclayton commented Sep 4, 2018 • edited Loading

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

maddieclayton Sep 5, 2018 • edited Loading

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

maddieclayton commented Sep 4, 2018

panchagnula commented Sep 6, 2018

cormacpayne commented Sep 11, 2018

panchagnula commented Sep 11, 2018

maddieclayton commented Sep 11, 2018

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

maddieclayton commented Sep 13, 2018

maddieclayton commented Sep 13, 2018

maddieclayton commented Sep 4, 2018 •

edited

Loading

maddieclayton Sep 5, 2018 •

edited

Loading