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

[AppService] Fix - bug that prevented changing Container settings in Set-AzWebApp and Set-AzWebAppSlot #12107

Merged
merged 2 commits into from
Jun 11, 2020
Merged

[AppService] Fix - bug that prevented changing Container settings in Set-AzWebApp and Set-AzWebAppSlot #12107

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45db450
Fixed bug that prevented changing Container settings in `Set-AzWebApp…
Jun 10, 2020
3b02066
Adding tests and fixingTest-CreateNewWebAppHyperV false positive
Jun 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Websites/Websites/ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
## Upcoming Release
* Added safeguard to delete created webapp if restore failed in `Restore-AzDeletedWebApp`
* Added "SourceWebApp.Location" for `New-AzWebApp` and `New-AzWebAppSlot`
* Fixed bug that prevented changing Container settings in `Set-AzWebApp` and `Set-AzWebAppSlot`

## Version 1.9.0
* Fixed typo on help of `Update-AzWebAppAccessRestrictionConfig`.
Expand Down
6 changes: 3 additions & 3 deletions src/Websites/Websites/Cmdlets/DeploymentSlots/NewAzureWebAppSlot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,15 @@ private Hashtable GetAppSettingsToUpdate()
Hashtable appSettings = new Hashtable();
if (ContainerRegistryUrl != null)
{
appSettings[CmdletHelpers.DocerRegistryServerUrl] = ContainerRegistryUrl;
appSettings[CmdletHelpers.DockerRegistryServerUrl] = ContainerRegistryUrl;
}
if (ContainerRegistryUser != null)
{
appSettings[CmdletHelpers.DocerRegistryServerUserName] = ContainerRegistryUser;
appSettings[CmdletHelpers.DockerRegistryServerUserName] = ContainerRegistryUser;
}
if (ContainerRegistryPassword != null)
{
appSettings[CmdletHelpers.DocerRegistryServerPassword] = ContainerRegistryPassword.ConvertToString();
appSettings[CmdletHelpers.DockerRegistryServerPassword] = ContainerRegistryPassword.ConvertToString();
}
if (EnableContainerContinuousDeployment.IsPresent)
{
Expand Down
6 changes: 3 additions & 3 deletions src/Websites/Websites/Cmdlets/DeploymentSlots/SetAzureWebAppSlot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,15 +218,15 @@ public override void ExecuteCmdlet()

if (ContainerRegistryUrl != null)
{
appSettings[CmdletHelpers.DocerRegistryServerUrl] = ContainerRegistryUrl;
appSettings[CmdletHelpers.DockerRegistryServerUrl] = ContainerRegistryUrl;
}
if (ContainerRegistryUser != null)
{
appSettings[CmdletHelpers.DocerRegistryServerUserName] = ContainerRegistryUser;
appSettings[CmdletHelpers.DockerRegistryServerUserName] = ContainerRegistryUser;
}
if (ContainerRegistryPassword != null)
{
appSettings[CmdletHelpers.DocerRegistryServerPassword] = ContainerRegistryPassword.ConvertToString();
appSettings[CmdletHelpers.DockerRegistryServerPassword] = ContainerRegistryPassword.ConvertToString();
}

if (parameters.Contains("EnableContainerContinuousDeployment"))
Expand Down
6 changes: 3 additions & 3 deletions src/Websites/Websites/Cmdlets/WebApps/NewAzureWebApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,17 +318,17 @@ private SiteConfig GetNewConfig(AppServicePlan appServiceplan)
}
if (_cmdlet.ContainerRegistryUrl != null)
{
siteConfig.AppSettings.Add(new NameValuePair(CmdletHelpers.DocerRegistryServerUrl, _cmdlet.ContainerRegistryUrl));
siteConfig.AppSettings.Add(new NameValuePair(CmdletHelpers.DockerRegistryServerUrl, _cmdlet.ContainerRegistryUrl));
newConfigAdded = true;
}
if (_cmdlet.ContainerRegistryUser != null)
{
siteConfig.AppSettings.Add(new NameValuePair(CmdletHelpers.DocerRegistryServerUserName, _cmdlet.ContainerRegistryUser));
siteConfig.AppSettings.Add(new NameValuePair(CmdletHelpers.DockerRegistryServerUserName, _cmdlet.ContainerRegistryUser));
newConfigAdded = true;
}
if (_cmdlet.ContainerRegistryPassword != null)
{
siteConfig.AppSettings.Add(new NameValuePair(CmdletHelpers.DocerRegistryServerPassword, _cmdlet.ContainerRegistryPassword.ConvertToString()));
siteConfig.AppSettings.Add(new NameValuePair(CmdletHelpers.DockerRegistryServerPassword, _cmdlet.ContainerRegistryPassword.ConvertToString()));
newConfigAdded = true;
}
if (_cmdlet.EnableContainerContinuousDeployment.IsPresent)
Expand Down
26 changes: 19 additions & 7 deletions src/Websites/Websites/Cmdlets/WebApps/SetAzureWebApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,14 @@ public class SetAzureWebAppCmdlet : WebAppBaseCmdlet
public string ContainerImageName { get; set; }

[Parameter(Mandatory = false, HelpMessage = "Private Container Registry Server Url", ParameterSetName = ParameterSet1Name)]
[ValidateNotNullOrEmpty]
[ValidateNotNull]
public string ContainerRegistryUrl { get; set; }

[Parameter(Mandatory = false, HelpMessage = "Private Container Registry Username", ParameterSetName = ParameterSet1Name)]
[ValidateNotNullOrEmpty]
[ValidateNotNull]
public string ContainerRegistryUser { get; set; }

[Parameter(Mandatory = false, HelpMessage = "Private Container Registry Password", ParameterSetName = ParameterSet1Name)]
[ValidateNotNullOrEmpty]
public SecureString ContainerRegistryPassword { get; set; }

[Parameter(Mandatory = false, HelpMessage = "Enables/Disables container continuous deployment webhook", ParameterSetName = ParameterSet1Name)]
Expand Down Expand Up @@ -220,18 +219,31 @@ public override void ExecuteCmdlet()
}
}


if (ContainerRegistryUrl != null)
{
appSettings[CmdletHelpers.DocerRegistryServerUrl] = ContainerRegistryUrl;
appSettings.Remove(CmdletHelpers.DockerRegistryServerUrl);

if (ContainerRegistryUrl != string.Empty)
{
appSettings[CmdletHelpers.DockerRegistryServerUrl] = ContainerRegistryUrl;
}
}

if (ContainerRegistryUser != null)
{
appSettings[CmdletHelpers.DocerRegistryServerUserName] = ContainerRegistryUser;
appSettings.Remove(CmdletHelpers.DockerRegistryServerUserName);

if (ContainerRegistryUser != string.Empty)
{
appSettings[CmdletHelpers.DockerRegistryServerUserName] = ContainerRegistryUser;
}
}

appSettings.Remove(CmdletHelpers.DockerRegistryServerPassword);

if (ContainerRegistryPassword != null)
{
appSettings[CmdletHelpers.DocerRegistryServerPassword] = ContainerRegistryPassword.ConvertToString();
appSettings[CmdletHelpers.DockerRegistryServerPassword] = ContainerRegistryPassword.ConvertToString();
}

if (parameters.Contains("EnableContainerContinuousDeployment"))
Expand Down
6 changes: 3 additions & 3 deletions src/Websites/Websites/Utilities/CmdletHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ public static NetworkManagementClient networkClient
private const string ApplicationServiceEnvironmentResourceIdFormat =
"/subscriptions/{0}/resourcegroups/{1}/providers/Microsoft.Web/{2}/{3}";

public const string DocerRegistryServerUrl = "DOCKER_REGISTRY_SERVER_URL";
public const string DocerRegistryServerUserName = "DOCKER_REGISTRY_SERVER_USERNAME";
public const string DocerRegistryServerPassword = "DOCKER_REGISTRY_SERVER_PASSWORD";
public const string DockerRegistryServerUrl = "DOCKER_REGISTRY_SERVER_URL";
public const string DockerRegistryServerUserName = "DOCKER_REGISTRY_SERVER_USERNAME";
public const string DockerRegistryServerPassword = "DOCKER_REGISTRY_SERVER_PASSWORD";
public const string DockerEnableCI = "DOCKER_ENABLE_CI";
public const string DockerImagePrefix = "DOCKER|";

Expand Down
26 changes: 14 additions & 12 deletions src/Websites/Websites/Utilities/WebsitesClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,14 +539,6 @@ public void UpdateWebAppConfiguration(string resourceGroupName, string location,

if (useSlot)
{
if (siteConfig != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

@vinisoto - what was the scenario to move siteConfig configuration update to behind the appSettings configuration update. It is reported as regression issue in #14998 . please let me whether I can alter their sequence of updating the configuration or it should be like this for any of the other functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to make sure that the app settings below are updated before or at the same time as WindowsFxVersion (which is in SiteConfig). Otherwise, server side validation for WindowsFxVersion will fail since it will be trying to use "old" container settings.

public const string DockerRegistryServerUrl = "DOCKER_REGISTRY_SERVER_URL";
public const string DockerRegistryServerUserName = "DOCKER_REGISTRY_SERVER_USERNAME";
public const string DockerRegistryServerPassword = "DOCKER_REGISTRY_SERVER_PASSWORD";

Copy link
Contributor

Choose a reason for hiding this comment

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

@vinisoto - For now I have reverted your changes. Because it has created a lot of mess in just few days. can you give me the #GitID for the same. I want to analyze it further to understand the issue and fix it if required.

@panchagnula - FYI.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kotasudhakarreddy we should make sure to fix the issue Vini was working on as well with your changes so that we don't trade one regression for another. i.e this bug #13866

Copy link
Contributor

Choose a reason for hiding this comment

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

@vinisoto - I have reverted your changes only from WebsitesClient.cs->UpdateWebAppConfiguration method. But I don't see any server side validation for setting WindowsFxVersion. Could you please test this PR and confirm us the same.

{
WrappedWebsitesClient.WebApps().UpdateConfigurationSlot(
resourceGroupName,
webSiteName,
siteConfig.ConvertToSiteConfigResource(),
slotName);
}

if (appSettings != null)
{
Expand All @@ -557,6 +549,15 @@ public void UpdateWebAppConfiguration(string resourceGroupName, string location,
slotName);
}

if (siteConfig != null)
{
WrappedWebsitesClient.WebApps().UpdateConfigurationSlot(
resourceGroupName,
webSiteName,
siteConfig.ConvertToSiteConfigResource(),
slotName);
}

if (connectionStrings != null)
{
WrappedWebsitesClient.WebApps().UpdateConnectionStringsSlot(
Expand All @@ -577,10 +578,6 @@ public void UpdateWebAppConfiguration(string resourceGroupName, string location,
}
else
{
if (siteConfig != null)
{
WrappedWebsitesClient.WebApps().UpdateConfiguration(resourceGroupName, webSiteName, siteConfig.ConvertToSiteConfigResource());
}

if (appSettings != null)
{
Expand All @@ -590,6 +587,11 @@ public void UpdateWebAppConfiguration(string resourceGroupName, string location,
new StringDictionary { Properties = appSettings });
}

if (siteConfig != null)
{
WrappedWebsitesClient.WebApps().UpdateConfiguration(resourceGroupName, webSiteName, siteConfig.ConvertToSiteConfigResource());
}

if (connectionStrings != null)
{
WrappedWebsitesClient.WebApps().UpdateConnectionStrings(
Expand Down