Websites: Adding support for MSI & Https in slots #5986

panchagnula · 2018-04-19T01:29:41Z

Description

Enables support for MSI & HttpsOnly settings in Slots. Also changed the MSI & slot properties to bool on webapp as well the cmdlet review description.

NOTE: This has been rebased to the the existing websites PR #5967

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-04-19T01:31:56Z

This change is rebased using the active PR #5967

The actual change is a single commit here cb48f36

cormacpayne · 2018-04-19T21:42:22Z

@maddieclayton assigning to you (corresponding cmdlet review found here)

maddieclayton · 2018-04-20T18:11:38Z

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

@@ -89,10 +89,17 @@ public class SetAzureWebAppSlotCmdlet : WebAppSlotBaseCmdlet
        [Parameter(Mandatory = false, HelpMessage = "Run cmdlet in the background")]
        public SwitchParameter AsJob { get; set; }

+        [Parameter(ParameterSetName = ParameterSet1Name, Position = 16, Mandatory = false, HelpMessage = "Enable MSI on an existing azure webapp")]


Remove positions for these two parameters - there's no reason to have positions on non-mandatory parameters.

maddieclayton · 2018-04-20T18:16:17Z

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

+                        else
+                        {
+                            // disabling the identity property updates the siteconfig only
+                            appSettings["WEBSITE_DISABLE_MSI"] = (!AssignIdentity).ToString();


You should just assign false here - the (!AssignIdentity).ToString() is unnecessary.

maddieclayton · 2018-04-20T18:17:10Z

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

@@ -89,10 +89,17 @@ public class SetAzureWebAppSlotCmdlet : WebAppSlotBaseCmdlet
        [Parameter(Mandatory = false, HelpMessage = "Run cmdlet in the background")]


Please add tests for these new parameters.

Added it for the webapp version

maddieclayton · 2018-04-20T18:20:04Z

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

                    if (parameters.Contains("AppServicePlan"))
                    {
                        WebsitesClient.UpdateWebApp(ResourceGroupName, location, Name, Slot, AppServicePlan);
                    }

+                    if (parameters.Contains("AssignIdentity") || parameters.Contains("HttpsOnly"))


Why do you have a separate if loop here? Is it not the same as if (parameters.Any(p => CmdletHelpers.SiteParameters.Contains(p)))

maddieclayton · 2018-04-20T18:20:38Z

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

@@ -97,11 +97,11 @@ public class SetAzureWebAppCmdlet : WebAppBaseCmdlet
        [Parameter(Mandatory = false, HelpMessage = "Run cmdlet in the background")]
        public SwitchParameter AsJob { get; set; }

-        [Parameter(ParameterSetName = ParameterSet1Name, Mandatory = false, HelpMessage = "Enable MSI on an existing azure webapp")]
-        public SwitchParameter AssignIdentity { get; set; }
+        [Parameter(ParameterSetName = ParameterSet1Name, Position= 16, Mandatory = false, HelpMessage = "Enable MSI on an existing azure webapp")]


Remove position for these two as well.

maddieclayton · 2018-04-20T18:21:56Z

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

@@ -172,10 +173,8 @@ public override void ExecuteCmdlet()
                        else
                        {
                            // disabling the identity property updates the siteconfig only
-                            appSettings["WEBSITE_DISABLE_MSI"] = (!AssignIdentity.IsPresent).ToString();
+                            appSettings["WEBSITE_DISABLE_MSI"] = (!AssignIdentity).ToString();


You should just assign false here - the (!AssignIdentity).ToString() is unnecessary.

maddieclayton · 2018-04-23T18:18:29Z

@panchagnula Can you also update the webappslot tests with your changes? After this, things look mostly good to me, but I'd like to take another quick look once website-preview is merged to preview and this PR only contains your changes.

markcowl · 2018-04-26T17:07:54Z

@panchagnula You will need to pull in the latest and fix the failign tests

markcowl · 2018-04-26T17:25:47Z

@panchagnula Note that you should not use boolean parameters - use switch parameters instead

markcowl · 2018-04-26T19:08:02Z

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

@@ -89,10 +89,17 @@ public class SetAzureWebAppSlotCmdlet : WebAppSlotBaseCmdlet
        [Parameter(Mandatory = false, HelpMessage = "Run cmdlet in the background")]
        public SwitchParameter AsJob { get; set; }

+        [Parameter(ParameterSetName = ParameterSet1Name, Mandatory = false, HelpMessage = "Enable MSI on an existing azure webapp")]


Is this a PATCH or a PUT? If it is a PATCH, this is OK. If it is a PUT, then these should be switch parameters

This is a PATCH Mark, but during our commandlet review (https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/51)we concluded that

this can be confusing for customers, since the rest of the properties use a bool

customers can accidentally turn off the property by running the set command without any parameters

Can i propose that we update all the properties of the SetWebApp & SetWebAppSlot to use the switch parameter as a separate change, i agree switch parameter is the valid PowerShell way , however to have consistency we should change all which i want to avoid this time, since its a bigger change. Let me know your thoughts!

For PATCH boolean parameters are acceptable, for PUT they are not. I am OK with this for now.

Thank you this is the issue to track this #6066

Addressed

markcowl · 2018-04-27T03:55:17Z

on demand: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/779/

panchagnula requested a review from maddieclayton April 19, 2018 01:29

panchagnula added needs-review Do Not Merge 🚫 labels Apr 19, 2018

azuresdkci added the needs-review label Apr 19, 2018

cormacpayne assigned maddieclayton Apr 19, 2018

maddieclayton previously requested changes Apr 20, 2018

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels Apr 20, 2018

maddieclayton assigned panchagnula and unassigned maddieclayton Apr 20, 2018

panchagnula force-pushed the sisirap-BUILD-Merged-MSI branch from cb48f36 to 8581c5c Compare April 20, 2018 23:58

panchagnula added needs-review and removed needs-revision labels Apr 20, 2018

maddieclayton added needs-revision and removed needs-review labels Apr 23, 2018

markcowl removed the Do Not Merge 🚫 label Apr 26, 2018

markcowl changed the base branch from preview to release-6.0.0 April 26, 2018 19:05

panchagnula added 2 commits April 26, 2018 12:49

Adding support for MSI & Https in Slots.

a0a4a4f

Adding tests, updating help

5a92009

markcowl reviewed Apr 26, 2018

View reviewed changes

panchagnula added 2 commits April 26, 2018 14:50

Fixing build

87317a8

Updating the test for set slot & updating the session record

a0a914a

panchagnula force-pushed the sisirap-BUILD-Merged-MSI branch from 8581c5c to a0a914a Compare April 26, 2018 23:06

markcowl approved these changes Apr 27, 2018

View reviewed changes

markcowl added Waiting for On Demand and removed needs-revision labels Apr 27, 2018

markcowl merged commit 05a4844 into Azure:release-6.0.0 Apr 27, 2018

panchagnula deleted the sisirap-BUILD-Merged-MSI branch June 7, 2018 01:15

		@@ -89,10 +89,17 @@ public class SetAzureWebAppSlotCmdlet : WebAppSlotBaseCmdlet
		[Parameter(Mandatory = false, HelpMessage = "Run cmdlet in the background")]

Websites: Adding support for MSI & Https in slots #5986

Websites: Adding support for MSI & Https in slots #5986

Uh oh!

Conversation

panchagnula commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

panchagnula commented Apr 19, 2018

Uh oh!

cormacpayne commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maddieclayton Apr 20, 2018 • 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!

maddieclayton Apr 20, 2018 • 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!

maddieclayton commented Apr 23, 2018

Uh oh!

markcowl commented Apr 26, 2018

Uh oh!

markcowl commented Apr 26, 2018

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!

markcowl Apr 27, 2018 • 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!

markcowl commented Apr 27, 2018

Uh oh!

Uh oh!

panchagnula commented Apr 19, 2018 •

edited

Loading

maddieclayton Apr 20, 2018 •

edited

Loading

maddieclayton Apr 20, 2018 •

edited

Loading

markcowl Apr 27, 2018 •

edited

Loading