-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
Fix bug where New-AzBatchApplicationPackage wouldn't work if allowUpdates was False #24941
Fix bug where New-AzBatchApplicationPackage wouldn't work if allowUpdates was False #24941
Conversation
️✔️Az.Accounts
️✔️Az.Batch
️✔️Az.Network
|
Thank you for your contribution KennethHarmon! We will review the pull request and get back to you soon. |
@microsoft-github-policy-service agree company="Microsoft" |
@microsoft-github-policy-service agree company="Microsoft" |
/azp run azure-powershell - security-tools |
Azure Pipelines successfully started running 1 pipeline(s). |
Hi @KennethHarmon, for this issue, why do we update the function itself rather than the calling for the original method? Because there might be another place calling this function, your update might break other behaviors. |
Hey @NoriZC, I considered that, but it made more sense semantically for me to return whether the package existed already rather than if one was created (I'm aware this is basically the same thing). It's a private function anyway and when I searched only had one reference (the one in this class). Could definitely change it to modify the caller if you'd prefer. |
We usaully put the latest changelog to the head of Upcoming session to avoid mismerge
/azp run azure-powershell - security-tools |
Azure Pipelines successfully started running 1 pipeline(s). |
Description
This PR fixes #23809
Problem:
This issue was caused by a logical error in this function:
private string GetStorageUrl(string resourceGroupName, string accountName, string applicationName, string version, out bool didCreateAppPackage)
{
try
{
// Checks to see if the package exists
var response = BatchManagementClient.ApplicationPackage.Get(
resourceGroupName,
accountName,
applicationName,
version);
didCreateAppPackage = false;
return response.StorageUrl;
}
catch (CloudException exception)
{
// If the application package is not found we want to create a new application package.
if (exception.Response.StatusCode != HttpStatusCode.NotFound)
{
var message = string.Format(Resources.FailedToGetApplicationPackage, applicationName, version, exception.Message);
throw new CloudException(message, exception);
}
}
try
{
var addResponse = BatchManagementClient.ApplicationPackage.Create(
resourceGroupName,
accountName,
applicationName,
version);
// If Application was created we need to return a flag.
didCreateAppPackage = true;
return addResponse.StorageUrl;
}
catch (Exception exception)
{
var message = string.Format(Resources.FailedToAddApplicationPackage, applicationName, version, exception.Message);
throw new NewApplicationPackageException(message, exception);
}
}
azure-powershell/src/Batch/Batch/Models/BatchClient.ApplicationPackages.cs
Lines 192 to 233 in 784c5e0
The flag it is outputting reflects whether or not it created a new app package in the absence of one.
However in it's caller:
azure-powershell/src/Batch/Batch/Models/BatchClient.ApplicationPackages.cs
Lines 116 to 121 in 784c5e0
This flag is being interpreted as whether the application package already existed.
This means that if the package didn't exist, this flag would return true, causing a check on whether updates are allowed for this application, resulting in an error if allowUpdates is false.
Solution:
This fix renames the flag and changes the logic to properly reflect whether the application package already existed or not, preserving the existing logic in the caller method.
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.